OCPBUGS-85677, OCPBUGS-90541: Don't set Progressing=False until all pods available#3034
OCPBUGS-85677, OCPBUGS-90541: Don't set Progressing=False until all pods available#3034mdbooth wants to merge 2 commits into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@mdbooth: This pull request references Jira Issue OCPBUGS-90541, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe status manager now records stable generations in an annotation, uses that state to drive rollout messaging for DaemonSets, StatefulSets, and Deployments, and updates tests and fixtures to preserve and assert the new behavior. ChangesObserved-stable rollout tracking
Sequence Diagram(s)sequenceDiagram
participant SetFromPods
participant WorkloadStatus
participant ObjectAnnotation
SetFromPods->>WorkloadStatus: evaluate available/unavailable status
WorkloadStatus->>ObjectAnnotation: read or set observed-stable-generation
ObjectAnnotation-->>SetFromPods: stable state for progression handling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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@v0.8.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 17357 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
🤖 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 172-188: The condition on line 172 requires ReadyReplicas > 0 to
treat a StatefulSet as having an active rollout, but when hadState is true
(indicating tracked state from a previous rollout) and ReadyReplicas is 0 while
UpdatedReplicas equals Replicas, the StatefulSet falls out of the
rollout-in-progress path prematurely, clearing tracked state before pods
recover. Modify the condition to also handle the zero-ready availability case by
checking if we have tracked state or an active rollout regardless of whether
ReadyReplicas is greater than zero, ensuring that pending pods waiting to become
ready are properly tracked as progressing rather than being reported as
non-progressing.
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 2388-2390: The test file hardcodes the release version annotation
as "v1.0.0" while the rollout logic reads the actual RELEASE_VERSION environment
variable. This mismatch causes test assertions to fail when the environment
variable differs from the hardcoded value. Replace all hardcoded "v1.0.0" values
in the Annotations map with the "release.openshift.io/version" key at all
locations (lines 2388-2390, 2562-2564, 2653-2655, 2766-2768, and 2882-2884) by
retrieving the actual RELEASE_VERSION environment variable value using os.Getenv
and using that value instead. This ensures the test annotations match what the
rollout logic actually reads from the environment.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d9ebdf1-4112-4d54-a0f5-168c3667049c
📒 Files selected for processing (2)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager_test.go
|
/retest-required |
|
Pods are crashlooping due to: |
We don't want the ClusterOperator to report Progressing=True whenever a Node reboots. However, we also don't want to report Progressing=False during a CNI rollout until all pods are available. This change ensures both are covered.
mdbooth
force-pushed
the
fix-progressing-false-timing
branch
from
d343fbc to
6812a1e
Compare
|
/assign @danwinship @jluhrsen |
according to claude this PR is also fixing OCPBUGS-85677 which I was chasing with this #3018. Should I close 3018 and dup OCPBUGS-85677 to OCPBUGS-90541 ? |
|
/retest |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/66eaf760-7052-11f1-8ba4-dea2a7eefeae-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cfe9ea30-70ae-11f1-8306-a6764354848f-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 /retest |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c728f6b0-717c-11f1-8aed-1c98979481b0-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4d13ebd0-71b0-11f1-8ec6-39f7ceeeb643-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 /retest |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e1ee8920-725c-11f1-90a8-66ef2ab22b57-0 |
@jluhrsen This was green. |
yes. we have 50 results now from the aggregate jobs and now of them showed the network operator as Progressing=True when it should not have. I marked OCPBUGS-85677 as a duplicate of OCPBUGS-90541 and closed my original PR in favor of this one /lgtm /verified by aggregate jobs |
openshift-ci-robot
added
the
verified
|
@jluhrsen: This PR has been marked as verified by 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jluhrsen, mdbooth 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 |
|
/retest-required |
|
/retitle OCPBUGS-85677, OCPBUGS-90541: Don't set Progressing=False until all pods available /retest |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@mdbooth: This pull request references Jira Issue OCPBUGS-85677, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-90541, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jluhrsen: This pull request references Jira Issue OCPBUGS-85677, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-90541, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. 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. |
|
The direct cause of the ipsec failures is https://redhat.atlassian.net/browse/OCPBUGS-89238 However, I've noticed an additional problem here. Other jobs are currently passing the install step in CI even though the ipsec DS never becomes stable. This is because of a related bug in the rollout code: How the old code was always broken:The original rollout renders the MCO is updated to add ipsec. Nodes reboot, etc, etc. CNO observes the new MachineConfig and re-renders the HOWEVER, because the ds had previously rendered successfully, it is still annotated with installComplete=true. Also dsRolloutActive= false because: dsRolloutActive := !status.installComplete || ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumber
ScheduledThe pods are all scheduled: they're just crash looping. So when we enter the How my new code usually works, but has a race:My new patch uses However, it has a race. If we're unlucky and don't observe that state, e.g. because CNO happen to be running on the control plane node which was rebooted first, we will never reset the state on the ds which enables us to know we're rolling out again. A better fixI'm going to push a new proposed fix which adds a new annotation: |
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
@mdbooth: This pull request references Jira Issue OCPBUGS-85677, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-90541, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/statusmanager/status_manager_test.go (1)
106-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotation merge is a no-op and drops
obj's own annotations.
merged := current.GetAnnotations()followed bymaps.Copy(merged, current.GetAnnotations())copies the server map onto itself — a no-op.obj.SetAnnotations(merged)then replacesobj's annotations with only the server's, discarding any annotation the test set onobj. Today this is masked because annotated fixtures hit theCreatebranch, but the update path silently loses test-set annotations. The intended merge was almost certainly overlayingobj's annotations onto the server's.♻️ Proposed fix
- // Avoid wiping annotations added by SetFromPods. - merged := current.GetAnnotations() - if merged == nil { - merged = map[string]string{} - } - maps.Copy(merged, current.GetAnnotations()) - obj.SetAnnotations(merged) + // Avoid wiping annotations added by SetFromPods: start from the + // server's annotations, then overlay any set on obj by the test. + merged := current.GetAnnotations() + if merged == nil { + merged = map[string]string{} + } + maps.Copy(merged, obj.GetAnnotations()) + obj.SetAnnotations(merged)🤖 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 106 - 112, The annotation merge in the status manager test is using the server annotations as both source and destination, so it becomes a no-op and drops any annotations already set on obj. Update the merge in the status manager update path to start from current.GetAnnotations() and then copy obj.GetAnnotations() into that map before calling obj.SetAnnotations; this keeps the server annotations while preserving test-set annotations. Use the existing obj, current.GetAnnotations(), maps.Copy, and obj.SetAnnotations flow in status_manager_test.go to locate the change.
🤖 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/status_manager_test.go`:
- Around line 1876-1899: Update the assertions in status_manager_test.go so the
failure messages reflect the actual table-driven case being tested. In the
Progressing condition check around conditionsInclude, replace the hardcoded
DaemonSet-specific message with one that uses the current test case context from
tt so it works for DaemonSet, Deployment, StatefulSet, and both true/false
expectations. In the observed stable generation assertion, keep comparing
against tt.object.GetGeneration() but change the failure text to print that same
expected value instead of tt.observedStableGeneration so the message matches the
assertion.
---
Nitpick comments:
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 106-112: The annotation merge in the status manager test is using
the server annotations as both source and destination, so it becomes a no-op and
drops any annotations already set on obj. Update the merge in the status manager
update path to start from current.GetAnnotations() and then copy
obj.GetAnnotations() into that map before calling obj.SetAnnotations; this keeps
the server annotations while preserving test-set annotations. Use the existing
obj, current.GetAnnotations(), maps.Copy, and obj.SetAnnotations flow in
status_manager_test.go to locate the change.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23c5ddf5-6160-4e80-993c-a5cb32e43e70
📒 Files selected for processing (3)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager_test.gopkg/names/names.go
✅ Files skipped from review due to trivial changes (1)
- pkg/names/names.go
|
/test e2e-aws-ovn-hypershift-conformance hypershift-e2e-aks |
|
/retest-required |
Adds a networkoperator.openshift.io/observed-stable-generation annotation to CNO operands when they are observed stable. Unlike the copy of the object's status temporarily stored in a ClusterOperator annotation during a rollout, this is persistent state. With this change, an operand is rolling out if: * Its observed generation does not match its generation * It is not currently available and it has never been observed stable at this generation That is, a rollout completes once we have observed a stable rollout at a particular generation at least once. Therefore: * We only go Progressing=False once all pods are available * We do not subsequently go Progressing=False unless the operand's generation changes, i.e. a for a spec change
mdbooth
force-pushed
the
fix-progressing-false-timing
branch
from
9f93b53 to
4f149a4
Compare
|
@mdbooth: 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. |
5 participants
We don't want the ClusterOperator to report Progressing=True whenever a Node reboots. However, we also don't want to report Progressing=False during a CNI rollout until all pods are available. This change ensures both are covered.
Related to openshift/origin#31320
Adds a
networkoperator.openshift.io/observed-stable-generationannotation to CNO operands when they are observed stable. Unlike the copy of the object's status temporarily stored in a ClusterOperator annotation during a rollout, this is persistent state.With this change, an operand is rolling out if:
That is, a rollout completes once we have observed a stable rollout at a particular generation at least once. Therefore:
This is v2 of this PR, but I have left the original commit in place for comparison and in case we want to roll back to it. The reasons for v2 are explained in #3034 (comment)
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests