Copilot commented on code in PR #3855:
URL: https://github.com/apache/celeborn/pull/3855#discussion_r4047267062
##########
charts/celeborn/templates/worker/statefulset.yaml:
##########
@@ -103,15 +131,51 @@ spec:
true; do
echo "waiting for master";
sleep 2;
- done && exec /opt/celeborn/sbin/start-worker.sh
+ {{- /*
+ Publish the zone as a metric label, so queries select on it rather
than on pod names.
+ The conf file is loaded with setIfMissing, so a system property wins
over it, and
+ start-worker.sh builds CELEBORN_JAVA_OPTS from
CELEBORN_WORKER_JAVA_OPTS - so this
+ composes with whatever `worker.env` already sets.
+ */}}
+ {{- $zoneOpts := "" }}
+ {{- if and .zone .Values.worker.zoneAwareReplication.metricsLabel }}
+ {{- $extraLabels := printf "zone=%s" .zone.name }}
+ {{- with get .Values.celeborn "celeborn.metrics.extraLabels" }}
+ {{- $extraLabels = printf "%s,%s" . $extraLabels }}
+ {{- end }}
+ {{- $zoneOpts = printf "export
CELEBORN_WORKER_JAVA_OPTS=\"${CELEBORN_WORKER_JAVA_OPTS:-}
-Dceleborn.metrics.extraLabels=%s\" && " $extraLabels }}
Review Comment:
The configured `celeborn.metrics.extraLabels` value is interpolated inside a
shell double-quoted command. Valid label values containing shell expansion
syntax (for example `$()` or backticks) will be evaluated by `/bin/sh`, and
quotes can break the command, so enabling zone labels can alter or prevent
worker startup based on an otherwise valid metrics configuration. Pass the
value through an environment variable or otherwise shell-escape it before
constructing this command.
##########
charts/celeborn/templates/worker/statefulset.yaml:
##########
@@ -153,14 +231,23 @@ spec:
path: log4j2.xml
- key: metrics.properties
path: metrics.properties
+ {{- if $drainMount }}
+ - key: worker-drain.sh
+ path: worker-drain.sh
+ {{- end }}
{{- with .Values.worker.volumes }}
{{- toYaml . | nindent 6 }}
{{- end }}
- {{- with .Values.worker.nodeSelector }}
+ {{- $nodeSelector := merge (deepCopy (default dict (and .zone
.zone.nodeSelector))) (default dict .Values.worker.nodeSelector) }}
+ {{- with $nodeSelector }}
nodeSelector:
{{- toYaml . | nindent 8 }}
{{- end }}
- {{- with .Values.worker.affinity }}
+ {{- $affinity := .Values.worker.affinity }}
+ {{- if and .zone .zone.affinity }}
+ {{- $affinity = .zone.affinity }}
+ {{- end }}
Review Comment:
An explicitly configured empty `zones[].affinity: {}` is falsey in Helm, so
this branch is skipped and the global `worker.affinity` is retained. That
contradicts the documented replacement behavior and prevents a zone from
deliberately clearing the global affinity; check whether the key exists rather
than whether the map is non-empty.
##########
charts/celeborn/templates/worker/_helpers.tpl:
##########
@@ -76,8 +84,75 @@ Create the name of the worker priority class to use
Create the name of the worker statefulset to use
*/}}
{{- define "celeborn.worker.statefulSet.name" -}}
+{{- if .zone -}}
+{{ include "celeborn.fullname" . }}-worker-{{ .zone.name }}
+{{- else -}}
{{ include "celeborn.fullname" . }}-worker
{{- end }}
+{{- end }}
+
+{{/*
+Number of replicas for a worker statefulset. Without zone-aware replication
this is
+`worker.replicas` as-is; with it, `worker.replicas` is the total across all
zones and each
+zone gets `ceil(replicas / zones)` unless the zone overrides it.
+*/}}
+{{- define "celeborn.worker.replicas" -}}
+{{- if .zone -}}
+{{- if .zone.replicas -}}
Review Comment:
`if .zone.replicas` treats an explicit `0` as unset, so a zone configured
with `replicas: 0` is rendered with the ceil-split count instead. This makes it
impossible to intentionally disable one zone (and is inconsistent with the
chart's support for scaling to zero); test for key presence rather than
truthiness.
--
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]
