Fix printPod panic with spurious container statuses #124906
Conversation
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-priority
There was a problem hiding this comment.
Thanks @liggitt!
/lgtm
Since you've CCed @SergeyKanzhelev I'm holding just to let him a chance to review. Feel entirely free to remove the hold!
/hold
| @@ -1718,6 +1720,32 @@ func TestPrintPodWithRestartableInitContainer(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| // Test pod has container statuses for non-existent initContainers and containers | |||
There was a problem hiding this comment.
For my education: in which scenarios can this ever happen?
There was a problem hiding this comment.
non-kubelet clients that mirror back pod status from other systems (like https://github.com/admiraltyio/admiralty) can incorrectly report back status for injected containers which don't exist in the API :-/
it's not guarded against in API validation, unfortunately (I'll open an issue for that separately)
There was a problem hiding this comment.
Very interesting. Thanks for the explanation!
| if initContainer == nil { | ||
| return false | ||
| } | ||
| if initContainer.RestartPolicy == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
nit
| if initContainer == nil { | |
| return false | |
| } | |
| if initContainer.RestartPolicy == nil { | |
| return false | |
| } | |
| if initContainer == nil || initContainer.RestartPolicy == nil { | |
| return false | |
| } |
There was a problem hiding this comment.
if it's ok, I'd like to leave the nil case standalone since it's a weird exception (I also want to backport this since the nil panic was introduced in 1.28, and I want to make the diff as clean as possible)
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7a4a39229aadd4ea82117dbd077fff701138a0ca
|
k8s-ci-robot
added
release-note
liggitt
added
release-note-none
k8s-ci-robot
added
release-note
|
/retest |
There was a problem hiding this comment.
/lgtm
/approve
Thank you! this is great for backport preserving whatever behavior we have today. I was looking into what else will be printed wrong if such status exists and I think fully ignoring the status like this will be a better fix in 1.31. So I suggest we merge this one and then in 1.31, fully ignore this type of status when printing. WDYT?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
that seems ok opened cherry-picks |
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
|
/retest |
…906-upstream-release-1.30 Automated cherry pick of #124906: Fix printPod panic with spurious container statuses
…906-upstream-release-1.28 Automated cherry pick of #124906: Fix printPod panic with spurious container statuses
…906-upstream-release-1.29 Automated cherry pick of #124906: Fix printPod panic with spurious container statuses
What type of PR is this?
/kind bug
What this PR does / why we need it:
Avoids panic when printing pods with spurious initContainer status
Does this PR introduce a user-facing change?
/sig node
/cc @SergeyKanzhelev