OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions#2128
OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions#2128tchap wants to merge 2 commits 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
To enable scaling not to automatically set Progressing/Degraded, conditions handling is aligned so that the Deployment generation is not consulted any more. Degraded only happens when the Deployment is not Available or it times out progressing. Progressing is set to True when the Deployment is not in NewReplicaSetAvailable and it hasn't timed out progressing.
tchap
force-pushed
the
workload-condition-overwrites
branch
from
0b782a2 to
08c8c15
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReworked workload controller condition evaluation: replaced generation-based Progressing logic with timeout-based checks via a new helper, simplified Degraded logic to availability/deadline checks, removed an external dependency, and tightened version-recording gating. Tests updated to reflect new condition outcomes and version-recording behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Deployment as DeploymentStatus
participant VersionRecorder
Controller->>Deployment: Read deployment status (AvailableReplicas, UpdatedReplicas, Progressing, ObservedGeneration)
alt Progressing not NewReplicaSetAvailable and not timed out
Controller->>Controller: set Progressing = True (PodsUpdating)
else ProgressDeadlineExceeded
Controller->>Controller: set Progressing = False (ProgressDeadlineExceeded)
Controller->>Controller: set Degraded = True (ProgressDeadlineExceeded)
else AsExpected
Controller->>Controller: set Progressing = False (AsExpected)
Controller->>Controller: set Degraded = False (AsExpected)
end
alt AvailableReplicas == 0
Controller->>Controller: set Degraded = True (Unavailable)
end
alt operator config at highest generation AND workload.Generation == workload.Status.ObservedGeneration AND AvailableReplicas == desiredReplicas AND UpdatedReplicas == desiredReplicas
Controller->>VersionRecorder: SetVersion(operandName, version)
else
note right of Controller: skip version recording
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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
|
@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. |
|
/hold cancel |
openshift-ci
bot
added
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 |
| @@ -4,7 +4,6 @@ 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.
4 participants
To enable scaling not to automatically set Progressing/Degraded,
conditions handling is aligned so that the Deployment generation is not
consulted any more.
Degraded only happens when the deployment is not Available or it times
out progressing. This is less strict than before, but checking available
replicas would mean we go Degraded on scaling automatically.
Progressing is set to True when the deployment is not in
NewReplicaSetAvailable and it hasn't timed out progressing.
I also improved the overall test case names in a separate commit.
This affects
authentication-operatorandopenshift-apiserver-operator.Summary by CodeRabbit
Bug Fixes
Tests