SteNicholas commented on code in PR #3855: URL: https://github.com/apache/celeborn/pull/3855#discussion_r4134396519
########## 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: [P2] Bound the HTTP requests before waiting for the drain Could the scale lookup and exit POSTs have explicit connection and total request deadlines (including the wget path)? If the API server accepts the connection but stops responding, this curl call waits indefinitely, so the hook never reaches either the exit request or its graceful fallback. Kubernetes counts preStop against terminationGracePeriodSeconds, so the pod can spend the entire six-hour grace period here and then be killed without starting decommissioning. A stalled first exit endpoint likewise prevents trying loopback. Only the final wait after a successful exit request should be allowed to consume the drain budget. ########## 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: [P2] Scope built-in trigger queries to the target release The selector does not identify the Helm release when zones are disabled or when metricsLabel is enabled (the default). I rendered releases alpha and beta in namespace shared: their ScaledObjects target different StatefulSets but both disk and memory queries are identical. The same happens with zones enabled when both releases use the same zone name. Consequently, a busy worker in alpha makes beta scale out too, and prevents beta from scaling back down independently. Please always constrain the selector to the target StatefulSet, e.g. with the pod-name matcher already used in the fallback branch, or publish and select a release-specific metric label. A render test comparing two releases in the same namespace would catch this. -- 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]
