OCPBUGS-85677, OCPBUGS-90541: Don't set Progressing=False until all pods available - #3095
OCPBUGS-85677, OCPBUGS-90541: Don't set Progressing=False until all pods available#3095jluhrsen wants to merge 2 commits into
Conversation
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.
openshift-ci-robot
added
jira/valid-reference
|
@jluhrsen: This pull request references Jira Issue OCPBUGS-85677, which is valid. 3 validation(s) were run on this bug
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. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Summary by CodeRabbit
WalkthroughRollout tracking now uses an observed stable generation annotation for DaemonSets, StatefulSets, and Deployments. Stable objects receive the annotation, progression checks use it as the gating signal, timestamps use the injected clock, and tests cover updated rollout and restart behavior. ChangesStable generation rollout tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SetFromPods
participant WorkloadStatus
participant ObjectAnnotations
SetFromPods->>WorkloadStatus: read generation and availability status
WorkloadStatus-->>SetFromPods: report stable or progressing state
SetFromPods->>ObjectAnnotations: write observed-stable-generation
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 /pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
@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/954d4ec0-8b67-11f1-9f2c-80a779d0e75b-0 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jluhrsen 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: 1
🧹 Nitpick comments (1)
pkg/controller/statusmanager/pod_status.go (1)
73-283: 📐 Maintainability & Code Quality | 🔵 TrivialVerify e2e coverage for this user-facing Progressing behavior change.
SetFromPodsnow changes when the ClusterOperator'sProgressingcondition flips toFalseduring node reboots/CNI rollouts (the stated goal of OCPBUGS-85677/90541). Only unit tests were included in this review; notest/e2e/changes were provided.Can you confirm whether corresponding
test/e2e/coverage and a PR "Testing"/"How to verify it" section (CI lanes, platform coverage, results) exist for this change?As per path instructions, "For new or modified Go code under
pkg/that changes user-facing behavior or fixes a bug, add or modify corresponding files undertest/e2e/, and include a PRTestingorHow to verify itsection describing CI lanes, platform coverage, and test results."🤖 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 73 - 283, Add or update test/e2e coverage for the user-facing Progressing condition behavior changed in SetFromPods, specifically during node reboots and CNI rollouts, covering the OCPBUGS-85677/90541 scenarios. Ensure the pull request includes a Testing or How to verify it section documenting CI lanes, platform coverage, and test results.Source: Path instructions
🤖 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 89-120: Fix the annotation merge in set() so it preserves both
annotations already persisted on current and new annotations on obj. Start with
a separate copy of current.GetAnnotations(), then copy obj.GetAnnotations() into
it before calling obj.SetAnnotations(merged); retain the existing
resource-version update and create/update behavior.
---
Nitpick comments:
In `@pkg/controller/statusmanager/pod_status.go`:
- Around line 73-283: Add or update test/e2e coverage for the user-facing
Progressing condition behavior changed in SetFromPods, specifically during node
reboots and CNI rollouts, covering the OCPBUGS-85677/90541 scenarios. Ensure the
pull request includes a Testing or How to verify it section documenting CI
lanes, platform coverage, and test results.
🪄 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: 5b1343dd-4349-429c-b49e-01a0b46739b5
📒 Files selected for processing (3)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager_test.gopkg/names/names.go
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
|
/test 5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade 👻🚫 retest requested by |
|
/test 5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade 👻🚫 auto-retest triggered for |
|
/test e2e-aws-ovn-fdp-qe 👻🚫 auto-retest triggered for |
|
/test e2e-aws-ovn-hypershift-conformance 👻🚫 auto-retest triggered for |
|
/test e2e-aws-ovn-upgrade 👻🚫 auto-retest triggered for |
|
/test e2e-aws-ovn-upgrade-ipsec 👻🚫 auto-retest triggered for |
|
/test e2e-gcp-ovn 👻🚫 auto-retest triggered for |
|
/test e2e-gcp-ovn-techpreview 👻🚫 auto-retest triggered for |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw 👻🚫 auto-retest triggered for |
|
/test e2e-metal-ipi-ovn-ipv6-ipsec 👻🚫 auto-retest triggered for |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 👻🚫 retest requested by |
|
/test e2e-ovn-ipsec-step-registry 👻🚫 retest requested by |
|
/test hypershift-e2e-aks 👻🚫 retest requested by |
|
@openshift-pr-manager[bot]: 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/c59c8530-8ba5-11f1-8d0d-1cc3603e6024-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-ipsec 10 |
61d6f6c to
256039d
Compare
|
@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/f70d9da0-8ba7-11f1-8261-77f45802b631-0 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/statusmanager/status_manager_test.go (1)
2562-2563: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBoth restart tests still justify
Progressing=Falsevia the old "UpdatedReplicas >= Replicas" rule. The gate is now observed-stable-generation alignment — which is precisely whyobserved-stable-generation: "1"was added to both fixtures; the comments will mislead the next reader debugging these expectations.
pkg/controller/statusmanager/status_manager_test.go#L2562-L2563: reword to state thatbetawas already observed stable at generation 1, so post-rollout unavailability is treated as reboot churn.pkg/controller/statusmanager/status_manager_test.go#L2636-L2637: same rewording for the StatefulSet's unready replica.🤖 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 2562 - 2563, The comments in both restart test fixtures still describe the obsolete UpdatedReplicas >= Replicas completion rule. Reword the comments at pkg/controller/statusmanager/status_manager_test.go lines 2562-2563 and 2636-2637 to state that beta was already observed stable at generation 1, so post-rollout unavailability is treated as reboot churn; update comments only, leaving the test behavior unchanged.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/controller/statusmanager/status_manager_test.go (2)
120-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setStatusclobbers caller-set annotations instead of merging.Unlike
set()(which now merges correctly),setStatusoverwritesobj's annotations with the server copy, mutating the caller's object. No current test depends on it, but it's the same latent trapset()just fixed, and it silently drops locally-added annotations. Also, the status subresource update doesn't need annotations at all.♻️ Mirror set()'s merge semantics
obj.SetResourceVersion(current.GetResourceVersion()) - obj.SetAnnotations(current.GetAnnotations()) + merged := make(map[string]string) + maps.Copy(merged, current.GetAnnotations()) + 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 120 - 134, Update setStatus to preserve caller-set annotations by merging the current server annotations with obj’s existing annotations, matching set()’s merge semantics, rather than replacing them. Since the status subresource update does not require annotations, avoid mutating obj’s annotations when refreshing the resource version before Status().Update.
1846-1857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCases with
observedStableGeneration == 0write"...observed-stable-generation": "0"rather than leaving it absent.The "New " cases are meant to exercise the never-annotated path, but the helper unconditionally stamps the annotation.
isObservedStablestill returns false for"0"vs generation 1, so expectations hold, yet the genuinely un-annotated path — the one the one-time migration inSetFromPodskeys off (annotations[...] == "") — is never covered here.♻️ Only set the annotation when the case specifies one
- annotations := tt.object.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string, 1) - } - annotations[names.ObservedStableGenerationAnnotation] = strconv.FormatInt(tt.observedStableGeneration, 10) - tt.object.SetAnnotations(annotations) + if tt.observedStableGeneration != 0 { + annotations := tt.object.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string, 1) + } + annotations[names.ObservedStableGenerationAnnotation] = strconv.FormatInt(tt.observedStableGeneration, 10) + tt.object.SetAnnotations(annotations) + }🤖 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 1846 - 1857, Update the test setup around observedStableGeneration so it only adds ObservedStableGenerationAnnotation when the case specifies a nonzero value; leave annotations absent for zero-valued “New <workload>” cases. Preserve the existing annotation formatting and object setup for explicitly configured generations.
🤖 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`:
- Line 170: The timeout checks in the DaemonSet, StatefulSet, and Deployment
status paths must use the injected clock consistently. In
pkg/controller/statusmanager/pod_status.go at lines 170-170, 234-234, and
297-297, update each time.Since-based comparison to calculate elapsed time from
status.clock.Now(), matching the LastChangeTime assignments and preserving the
existing hung-rollout thresholds.
- Around line 105-106: Replace the status-manager migration failure logging
around setAnnotation in status.Update with the appropriate klog call, preserving
the existing message, namespace/name context, and error value. Also update the
other log.Printf usages in pod_status.go to klog so all status-manager failures
follow the production logging policy.
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 1872-1873: Remove the stale “both DSes” comment above the
table-driven assertion, or replace it with a concise description of the
single-object parameterized expectation covered by each case. Keep the assertion
and test behavior unchanged.
In `@pkg/controller/statusmanager/status_manager.go`:
- Around line 114-117: Persist or recover annotationMigrationDone in
status_manager.go so migration completion survives CNO restarts and is not rerun
when installComplete is restored; in pod_status.go, update the DaemonSet,
Deployment, and StatefulSet stable checks to require each resource’s full
availability/readiness predicates in addition to ObservedGeneration >=
Generation, preventing unavailable active rollouts from being classified as
stable. Affected sites: pkg/controller/statusmanager/status_manager.go lines
114-117; pkg/controller/statusmanager/pod_status.go lines 102-103, 111-112, and
120-121.
---
Outside diff comments:
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 2562-2563: The comments in both restart test fixtures still
describe the obsolete UpdatedReplicas >= Replicas completion rule. Reword the
comments at pkg/controller/statusmanager/status_manager_test.go lines 2562-2563
and 2636-2637 to state that beta was already observed stable at generation 1, so
post-rollout unavailability is treated as reboot churn; update comments only,
leaving the test behavior unchanged.
---
Nitpick comments:
In `@pkg/controller/statusmanager/status_manager_test.go`:
- Around line 120-134: Update setStatus to preserve caller-set annotations by
merging the current server annotations with obj’s existing annotations, matching
set()’s merge semantics, rather than replacing them. Since the status
subresource update does not require annotations, avoid mutating obj’s
annotations when refreshing the resource version before Status().Update.
- Around line 1846-1857: Update the test setup around observedStableGeneration
so it only adds ObservedStableGenerationAnnotation when the case specifies a
nonzero value; leave annotations absent for zero-valued “New <workload>” cases.
Preserve the existing annotation formatting and object setup for explicitly
configured generations.
🪄 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: ffac25e0-051e-45a9-9587-7e84c83d5d84
📒 Files selected for processing (4)
pkg/controller/statusmanager/pod_status.gopkg/controller/statusmanager/status_manager.gopkg/controller/statusmanager/status_manager_test.gopkg/names/names.go
| if err := status.setAnnotation(context.TODO(), ds, names.ObservedStableGenerationAnnotation, &gen); err != nil { | ||
| log.Printf("Error migrating DaemonSet %s/%s annotation: %v", ds.Namespace, ds.Name, err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/controller/statusmanager/pod_status.go"
echo "== file size =="
wc -l "$file"
echo
echo "== imports and relevant symbols =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines 80-140 =="
sed -n '80,140p' "$file" | cat -n
echo
echo "== search for log.Printf / klog in file =="
rg -n 'log\.Printf|klog\.' "$file" || trueRepository: openshift/cluster-network-operator
Length of output: 6059
Use klog instead of log.Printf here. This file still uses log.Printf in the migration path and elsewhere; switch these status-manager failures to klog to match the production logging policy.
🤖 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 105 - 106, Replace
the status-manager migration failure logging around setAnnotation in
status.Update with the appropriate klog call, preserving the existing message,
namespace/name context, and error value. Also update the other log.Printf usages
in pod_status.go to klog so all status-manager failures follow the production
logging policy.
Source: Coding guidelines
|
|
||
| if !hadState || !reflect.DeepEqual(dsState.LastSeenStatus, ds.Status) { | ||
| dsState.LastChangeTime = time.Now() | ||
| dsState.LastChangeTime = status.clock.Now() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/controller/statusmanager/pod_status.go"
echo "== file outline / line count =="
wc -l "$file"
echo "== search for time.Since and clock usage =="
rg -n "time\.Since|status\.clock|LastChangeTime" "$file"
echo "== relevant excerpts =="
sed -n '150,190p;220,250p;280,315p' "$file"Repository: openshift/cluster-network-operator
Length of output: 5281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,185p;228,245p;290,308p' pkg/controller/statusmanager/pod_status.goRepository: openshift/cluster-network-operator
Length of output: 2280
Use the injected clock for the timeout checks. The DaemonSet, StatefulSet, and Deployment paths set LastChangeTime from status.clock, but time.Since still uses wall time, so fake or non-wall clocks can make hung-rollout detection fire too early or never. Compare against status.clock.Now() in each path.
📍 Affects 1 file
pkg/controller/statusmanager/pod_status.go#L170-L170(this comment)pkg/controller/statusmanager/pod_status.go#L234-L234pkg/controller/statusmanager/pod_status.go#L297-L297
🤖 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` at line 170, The timeout checks
in the DaemonSet, StatefulSet, and Deployment status paths must use the injected
clock consistently. In pkg/controller/statusmanager/pod_status.go at lines
170-170, 234-234, and 297-297, update each time.Since-based comparison to
calculate elapsed time from status.clock.Now(), matching the LastChangeTime
assignments and preserving the existing hung-rollout thresholds.
| // Both DSes have unavailable pods and neither has been observed stable | ||
| // at its current generation, so Progressing must be True. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale copy-pasted comment. This table-driven assertion covers a single object per case (DS/Deployment/StatefulSet); there are no "both DSes". Drop or replace with a description of the parameterized expectation.
As per coding guidelines: "Flag obvious comment slop... Fail unless comments add genuine value."
🤖 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 1872 -
1873, Remove the stale “both DSes” comment above the table-driven assertion, or
replace it with a concise description of the single-object parameterized
expectation covered by each case. Keep the assertion and test behavior
unchanged.
Source: Coding guidelines
| // annotationMigrationDone is set after the first SetFromPods call where | ||
| // installComplete is true. It prevents the one-time migration from running | ||
| // again on subsequent calls. | ||
| annotationMigrationDone bool |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Persist and tighten the migration boundary.
annotationMigrationDone resets on every CNO restart, but installComplete is restored. The migration therefore reclassifies post-install workloads; ObservedGeneration >= Generation does not establish availability, so an unavailable active rollout can be annotated as stable and stop reporting Progressing.
pkg/controller/statusmanager/status_manager.go#L114-L117: persist or recover migration completion across restarts.pkg/controller/statusmanager/pod_status.go#L102-L103: require the DaemonSet’s full stable predicates, not only observed generation.pkg/controller/statusmanager/pod_status.go#L111-L112: require the Deployment’s full stable predicates, not only observed generation.pkg/controller/statusmanager/pod_status.go#L120-L121: require the StatefulSet’s full stable predicates, not only observed generation.
📍 Affects 2 files
pkg/controller/statusmanager/status_manager.go#L114-L117(this comment)pkg/controller/statusmanager/pod_status.go#L102-L103pkg/controller/statusmanager/pod_status.go#L111-L112pkg/controller/statusmanager/pod_status.go#L120-L121
🤖 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.go` around lines 114 - 117,
Persist or recover annotationMigrationDone in status_manager.go so migration
completion survives CNO restarts and is not rerun when installComplete is
restored; in pod_status.go, update the DaemonSet, Deployment, and StatefulSet
stable checks to require each resource’s full availability/readiness predicates
in addition to ObservedGeneration >= Generation, preventing unavailable active
rollouts from being classified as stable. Affected sites:
pkg/controller/statusmanager/status_manager.go lines 114-117;
pkg/controller/statusmanager/pod_status.go lines 102-103, 111-112, and 120-121.
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
@jluhrsen: 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. |
3 participants
No description provided.