Repository navigation
fix: compare waiting container reason to PodInitializing - #1396
jim-relation wants to merge 1 commit into
Conversation
`get_status_from_container_state` compared a bare list literal to a
string, so the condition was always False:
if ["reason"] == 'PodInitializing':
The refactor in kubeflow/kubeflow#7585 extracted
`container_state["waiting"]` into `waiting_state` but dropped the
receiver on this one line, leaving a list literal instead of a
subscript. The WAITING branch became unreachable and every waiting
container fell through to the WARNING branch, so notebooks that were
simply starting up rendered as "warning" with "PodInitializing: No
available message for container state." instead of "waiting" with
"PodInitializing".
Compare `waiting_state.get("reason")` instead. `.get` rather than a
subscript because `status.containerState` is a `corev1.ContainerState`
and `ContainerStateWaiting.Reason` is `+optional` with `omitempty`, so
the key can legitimately be absent.
`test_no_message_container_state` asserted the buggy output, so it is
retargeted at a non-initializing reason to keep covering the
missing-message default. Two tests are added: one pinning the
PodInitializing path that regressed, and one for the warning path with
both reason and message, which had no coverage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jim Broadbent <jim.broadbent@relationrx.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
google-oss-prow
Bot
added
area/frontend
|
Hey, thanks a lot for contributing to Kubeflow Notebooks! 🎉 Out of interest, did you create your GitHub account just for this contribution? 😆 /ok-to-test |
Corporate account. Happy to confirm in DM that I'm also an account with some other FOSS contributions |
|
@christian-heusel Can this please be reviewed? Is the policy that you aren't fixing bugs in |
get_status_from_container_statecompares a list literal to a string:["reason"]has no receiver, so Python parses it as a one-element list literal rather than a subscript. The comparison is therefore alwaysFalse, theWAITINGbranch is dead code, and every waiting container falls through toWARNING. A notebook that is just starting up shows a warning readingPodInitializing: No available message for container state.instead of a waiting spinner readingPodInitializing.Introduced in kubeflow/kubeflow#7585, which extracted
container_state["waiting"]intowaiting_state. The other lines in that hunk picked up the new local; this one lost its old receiver without gaining it. flake8 sees valid syntax, so linting never flagged it.Present on
notebooks-v1,v1.11-branchand thev1.11.0tag. The v2 backend has no equivalent code path.Fix
Compare
waiting_state.get("reason")..getrather than["reason"]becausestatus.containerStateis acorev1.ContainerState(notebook-controller/api/v1beta1/notebook_types.go:43) andContainerStateWaiting.Reasonis+optionalwithomitempty, so the key can be absent — the pre-#7585 subscript could raiseKeyErroron a valid Notebook.Tests
test_no_message_container_stateasserted the buggy output, so it was holding the defect in place. It now usesImagePullBackOff, which keeps its original missing-message coverage.Added
test_pod_initializing_container_state, which fails against the unfixed source, andtest_waiting_container_state_with_messagefor the warning path with both fields set, which had no coverage.make unittestpasses (12 tests) and flake8 is clean on both files.Not fixed here
kubelet uses
ContainerCreatingfor this same placeholder state when a pod has no init containers, andPodInitializingwhen it has them — seedefaultWaitingStateinpkg/kubelet/kubelet_pods.go. Notebooks without init containers will still show a warning while starting. That predates #7585, which had the same single-reason check, so I have left it out to keep this change focused. Glad to widen the predicate to both reasons here if you would rather have it in one PR.Discovered by Claude while consuming this status output; patch and analysis also by Claude Code.