zuozhiw commented on code in PR #5377:
URL: https://github.com/apache/texera/pull/5377#discussion_r4346774161
##########
bin/k8s/templates/base/workflow-computing-unit-manager/workflow-computing-unit-manager-deployment.yaml:
##########
@@ -151,4 +151,5 @@ spec:
{{- range .Values.texeraEnvVars }}
- name: {{ .name }}
value: "{{ .value }}"
- {{- end }}
\ No newline at end of file
+ {{- end }}
+ {{- include "texera.observability.env" . | nindent 12 }}
Review Comment:
Enabling telemetry on the manager does not enable it in the CU pods that
actually run workflows. The manager creates those pods from
`ComputingUnitManagingResource.computingUnitEnvironmentVariables`, a separate
environment map that omits the observability settings; `KubernetesClient` uses
that map for the container environment.
For example, an operator can enable telemetry and point the manager at the
collector, then launch a workflow whose CU still uses the SDK-disabled default.
The collector could receive manager telemetry while receiving no CU logs,
metrics, or traces from that execution, precisely where a worker failure needs
to be investigated.
Please forward the SDK enable/disable setting, OTLP endpoint, allowed hosts,
metric interval, and deployment resource attributes into newly created CUs.
Give each CU its own instance ID rather than copying the manager's. A unit test
of the environment-building helper with telemetry enabled/disabled would cover
this without starting Kubernetes.
##########
bin/k8s/templates/base/observability/grafana.yaml:
##########
@@ -0,0 +1,152 @@
+{{/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/}}
+
+{{- if and .Values.observability.enabled .Values.observability.grafana.enabled
}}
+# Grafana: dashboard/UI, k8s counterpart of the docker-compose grafana
+# service. Reads the Apache-2.0 backends (Loki, VictoriaMetrics, Jaeger)
+# as pre-provisioned datasources via the ConfigMap below. Grafana is
+# AGPLv3, so it is an optional operator-run component, not part of any
+# Texera release artifact (see bin/observability/README.md). Only this
+# ASF-licensed template lives in the tree. The Service is ClusterIP (no
+# Ingress), matching the rest of the stack; expose it via your own
+# Ingress / port-forward to reach the UI.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ .Release.Name }}-grafana-datasources
+ namespace: {{ .Release.Namespace }}
+ labels:
+ app: {{ .Release.Name }}-grafana
+data:
+ datasources.yaml: |
+ apiVersion: 1
+ datasources:
+ # Metrics: VictoriaMetrics speaks the Prometheus query API. Default.
+ - name: Metrics
+ type: prometheus
+ uid: metrics
+ access: proxy
+ url: http://{{ .Release.Name }}-victoriametrics:{{
.Values.observability.metrics.port }}
+ isDefault: true
+ editable: false
+ {{- if .Values.observability.logs.enabled }}
+ # Logs: Loki, LogQL over OTLP-ingested logs.
+ - name: Loki
+ type: loki
+ uid: loki
+ access: proxy
+ url: http://{{ .Release.Name }}-loki:{{
.Values.observability.logs.port }}
+ editable: false
+ {{- end }}
+ {{- if .Values.observability.traces.enabled }}
+ # Traces: Jaeger query API / UI backend.
+ - name: Jaeger
+ type: jaeger
+ uid: jaeger
+ access: proxy
+ url: http://{{ .Release.Name }}-jaeger:{{
.Values.observability.traces.queryPort }}
+ editable: false
+ {{- end }}
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: {{ .Release.Name }}-grafana
+ namespace: {{ .Release.Namespace }}
+ labels:
+ app: {{ .Release.Name }}-grafana
+spec:
+ replicas: 1
+ selector:
+ matchLabels:
+ app: {{ .Release.Name }}-grafana
+ template:
+ metadata:
+ labels:
+ app: {{ .Release.Name }}-grafana
+ annotations:
+ # Roll the pod when the provisioned datasources change.
+ checksum/config: {{ include (print $.Template.BasePath
"/base/observability/grafana.yaml") . | sha256sum }}
Review Comment:
Enabling observability makes Helm rendering fail, so an operator cannot
install or upgrade the chart with this stack enabled. This checksum includes
the same template file that contains the checksum. Rendering the file therefore
calls itself repeatedly. Loki's `checksum/config` annotation has the same
problem, so disabling only Loki still leaves Grafana failing.
Please put each ConfigMap/config body in a separate template or helper, then
hash just that content. For example, the Grafana Deployment should hash a
helper that renders its datasource configuration, not the template that renders
the Deployment itself.
Please add a Helm render check with observability enabled; the default-off
configuration skips these templates and hides the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
