OCPBUGS-86009: Fix dns operator reporting Progressing=True on scale up#477
OCPBUGS-86009: Fix dns operator reporting Progressing=True on scale up#477bentito wants to merge 2 commits into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@bentito: This pull request references Jira Issue OCPBUGS-86009, which is invalid:
Comment 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughA new ChangesDNS Rollout Detection and Conditional Progressing Messages
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (1)
pkg/operator/controller/dns_status_test.go (1)
114-115: ⚡ Quick winAdd a dedicated scale-up (non-rollout) regression case.
All updated fixtures pin
CurrentNumberScheduledto desired, so the new “scale-up but not rolling out” path is still untested. Please add at least one case withdesired > current,updated == current, and matching generation/observed-generation, then assertProgressing=False.Proposed test-direction diff
- makeDaemonSet := func(desired, available, updated int, nodeSelector map[string]string, tolerations []corev1.Toleration) *appsv1.DaemonSet { + makeDaemonSet := func(desired, current, available, updated int, nodeSelector map[string]string, tolerations []corev1.Toleration) *appsv1.DaemonSet { return &appsv1.DaemonSet{ Status: appsv1.DaemonSetStatus{ DesiredNumberScheduled: int32(desired), NumberAvailable: int32(available), - CurrentNumberScheduled: int32(desired), + CurrentNumberScheduled: int32(current), UpdatedNumberScheduled: int32(updated), }, } } + { + name: "scale-up only should not be progressing", + clusterIP: "172.30.0.10", + dnsDaemonset: makeDaemonSet(6, 5, 5, 5, defaultSelector, defaultTolerations), + nrDaemonset: makeDaemonSet(6, 6, 6, 6, defaultSelector, defaultTolerations), + nodeSelector: defaultSelector, + tolerations: defaultTolerations, + expected: operatorv1.ConditionFalse, + },Also applies to: 136-137, 477-478, 645-646
🤖 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/operator/controller/dns_status_test.go` around lines 114 - 115, Add a dedicated regression test case in dns_status_test.go that exercises the scale-up-but-not-rollout path: create a case where CurrentNumberScheduled < desired (desired > current), UpdatedNumberScheduled == CurrentNumberScheduled (no updated replicas), and generation == observedGeneration; place it alongside the existing table entries that use tc.inputs.desireDNS/CurrentNumberScheduled so it’s easy to find, and assert that the computed Progressing condition is False. Ensure the case sets the same generation and observedGeneration values and uses the same fields (CurrentNumberScheduled, UpdatedNumberScheduled, DesiredNumberScheduled or tc.inputs.desireDNS) as other fixtures so it mirrors the other tests but checks Progressing==False.
🤖 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.
Nitpick comments:
In `@pkg/operator/controller/dns_status_test.go`:
- Around line 114-115: Add a dedicated regression test case in
dns_status_test.go that exercises the scale-up-but-not-rollout path: create a
case where CurrentNumberScheduled < desired (desired > current),
UpdatedNumberScheduled == CurrentNumberScheduled (no updated replicas), and
generation == observedGeneration; place it alongside the existing table entries
that use tc.inputs.desireDNS/CurrentNumberScheduled so it’s easy to find, and
assert that the computed Progressing condition is False. Ensure the case sets
the same generation and observedGeneration values and uses the same fields
(CurrentNumberScheduled, UpdatedNumberScheduled, DesiredNumberScheduled or
tc.inputs.desireDNS) as other fixtures so it mirrors the other tests but checks
Progressing==False.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ad8a084-6f0d-4209-8763-7ce725c27dbf
📒 Files selected for processing (2)
pkg/operator/controller/dns_status.gopkg/operator/controller/dns_status_test.go
|
This PR is taking a similar approach as openshift/cluster-ingress-operator#1299 did for same problems on CIO |
The cluster-dns-operator was incorrectly reporting Progressing=True during normal cluster scale up events. This was caused by the computeDNSProgressingCondition checking UpdatedNumberScheduled against DesiredNumberScheduled. When a new node is added, DesiredNumberScheduled increases immediately, but UpdatedNumberScheduled lags until the pod is running, causing the operator to appear to be rolling out. This commit introduces an isDaemonSetRollingOut helper, modeled after similar logic in the cluster-ingress-operator. It checks if the DaemonSet Generation matches ObservedGeneration, and checks if UpdatedNumberScheduled is less than CurrentNumberScheduled. If not, the operator is simply scaling and should not report Progressing=True.
e75b7b4 to
ee254fc
Compare
|
/retest |
1 similar comment
|
/retest |
|
/assign @davidesalerno |
davidesalerno
left a comment
davidesalerno
left a comment
There was a problem hiding this comment.
The changes lgmt evenif I think we still need a dedicated regression test for the exact scenario this PR is fixing: scale-up without rollout.
Right now these fixture updates set CurrentNumberScheduled equal to DesiredNumberScheduled, so the tests don’t exercise the case where:
DesiredNumberScheduled > CurrentNumberScheduledUpdatedNumberScheduled == CurrentNumberScheduledGeneration == ObservedGeneration
In that state, the DaemonSet is scaling up but not rolling out, and computeDNSProgressingCondition should remain Progressing=False.
Could we add a table-driven case that covers that combination explicitly? That would make the regression protection much stronger for this bug.
Add dedicated regression test cases to TestComputeDNSProgressingCondition to verify that the Progressing condition is False when the DaemonSet is scaling up but not rolling out. This covers the scenario where DesiredNumberScheduled > CurrentNumberScheduled but no active rollout is taking place.
Thanks @davidesalerno sorry about delay. I've added unit tests to better lock this in. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bentito: This pull request references Jira Issue OCPBUGS-86009, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@bentito: This pull request references Jira Issue OCPBUGS-86009, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/payload-aggregate periodic-ci-openshift-release-master-nightly-5.1-e2e-aws-ovn-serial 3 |
|
@davidesalerno: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial 3 |
|
@davidesalerno: 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/b37c2500-6fd9-11f1-9166-b6ab77c06fb2-0 |
|
/test e2e-aws-ovn-serial-1of2 |
|
@bentito: 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. |
4 participants
The
cluster-dns-operatorwas incorrectly reportingProgressing=Trueduring normal cluster scale-up events. This was caused bycomputeDNSProgressingConditioncheckingUpdatedNumberScheduledagainstDesiredNumberScheduled. When a new node is added,DesiredNumberScheduledincreases immediately, butUpdatedNumberScheduledlags until the pod is scheduled/running, causing the operator to appear to be rolling out.This PR introduces an
isDaemonSetRollingOuthelper, modeled after similar logic in thecluster-ingress-operator(see PR #1299). It checks if the DaemonSetGenerationmatchesObservedGeneration, and checks ifUpdatedNumberScheduledis less thanCurrentNumberScheduled. If not, the operator is simply scaling and should not reportProgressing=True.Tested locally by ensuring the test suite correctly accommodates the new structure and verifies progression states.
Bug: OCPBUGS-86009
Summary by CodeRabbit