strelok89 commented on code in PR #3855: URL: https://github.com/apache/celeborn/pull/3855#discussion_r4181035671
########## charts/celeborn/files/worker-drain.sh: ########## @@ -0,0 +1,119 @@ +#!/bin/sh +# +# 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. +# +# preStop hook for a Celeborn worker. Asks the worker to decommission when a scale-in is +# removing it, and to shut down gracefully otherwise. See the chart's README.md. +# +# Reads POD_NAME, POD_IP, STS_NAME, WORKER_HTTP_PORT and ALWAYS_DECOMMISSION from the +# environment. + +set -u + +SA=/var/run/secrets/kubernetes.io/serviceaccount + +# preStop output is not collected, so log through the main process. +log() { + _msg="celeborn preStop: $*" + if [ -w /proc/1/fd/1 ]; then + echo "$_msg" >/proc/1/fd/1 + else + echo "$_msg" + fi +} + +ORDINAL="" +DESIRED="" +EXIT_TYPE=GRACEFUL + +if [ "${ALWAYS_DECOMMISSION:-false}" = "true" ]; then + # Graceful shutdown persists state to recover from on restart. Where the worker's storage + # does not outlive the pod there is nothing to recover, so a rollout would drop in-flight + # shuffles - drain every time instead, whatever is removing this pod. + EXIT_TYPE=DECOMMISSION + log "alwaysDecommission is set - draining regardless of why this pod is going away" +else + # A scale-in must decommission, waiting for the shuffle data to expire. A rolling update or + # a node drain must not, or every pod replacement would block for hours. The statefulset + # controller lowers spec.replicas before deleting pods on a scale-in, so an ordinal at or + # above the desired count is being removed for good. + ORDINAL=${POD_NAME:-} + ORDINAL=${ORDINAL##*-} + + if [ -r "$SA/token" ]; then + URL="https://kubernetes.default.svc/apis/apps/v1/namespaces/$(cat "$SA/namespace")/statefulsets/${STS_NAME:-}/scale" + AUTH="Authorization: Bearer $(cat "$SA/token")" + if command -v curl >/dev/null 2>&1; then + SCALE=$(curl -sS --cacert "$SA/ca.crt" -H "$AUTH" "$URL" 2>/dev/null) Review Comment: Good catch - fixed in 07a337d. The scale lookup and the exit POST now carry `--connect-timeout 5 --max-time 10` (curl) / `-T 10` (wget, the one timeout flag both busybox and GNU accept), so neither can eat the grace period or block the loopback fallback. `--tries=1` is deliberately not passed: busybox rejects it, and that option error in the exit loop would fall straight through to SIGTERM. Only the wait after a successful exit request is still unbounded, as you suggest. Bounding the POST itself is safe because `WorkerStatusManager.exit` hands the drain to a separate `worker-exit-thread` and the handler returns immediately, so a 10s deadline cannot cut a decommission short. ########## charts/celeborn/templates/worker/_helpers.tpl: ########## @@ -76,8 +84,76 @@ 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 -}} +{{- /* Key presence, not truthiness: an explicit 0 parks a zone without removing it. */ -}} +{{- if and (hasKey .zone "replicas") (not (kindIs "invalid" .zone.replicas)) -}} +{{ .zone.replicas }} +{{- else -}} +{{ divf .Values.worker.replicas (len .Values.worker.zoneAwareReplication.zones) | ceil | int }} +{{- end }} +{{- else -}} +{{ .Values.worker.replicas }} +{{- end }} +{{- end }} + +{{/* +Label selector used by the built-in autoscaling triggers. Scopes the query to this +statefulset's workers: by the `zone` label the worker publishes when it has one, and by pod +name when the zone metric label is turned off. +*/}} +{{- define "celeborn.worker.autoscaling.selector" -}} +{{- $selector := printf "role=\"Worker\",namespace=\"%s\"" .Release.Namespace -}} Review Comment: You're right, and the collision happens with zones disabled too - neither `role="Worker",namespace=...` nor the `zone` label says anything about which release a worker belongs to. Fixed in 48ff209: the selector now always constrains on `pod=~"<statefulset-name>-[0-9]+"`, which carries both the release name and the zone, and the `metricsLabel`/zone branching is gone. `metricsLabel` still controls the label the worker publishes, for queries of your own. Reproduced your case - `helm template alpha|beta -n shared`, zones on, same zone name - and the queries now differ: ``` ...pod=~"alpha-celeborn-worker-us-east-1a-[0-9]+" ...pod=~"beta-celeborn-worker-us-east-1a-[0-9]+" ``` Added render tests for two releases in one namespace, with and without a shared zone name. -- 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]
