OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions#2128
OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions#2128tchap wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-65896, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@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. |
|
/cc @atiratree |
| // Update is done when all pods have been updated to the latest revision | ||
| // and the deployment controller has reported NewReplicaSetAvailable | ||
| workloadIsBeingUpdated := !workloadAtHighestGeneration || !hasDeploymentProgressed(workload.Status) | ||
| workloadIsBeingUpdatedTooLong := v1helpers.IsUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type) |
There was a problem hiding this comment.
We don't use IsUpdatingTooLong any more, we based the check on ProgressDeadlineExceeded from the deployment controller.
There was a problem hiding this comment.
let's also set .spec.progressDeadlineSeconds to 15m on the consumer workloads to honor the superseded progressingConditionTimeout value
There was a problem hiding this comment.
I don't think we can do that. Sync from the delegate returns the synchronized deployment already. So we would have to send another update request, I guess.
There was a problem hiding this comment.
We can align this in auth when updating library-go. I will actually open a PR there once this is accepted to run CI anyway.
There was a problem hiding this comment.
Yup, I meant that we should update it in the consumer repos like auth.
There was a problem hiding this comment.
Might be better to open a proof PR before the library-go changes are merged.
There was a problem hiding this comment.
| // Update is done when all pods have been updated to the latest revision | ||
| // and the deployment controller has reported NewReplicaSetAvailable | ||
| workloadIsBeingUpdated := !workloadAtHighestGeneration || !hasDeploymentProgressed(workload.Status) | ||
| workloadIsBeingUpdatedTooLong := v1helpers.IsUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type) |
There was a problem hiding this comment.
let's also set .spec.progressDeadlineSeconds to 15m on the consumer workloads to honor the superseded progressingConditionTimeout value
atiratree
left a comment
atiratree
left a comment
There was a problem hiding this comment.
Just one nit, otherwise LGTM, thanks!
| // Update is done when all pods have been updated to the latest revision | ||
| // and the deployment controller has reported NewReplicaSetAvailable | ||
| workloadIsBeingUpdated := !workloadAtHighestGeneration || !hasDeploymentProgressed(workload.Status) | ||
| workloadIsBeingUpdatedTooLong := v1helpers.IsUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type) |
There was a problem hiding this comment.
Yup, I meant that we should update it in the consumer repos like auth.
| // Update is done when all pods have been updated to the latest revision | ||
| // and the deployment controller has reported NewReplicaSetAvailable | ||
| workloadIsBeingUpdated := !workloadAtHighestGeneration || !hasDeploymentProgressed(workload.Status) | ||
| workloadIsBeingUpdatedTooLong := v1helpers.IsUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type) |
There was a problem hiding this comment.
Might be better to open a proof PR before the library-go changes are merged.
tchap
force-pushed
the
workload-condition-overwrites
branch
from
9a05b4b to
0b782a2
Compare
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
tchap
force-pushed
the
workload-condition-overwrites
branch
from
0b782a2 to
08c8c15
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors workload controller condition evaluation: centralizes error-to-message formatting, replaces generation-based Progressing checks with timeout/progress detection, simplifies Degraded detection to pod-level failure reasoning, tightens version-recording gating, and expands tests to cover timeout, multi-pod failures, pod-list errors, and version-recording assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Deployment as DeploymentStatus
participant PodLister
participant VersionRecorder
Controller->>Deployment: read deployment status (generation, observedGeneration, desired/updated/available replicas, conditions)
Controller->>PodLister: list pods for deployment
alt pod list returns error
Controller->>Controller: collect error -> errs
Controller->>Controller: set Degraded = True (SyncError with errMessage)
else pod list OK
Controller->>Controller: hasFailingPods(pods) -> failingPodsCount
alt failingPodsCount > 0
Controller->>Controller: set Degraded = True (N of M instances unavailable...)
else
Controller->>Controller: hasDeploymentTimedOutProgressing(status) -> (msg, timedOut)
alt timedOut
Controller->>Controller: set Progressing = False (ProgressDeadlineExceeded, timed-out message)
Controller->>Controller: set Degraded = True (ProgressDeadlineExceeded)
else
Controller->>Controller: detect PodsUpdating or AsExpected
alt PodsUpdating
Controller->>Controller: set Progressing = True (PodsUpdating)
else AsExpected
Controller->>Controller: set Progressing = False (AsExpected)
Controller->>Controller: set Degraded = False (AsExpected)
end
end
end
end
alt Generation/replica gating satisfied (Generation==ObservedGeneration && updated==available==desired)
Controller->>VersionRecorder: SetVersion(operand, targetVersion)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@tchap: This pull request references Jira Issue OCPBUGS-65896, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/apiserver/controller/workload/workload_test.go (1)
54-67: Please add explicit coverage for the new version-recording gate.
pkg/operator/apiserver/controller/workload/workload.goLines 358-361 changed whenSetVersionfires, but this table still never exercises that path: no scenario setsoperatorConfigAtHighestRevision = true, and the test controller leavesversionRecorderunset. A small fake recorder plus one positive and one negative case would keep this behavior from regressing.Also applies to: 846-878
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/apiserver/controller/workload/workload_test.go` around lines 54 - 67, Add explicit scenarios to TestUpdateOperatorStatus that exercise the version-recording gate by (1) injecting a fake versionRecorder into the test controller and (2) adding two table entries: one with operatorConfigAtHighestRevision = true and one with it = false; verify that when operatorConfigAtHighestRevision is true the fake recorder's SetVersion method is invoked (and not invoked for the false case). Locate the test table in TestUpdateOperatorStatus and the controller instance used to run scenarios, set controller.versionRecorder to a simple fake recorder that records calls, and add assertions in each scenario's validateOperatorStatus to check whether SetVersion was called as expected to cover the SetVersion path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/apiserver/controller/workload/workload_test.go`:
- Around line 54-67: Add explicit scenarios to TestUpdateOperatorStatus that
exercise the version-recording gate by (1) injecting a fake versionRecorder into
the test controller and (2) adding two table entries: one with
operatorConfigAtHighestRevision = true and one with it = false; verify that when
operatorConfigAtHighestRevision is true the fake recorder's SetVersion method is
invoked (and not invoked for the false case). Locate the test table in
TestUpdateOperatorStatus and the controller instance used to run scenarios, set
controller.versionRecorder to a simple fake recorder that records calls, and add
assertions in each scenario's validateOperatorStatus to check whether SetVersion
was called as expected to cover the SetVersion path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15f9a88c-d378-463c-ad2b-87fa88d3a8d7
📒 Files selected for processing (2)
pkg/operator/apiserver/controller/workload/workload.gopkg/operator/apiserver/controller/workload/workload_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/apiserver/controller/workload/workload_test.go (1)
847-889: Add negativeSetVersion()cases for the other readiness gates.
SetVersion()is also gated onGeneration == ObservedGeneration,AvailableReplicas == desiredReplicas, andUpdatedReplicas == desiredReplicasinpkg/operator/apiserver/controller/workload/workload.go:355-364. Right now only theoperatorConfigAtHighestRevisiongate is exercised, so a regression that records the version during an incomplete rollout would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/apiserver/controller/workload/workload_test.go` around lines 847 - 889, Add negative test cases in the workload_test table that exercise the other readiness gates which should prevent SetVersion(): create entries similar to the existing cases but where (a) Generation != ObservedGeneration, (b) Status.AvailableReplicas < Spec.Replicas, and (c) Status.UpdatedReplicas < Spec.Replicas; each should set operatorConfigAtHighestRevision=true (so only the other gate blocks) use the same fakeVersionRecorder and validateVersionRecorder to assert r.setVersionCalls remains empty, and keep validateOperatorStatus as a no-op; locate tests in workload_test.go and mirror the structure/fields used by the existing "version ..." cases so the assertions against setVersionCalls catch regressions in workload.go’s SetVersion gating logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/apiserver/controller/workload/workload_test.go`:
- Around line 847-889: Add negative test cases in the workload_test table that
exercise the other readiness gates which should prevent SetVersion(): create
entries similar to the existing cases but where (a) Generation !=
ObservedGeneration, (b) Status.AvailableReplicas < Spec.Replicas, and (c)
Status.UpdatedReplicas < Spec.Replicas; each should set
operatorConfigAtHighestRevision=true (so only the other gate blocks) use the
same fakeVersionRecorder and validateVersionRecorder to assert r.setVersionCalls
remains empty, and keep validateOperatorStatus as a no-op; locate tests in
workload_test.go and mirror the structure/fields used by the existing "version
..." cases so the assertions against setVersionCalls catch regressions in
workload.go’s SetVersion gating logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f83fb810-ff6d-40fd-9f77-c2365c142fe9
📒 Files selected for processing (1)
pkg/operator/apiserver/controller/workload/workload_test.go
tchap
force-pushed
the
workload-condition-overwrites
branch
from
882ecf8 to
3ae8d51
Compare
|
/hold cancel |
openshift-ci
Bot
added
lgtm
| package workload | ||
|
|
||
| import ( | ||
| "context" |
There was a problem hiding this comment.
Am I right that those are the three (at least) main behavioral changes introduced in this PR ?
1. Scaling the workload (up/down) won't report Progressing=True
2. A stuck rollout eventually stops reporting Progressing=True
3. A workload missing some replicas (2/3) no longer triggers Degraded=True
There was a problem hiding this comment.
Yeah, right. We only degrade on Available == 0.
Also checking the Progressing timeout now depends on the native Deployment ProgressDeadlineExceeded instead of using a helper and once Progressing deadline is hit, we go Degraded.
There was a problem hiding this comment.
Ok, I added another commit that actually makes Degraded work as expected, i.e. Degraded on not all replicas available. This is done by actually checking relevant pods and counting the failing ones so that starting new pods is not included into the Degrading condition.
tchap
force-pushed
the
workload-condition-overwrites
branch
from
08ecc7d to
dee9075
Compare
| } else { | ||
| if hasFailing || workload.Status.AvailableReplicas == 0 { | ||
| var failureDescription string | ||
| if hasFailing { |
There was a problem hiding this comment.
The pods might still exist with AvailableReplicas == 0. Even if they don't, shouldn't PodContainersStatus fn handle that?
|
/cc |
tchap
force-pushed
the
workload-condition-overwrites
branch
from
a216c34 to
8af844e
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-65896, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@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. |
tchap
force-pushed
the
workload-condition-overwrites
branch
from
8af844e to
3159205
Compare
|
I think, this looks like a reasonable to way to monitor progressing and degraded even during scalling operations. The deployment controller does not support this, but we do use the Deployment primitives to achieve this. |
Rework how the workload controller reports Degraded and Progressing conditions to support scaling without false positives, and improve failure detection using pod-level Ready conditions. Key changes: - Stop consulting deployment's observed vs desired generation for Progressing; instead use the Progressing condition's NewReplicaSetAvailable reason, so scaling does not trigger Progressing=True. - Report ProgressDeadlineExceeded via both DeploymentDegraded and DeploymentProgressing (set to False) when the deployment controller signals a timeout. - Replace container-level failure inspection with pod Ready condition checks gated by per-pod ProgressDeadlineSeconds. Filter out terminating pods to avoid false positives from old ReplicaSets. - Detect flapping Ready conditions: when MinReadySeconds > 0 and a pod keeps toggling Ready without staying stable, flag it as failing once the combined progressDeadline + minReadySeconds window has elapsed. - Move SyncError condition into a defer so it is always set regardless of the return path. - Always call PodContainersStatus when reporting UnavailablePod to include container-level diagnostics in the degraded message.
tchap
force-pushed
the
workload-condition-overwrites
branch
from
3159205 to
2f343be
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: atiratree, tchap 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 |
|
@tchap: all tests passed! 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. |
|
Btw @tchap can you also try to run this again? openshift/cluster-authentication-operator#843 |
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
|
Could we split this PR into smaller PRs? It looks like the following changes were included:
This breakdown would help me better understand the proposed changes. Thanks. |
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
…controller Contains the degraded/progressing condition changes made to the workload controller from openshift#2128. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
5 participants
Reworks how the workload controller computes Degraded, Progressing, and Available conditions to fix false positives during scaling and improve failure detection.
Progressing/Degraded no longer consult Deployment generation. Previously, a generation mismatch (e.g. from a scale-up) would immediately set Progressing=True and could trigger Degraded. Now the controller relies solely on the Deployment's own Progressing condition:
NewReplicaSetAvailableand hasn't timed out.ProgressDeadlineExceededis detected from the Deployment's Progressing condition and surfaces as both Progressing=False and DeploymentDegraded=True.DeploymentDegraded now detects failing pods (when not mid-rollout). A pod is considered failing when it is not Ready past its ProgressDeadlineSeconds since creation, or (when MinReadySeconds > 0) its Ready condition is flapping — currently Ready but the last transition is too recent to count as available, checked only after ProgressDeadlineSeconds + MinReadySeconds have elapsed. Terminating pods are excluded to avoid false positives from the old ReplicaSet during rollouts. Diagnostic messages use
deployment.PodContainersStatusto describe container states.WorkloadDegraded SyncError is set via defer to ensure it is applied regardless of the return path.
This affects
authentication-operatorandopenshift-apiserver-operator.Summary by CodeRabbit
Bug Fixes
Tests