pod watch: delete dead pods#1972
Conversation
nicks
left a comment
nicks
left a comment
There was a problem hiding this comment.
i have some concerns that it won't be this simple, but it seems safe to try! lgtm
| ns := k8s.NamespaceFromPod(pod) | ||
| hasSynclet := sidecar.PodSpecContainsSynclet(pod.Spec) | ||
|
|
||
| if pod.DeletionTimestamp != nil && !pod.DeletionTimestamp.IsZero() && ms.PodSet.Pods != nil { |
There was a problem hiding this comment.
i thought we had a separate prunePods function for removing pods? does this logic make more sense there?
| hasSynclet := sidecar.PodSpecContainsSynclet(pod.Spec) | ||
|
|
||
| if pod.DeletionTimestamp != nil && !pod.DeletionTimestamp.IsZero() && ms.PodSet.Pods != nil { | ||
| delete(ms.PodSet.Pods, podID) |
There was a problem hiding this comment.
hmmm...will the short-lived pod log stuff still work if we remove the pod immediately?
There was a problem hiding this comment.
depends on what you mean by "short-lived pod log stuff"
The case covered by the unit test is a Job, where "short-lived" simply means it isn't in state "Running" for very long - it transitions to "Completed" or "Failed", but doesn't actually get deleted - it still shows up in kubectl get pods, and doesn't have a DeletionTimestamp.
IIUC, the case that might stop working is if a pod gets deleted, we have a chance to grab its logs while it's in the "Terminating" state, but getting logs from a "Terminating" pod is always going to be a race condition, because it might be gone by the time we try to get the logs.
maiamcc
left a comment
maiamcc
left a comment
There was a problem hiding this comment.
🙌 amazing! (tbh my solution was just gonna be changing the log level of "error streaming logs" so this is much better 😬 )
3 participants
fixes #1862
Problem
If a manifest loses its pod and doesn't get a new pod to replace it, we never remove that pod from its podset, so every time
PodLogController'sOnChangegets called, it tries to watch the old pod and immediately fails and logs an error, resulting in spam.Our normal mechanism for removing dead pods from a manifest's podset only kicks in when the podset has one more than one pod (which is normally the case because normally a pod gets deleted when a new pod gets created).
Solution
Delete pods from podset as soon as we see they've been deleted.