fix updatePod() of RS and RC controllers #27415
Conversation
k8s-github-robot
added
size/M
bprashanth
added
lgtm
| if rs == nil { | ||
| return | ||
| } | ||
|
|
||
| if curPod.DeletionTimestamp != nil { |
There was a problem hiding this comment.
If deletion timestamp is set on the cur pod and old pod has a different rc (i.e we got a single update with deletion timestamp and label change -- this can happen if both updates hit during a relist), we should invoke deletePod with the old one too. It's safe to do this becase deletion timestamp can never be unset.
There was a problem hiding this comment.
Yeah, this sounds good. I've updated the code.
bprashanth
removed
the
lgtm
| if curPod.DeletionTimestamp != nil { | ||
| // when a pod is deleted gracefully it's deletion timestamp is first modified to reflect a grace period, | ||
| // and after such time has passed, the kubelet actually deletes it from the store. We receive an update | ||
| // for modification of the deletion timestamp and expect an rs to create more replicas asap, not wait | ||
| // until the kubelet actually deletes the pod. This is different from the Phase of a pod changing, because | ||
| // an rs never initiates a phase change, and so is never asleep waiting for the same. | ||
| rsc.deletePod(curPod) | ||
| if !reflect.DeepEqual(curPod.Labels, oldPod.Labels) { |
There was a problem hiding this comment.
Added these lines to address the comment.
There was a problem hiding this comment.
nit: please extract the deepequal into a local var with a name like labelsChanged since we're doing it twice, and add a comment about why it's safe to just invoke deletePod with the oldPod without checking delete timestamp (i.e we can never have a curPod without timestmap and oldPod with).
| oldPod := old.(*api.Pod) | ||
|
|
||
| glog.V(6).Infof("Pod %s updated, objectMeta %+v -> %+v.", curPod.Name, oldPod.ObjectMeta, curPod.ObjectMeta) |
There was a problem hiding this comment.
V(6) is useless IMO, we need V(4) for it to show up in test logs. I think V(4) objectmeta is fine, the really verbose part are container statuses.
…oller to handle pod label updates that match no rc or rs
|
Thanks, LGTM |
bprashanth
added
the
lgtm
|
Both your failures are #27451 but I don't think triggering a rerun ensures that we won't hit it |
goltermann
added
the
priority/critical-urgent
|
+priority/P0 label, this fixes an issue labeled P0 |
|
GCE e2e build/test passed for commit 63fb075. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 63fb075. |
|
Automatic merge from submit-queue |
Fix updatePod of replication controller manager and replica set controller to handle pod label updates that match no RC or RS.
Fix #27405