OPNET-783: statusmanager: a DaemonSet matching zero nodes is not progressing#3046
OPNET-783: statusmanager: a DaemonSet matching zero nodes is not progressing#3046mkowalski wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-783 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Summary by CodeRabbit
WalkthroughDaemonSet rollout status handling now distinguishes zero desired pods from unscheduled pods. A focused test verifies conditions, rollout tracking, stale-state pruning, and hung-rollout annotation behavior for DaemonSets matching no nodes. ChangesDaemonSet rollout status
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17377 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controller/statusmanager/status_manager_test.go (1)
1621-1728: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the unreconciled zero-node case.
This test only uses
ObservedGeneration == Generation, so it never exercises the new stale-generation branch. Add a case afterstatus.installCompleteis true withGeneration > ObservedGenerationandDesiredNumberScheduled == 0, assertingProgressing=Trueuntil the observed generation catches up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/statusmanager/status_manager_test.go` around lines 1621 - 1728, Extend TestStatusManagerDaemonSetMatchingZeroNodes with an unreconciled zero-node scenario after installation is complete: update the DaemonSet to have Generation greater than ObservedGeneration while DesiredNumberScheduled remains zero, reconcile, and assert Progressing=True. Then set ObservedGeneration equal to Generation, reconcile again, and assert Progressing=False, covering the stale-generation branch without treating the DaemonSet as a rollout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/statusmanager/pod_status.go`:
- Around line 115-119: Ensure stale-generation DaemonSets remain in rollout
processing when scheduled counts are zero: update the dsRolloutActive logic used
by the NumberAvailable == 0 branch to include ds.Status.ObservedGeneration <
ds.Generation, or bypass the gate for that condition. Preserve the existing
handling for DaemonSets with no desired pods while allowing newly updated
resources to progress until their controller observes the generation.
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 1633-1673: Make the DaemonSet fixture in the relevant
status-manager test deterministic with respect to RELEASE_VERSION: either add
the release annotation expected by the production availability logic using
os.Getenv("RELEASE_VERSION"), or use t.Setenv to explicitly control the
environment value before calling status.SetFromPods. Update the
fixture/assertion around status.SetFromPods and getStatuses so Available=True
remains valid regardless of the inherited environment.
---
Nitpick comments:
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 1621-1728: Extend TestStatusManagerDaemonSetMatchingZeroNodes with
an unreconciled zero-node scenario after installation is complete: update the
DaemonSet to have Generation greater than ObservedGeneration while
DesiredNumberScheduled remains zero, reconcile, and assert Progressing=True.
Then set ObservedGeneration equal to Generation, reconcile again, and assert
Progressing=False, covering the stale-generation branch without treating the
DaemonSet as a rollout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d8f5a23a-0904-4292-a372-6d0a49668439
📒 Files selected for processing (2)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager_test.go
| } else if ds.Status.NumberAvailable == 0 && (ds.Status.DesiredNumberScheduled > 0 || ds.Status.ObservedGeneration < ds.Generation) && dsRolloutActive { | ||
| // A DaemonSet whose observed status desires zero pods (e.g. its | ||
| // node selector matches no nodes) is fully rolled out. Only treat | ||
| // NumberAvailable == 0 as "unscheduled" if pods are desired or the | ||
| // DaemonSet controller has not yet observed the current generation. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep stale-generation DaemonSets progressing after installation.
When status.installComplete is true and the scheduled counts are both zero, dsRolloutActive is false. Thus, even with ObservedGeneration < Generation, the trailing gate prevents this branch and can report a newly updated zero-node DaemonSet as complete before its controller observes the generation. Include generation drift in dsRolloutActive or bypass this gate for stale generations.
Suggested fix
- dsRolloutActive := !status.installComplete || ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled
+ dsRolloutActive := !status.installComplete ||
+ ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled ||
+ ds.Status.ObservedGeneration < ds.Generation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if ds.Status.NumberAvailable == 0 && (ds.Status.DesiredNumberScheduled > 0 || ds.Status.ObservedGeneration < ds.Generation) && dsRolloutActive { | |
| // A DaemonSet whose observed status desires zero pods (e.g. its | |
| // node selector matches no nodes) is fully rolled out. Only treat | |
| // NumberAvailable == 0 as "unscheduled" if pods are desired or the | |
| // DaemonSet controller has not yet observed the current generation. | |
| dsRolloutActive := !status.installComplete || | |
| ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled || | |
| ds.Status.ObservedGeneration < ds.Generation |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/statusmanager/pod_status.go` around lines 115 - 119, Ensure
stale-generation DaemonSets remain in rollout processing when scheduled counts
are zero: update the dsRolloutActive logic used by the NumberAvailable == 0
branch to include ds.Status.ObservedGeneration < ds.Generation, or bypass the
gate for that condition. Preserve the existing handling for DaemonSets with no
desired pods while allowing newly updated resources to progress until their
controller observes the generation.
A DaemonSet whose node selector or affinity currently matches no nodes (desiredNumberScheduled == 0, observed generation current) is fully rolled out. The status manager treated it as "not yet scheduled on any nodes" and eventually as a hung rollout, degrading the operator. Guard the empty case while still treating an unreconciled DaemonSet (observedGeneration < generation) as progressing so freshly created DaemonSets do not flap Available.
bf90e2d to
d07c67a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controller/statusmanager/pod_status.go (1)
115-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale-generation zero-node DaemonSets can still be falsely marked rolled out.
The added
(DesiredNumberScheduled > 0 || ObservedGeneration < Generation)check is still gated by&& dsRolloutActiveat the end.dsRolloutActive(line 97) is!status.installComplete || UpdatedNumberScheduled < CurrentNumberScheduled. OnceinstallCompleteistrueand the DaemonSet is steady-state at zero scheduled pods,dsRolloutActiveisfalse, so the whole branch evaluates tofalseregardless ofObservedGeneration < Generation. If the DaemonSet spec is later updated (Generation bumps) while it still matches zero nodes, the controller hasn't observed the new generation yet, but this code will still treat it as fully rolled out — the exact bug flagged in the prior review, which the current OR-inside-the-branch approach does not actually fix because it's still ANDed withdsRolloutActive.🐛 Proposed fix: decouple the generation check from dsRolloutActive
- } else if ds.Status.NumberAvailable == 0 && (ds.Status.DesiredNumberScheduled > 0 || ds.Status.ObservedGeneration < ds.Generation) && dsRolloutActive { + } else if ds.Status.NumberAvailable == 0 && + (ds.Status.ObservedGeneration < ds.Generation || + (ds.Status.DesiredNumberScheduled > 0 && dsRolloutActive)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/statusmanager/pod_status.go` around lines 115 - 119, Update the DaemonSet rollout decision around dsRolloutActive so ObservedGeneration < Generation independently prevents a zero-available, zero-node DaemonSet from being considered fully rolled out. Do not leave the generation check gated solely by dsRolloutActive; preserve the existing DesiredNumberScheduled and active-rollout behavior for other cases.
🧹 Nitpick comments (1)
pkg/controller/statusmanager/status_manager_test.go (1)
1708-1753: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a post-installComplete generation bump on a zero-node DaemonSet.
This scenario doesn't cover the case where
status.installCompleteis alreadytrueand the DaemonSet'sGenerationadvances again (spec update) while it still matches zero nodes andObservedGenerationlags behind. That is exactly the case wherepod_status.go'sdsRolloutActivegate can mask a stale-generation state (see comment onpod_status.golines 115-119). Adding a case here would catch regressions once that logic is fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/statusmanager/status_manager_test.go` around lines 1708 - 1753, Extend the status-manager test around the existing zero-node DaemonSet setup to cover installComplete already being true, then advance the DaemonSet Generation while leaving ObservedGeneration behind and matching zero nodes. Call status.SetFromPods and assert the resulting rollout state/conditions reflect the stale generation rather than being hidden by dsRolloutActive, using the existing test helpers and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/controller/statusmanager/pod_status.go`:
- Around line 115-119: Update the DaemonSet rollout decision around
dsRolloutActive so ObservedGeneration < Generation independently prevents a
zero-available, zero-node DaemonSet from being considered fully rolled out. Do
not leave the generation check gated solely by dsRolloutActive; preserve the
existing DesiredNumberScheduled and active-rollout behavior for other cases.
---
Nitpick comments:
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 1708-1753: Extend the status-manager test around the existing
zero-node DaemonSet setup to cover installComplete already being true, then
advance the DaemonSet Generation while leaving ObservedGeneration behind and
matching zero nodes. Call status.SetFromPods and assert the resulting rollout
state/conditions reflect the stale generation rather than being hidden by
dsRolloutActive, using the existing test helpers and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7ec24614-ae86-4565-82b8-ed5d4df408a7
📒 Files selected for processing (2)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager_test.go
|
@mkowalski: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
What
A DaemonSet whose node selector or affinity currently matches no nodes (
status.desiredNumberScheduled == 0with the generation observed) is fully rolled out — but the status manager reported it asDaemonSet ... is not yet scheduled on any nodes(Progressing) and, after the timeout, as a hung rollout (Degraded,rollout is not making progress), blocking co/network.The guard keeps an unreconciled DaemonSet (
observedGeneration < generation, i.e. all-zero status right after creation) counted as progressing, so freshly created DaemonSets do not flap Available during install.Why now
Hit by BGP-based VIP management (openshift/enhancements#1982): on compact clusters the frr-k8s DaemonSet legitimately schedules zero pods (control plane nodes run the frr-k8s static pod instead and are excluded by node affinity), which degraded co/network and stalled installation. The bug is feature-independent though — any optional DaemonSet whose affinity matches no nodes triggers it.
Testing
New
TestStatusManagerDaemonSetMatchingZeroNodes(TDD — reproduces the exact live symptom before the fix): desired=0/observed-current ⇒ Available, not Progressing, not Degraded, no rollout state tracked; includes a stale-rollout-state + clock-step case proving no hung-rollout Degraded fires and state is pruned. Validated live in the enhancement-1982 reference implementation (https://github.com/mkowalski/bgp-vip-demo).Part of OPNET-595; CNO subtask: OPNET-783.