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
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.
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. |
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 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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[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.
| ds := &appsv1.DaemonSet{ | ||
| ObjectMeta: metav1.ObjectMeta{Namespace: "one", Name: "alpha", Generation: 1, Labels: sl}, | ||
| Spec: appsv1.DaemonSetSpec{ | ||
| Selector: &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{"app": "alpha"}, | ||
| }, | ||
| }, | ||
| Status: appsv1.DaemonSetStatus{ | ||
| CurrentNumberScheduled: 0, | ||
| DesiredNumberScheduled: 0, | ||
| UpdatedNumberScheduled: 0, | ||
| NumberAvailable: 0, | ||
| NumberReady: 0, | ||
| ObservedGeneration: 1, | ||
| }, | ||
| } | ||
| set(t, client, ds) | ||
| status.SetFromPods() | ||
|
|
||
| _, oc, err := getStatuses(client, "testing") | ||
| if err != nil { | ||
| t.Fatalf("error getting ClusterOperator: %v", err) | ||
| } | ||
| if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{ | ||
| { | ||
| Type: operv1.OperatorStatusTypeDegraded, | ||
| Status: operv1.ConditionFalse, | ||
| }, | ||
| { | ||
| Type: operv1.OperatorStatusTypeProgressing, | ||
| Status: operv1.ConditionFalse, | ||
| }, | ||
| { | ||
| Type: operv1.OperatorStatusTypeUpgradeable, | ||
| Status: operv1.ConditionTrue, | ||
| }, | ||
| { | ||
| Type: operv1.OperatorStatusTypeAvailable, | ||
| Status: operv1.ConditionTrue, | ||
| }, | ||
| }) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the availability assertion independent of RELEASE_VERSION.
The production path requires the DaemonSet release annotation to equal os.Getenv("RELEASE_VERSION"), but this fixture omits that annotation. If the test process inherits a non-empty RELEASE_VERSION, the expected Available=True condition will fail. Set the matching annotation or explicitly control the environment with t.Setenv.
🤖 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 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.
|
@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.