Fix a regression introduced lately: When any given PodInfraContainer on ... #5466
Conversation
|
Needs rebase btw :) |
| if err != nil { | ||
| glog.Errorf("Error listing containers %#v", containersInPod) | ||
| return err | ||
| } | ||
| containersInPod = containers.FindContainersByPod(uid, podFullName) |
There was a problem hiding this comment.
Wow can't believe I missed that...good catch.
|
LGTM, just needs a rebase |
|
Just verified using the same unittest generated here, earlier refactory of SyncPod introduced this regression, then later one just fixed it. :-) and :-( Looks like we only need unittests to avoid future breakage. I will double check it against a real cluster and update the PR. |
|
haha sorry for the churn, the latest refactoring makes things more concrete and easier to follow. |
…on a node is killed, kubelet kills all remaining containers no matter which pod that container belongs to. Fixed kubernetes#5373
|
Ok, run tests against the read cluster, the regression is fixed. Updated cl with unnitests only. |
|
LGTM, thanks @dchen1107! Will merge on green. |
Fix a regression introduced lately: When any given PodInfraContainer on ...
|
I dont get it - this is a no-op change? |
|
@thockin The initial PR I sent out does contain a proper fix and unittest reproducing the issue. The regression was introduced by a refactory lately which passes all running k8s containers after killing PodInfraContainers and deciding to kill rest in a given Pod. But at the same day, another refactory code was introduced which fix the initial issue. The only unittest is required then. |
...a node
is killed, kubelet kills all remaining containers no matter which pod that
container belongs to.
Fixed #5373
Also updated unittest to reproduce the issue without the fix.