Follow-up question on #517, about the ordering of the two guards at the top of syncWorkerToStore.
The gate
syncWorkerToStore checks pod eligibility before it checks for deletion:
|
func (s *WorkerPoolSyncer) syncWorkerToStore(ctx context.Context, pod *corev1.Pod) { |
|
if !isWorkerEligible(pod) { |
|
return |
|
} |
|
|
|
if pod.DeletionTimestamp != nil { |
|
// The pod has entered Terminating: mark the worker DRAINING so the |
|
// scheduler stops routing new actors to it. We deliberately do NOT touch |
|
// the bound actor here — inside the pod ateom has received SIGTERM and is |
|
// gracefully shutting the actor down. Actor cleanup happens on the Pod |
|
// Deleted event. |
|
if err := s.markWorkerDraining(ctx, pod.Namespace, pod.Labels[workerPodLabel], pod.Name); err != nil { |
|
slog.ErrorContext(ctx, "Failed to mark worker draining", slog.String("worker", pod.Namespace+"/"+pod.Name), slog.Any("err", err)) |
|
} |
|
return |
|
} |
isWorkerEligible is pod.Status.PodIP != "". So a Terminating pod whose status carries no IP returns at the first guard and never reaches markWorkerDraining, even though the draining transition itself never looks at the pod IP: it resolves the stored record by namespace, pool and pod name.
Adding this test next to TestSyncer_SoftDelete_MarksDraining, identical except that Status has no IP, fails on main:
deleting := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: pod,
Namespace: ns,
Labels: map[string]string{workerPodLabel: pool},
DeletionTimestamp: &metav1.Time{Time: time.Unix(1, 0)},
},
Status: corev1.PodStatus{},
}
s.syncWorkerToStore(ctx, deleting)syncer_test.go:450: worker state = STATE_ACTIVE, want DRAINING
Can a Terminating pod actually report no IP
As far as I can tell from the kubelet source, yes. convertStatusToAPIStatus rebuilds PodIPs from the runtime's podStatus.IPs on every status generation, and for non host network pods there is no fallback to oldPodStatus:
https://github.com/kubernetes/kubernetes/blob/v1.35.4/pkg/kubelet/kubelet_pods.go#L2080-L2094
Neither pkg/kubelet/status/status_manager.go nor pkg/kubelet/kubelet.go writes PodIP or PodIPs, so nothing preserves the previous value on the way to the API server. Once the sandbox is torn down while the pod object is still present, the regenerated status has an empty podIP.
Where I am unsure, and why I am asking rather than sending a PR
In the ordinary graceful path this window looks harmless. It opens only after the containers have exited, and the Pod Deleted event follows shortly after and runs reconcileDeadWorker, which releases the actor and deletes the record anyway. A missed DRAINING mark there changes nothing observable.
It looks like it matters only when a pod stays Terminating after its sandbox is gone, for example a hung CSI volume unmount or a finalizer holding the pod. For as long as that lasts, the worker stays STATE_ACTIVE and the scheduler keeps treating it as free capacity, so an actor placed on it is scheduled onto a pod that is already on its way out. With TerminationGracePeriodSeconds now defaulting to 300 there is also more wall clock time in the terminating state generally.
I cannot reproduce that stuck-Terminating case in a unit test, so I do not know whether it is worth guarding against in practice. You have much better visibility into how often worker pods get stuck terminating in real clusters.
Possible change
Moving the deletion check ahead of the eligibility check is enough, and does not need any other adjustment. A Terminating pod that never had a worker record is already handled: markWorkerDraining returns nil on store.ErrNotFound, which TestMarkWorkerDraining/worker_not_found_returns_nil covers.
func (s *WorkerPoolSyncer) syncWorkerToStore(ctx context.Context, pod *corev1.Pod) {
- if !isWorkerEligible(pod) {
- return
- }
-
if pod.DeletionTimestamp != nil {
...
return
}
+ if !isWorkerEligible(pod) {
+ return
+ }
+
poolName := pod.Labels[workerPodLabel]I have this locally with the test above, go build ./..., go vet ./cmd/ateapi/... and all of ./cmd/ateapi/... passing. Happy to send it as a PR if you think the stuck-Terminating window is worth handling, or to close this if the Deleted event is considered sufficient.
Follow-up question on #517, about the ordering of the two guards at the top of
syncWorkerToStore.The gate
syncWorkerToStorechecks pod eligibility before it checks for deletion:substrate/cmd/ateapi/internal/controlapi/syncer.go
Lines 110 to 125 in 87698ea
isWorkerEligibleispod.Status.PodIP != "". So a Terminating pod whose status carries no IP returns at the first guard and never reachesmarkWorkerDraining, even though the draining transition itself never looks at the pod IP: it resolves the stored record by namespace, pool and pod name.Adding this test next to
TestSyncer_SoftDelete_MarksDraining, identical except thatStatushas no IP, fails onmain:Can a Terminating pod actually report no IP
As far as I can tell from the kubelet source, yes.
convertStatusToAPIStatusrebuildsPodIPsfrom the runtime'spodStatus.IPson every status generation, and for non host network pods there is no fallback tooldPodStatus:https://github.com/kubernetes/kubernetes/blob/v1.35.4/pkg/kubelet/kubelet_pods.go#L2080-L2094
Neither
pkg/kubelet/status/status_manager.gonorpkg/kubelet/kubelet.gowritesPodIPorPodIPs, so nothing preserves the previous value on the way to the API server. Once the sandbox is torn down while the pod object is still present, the regenerated status has an emptypodIP.Where I am unsure, and why I am asking rather than sending a PR
In the ordinary graceful path this window looks harmless. It opens only after the containers have exited, and the Pod Deleted event follows shortly after and runs
reconcileDeadWorker, which releases the actor and deletes the record anyway. A missed DRAINING mark there changes nothing observable.It looks like it matters only when a pod stays Terminating after its sandbox is gone, for example a hung CSI volume unmount or a finalizer holding the pod. For as long as that lasts, the worker stays
STATE_ACTIVEand the scheduler keeps treating it as free capacity, so an actor placed on it is scheduled onto a pod that is already on its way out. WithTerminationGracePeriodSecondsnow defaulting to 300 there is also more wall clock time in the terminating state generally.I cannot reproduce that stuck-Terminating case in a unit test, so I do not know whether it is worth guarding against in practice. You have much better visibility into how often worker pods get stuck terminating in real clusters.
Possible change
Moving the deletion check ahead of the eligibility check is enough, and does not need any other adjustment. A Terminating pod that never had a worker record is already handled:
markWorkerDrainingreturns nil onstore.ErrNotFound, whichTestMarkWorkerDraining/worker_not_found_returns_nilcovers.func (s *WorkerPoolSyncer) syncWorkerToStore(ctx context.Context, pod *corev1.Pod) { - if !isWorkerEligible(pod) { - return - } - if pod.DeletionTimestamp != nil { ... return } + if !isWorkerEligible(pod) { + return + } + poolName := pod.Labels[workerPodLabel]I have this locally with the test above,
go build ./...,go vet ./cmd/ateapi/...and all of./cmd/ateapi/...passing. Happy to send it as a PR if you think the stuck-Terminating window is worth handling, or to close this if the Deleted event is considered sufficient.