strelok89 commented on code in PR #3855:
URL: https://github.com/apache/celeborn/pull/3855#discussion_r4056424489
##########
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:
Agreed, fixed in `df4aa1e6a`.
Reproduced: with `worker.replicas: 6` and zone `a` set to `replicas: 0`, it
rendered `3` (the ceil-split) rather than `0`, so a zone could not be parked
without removing it from the list.
Now tests for key presence — `hasKey .zone "replicas"` plus a `kindIs
"invalid"` guard so an explicitly null value still falls back to the split.
Regression test covers `0` on one zone while its sibling keeps the split.
##########
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:
Agreed, fixed in `df4aa1e6a`.
Reproduced with a values file: a zone with `affinity: {}` kept the chart's
own `podAntiAffinity`, which contradicts the documented "replaces
`worker.affinity`" behaviour and left no way to clear it for a single zone. Now
keyed on `hasKey` rather than truthiness, so `{}` clears it while a zone with
no `affinity` key still inherits the global one — both covered by a regression
test.
Worth noting for anyone reproducing this: `--set 'zones[0].affinity=null'`
is misleading here, because Helm removes a key set to null, so `hasKey` is
false and the behaviour looks unchanged. It needs a values file to see.
--
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]
