OCPBUGS-78085: Fix dual-stack service update by preserving IP families - #467
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
Walkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
/assign |
baf4270 to
60f82c6
Compare
|
I amended the last commit with an update to TestDNSServiceChanged to include a dedicated unit test for this regression. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/controller_dns_service_test.go`:
- Around line 145-166: The test case uses mutateOriginal/mutate/verify but only
asserts lengths of updated.Spec.IPFamilies and ClusterIPs, which can miss
loss/reordering of the primary ClusterIP or family order; update the fixture in
mutateOriginal to set a realistic dual-stack Service including
Service.Spec.ClusterIP (primary IPv4 e.g. "1.2.3.4") and ClusterIPs in the
proper order (["1.2.3.4", "fd00::1"]) and in verify assert equality of
updated.Spec.ClusterIP, updated.Spec.ClusterIPs, and updated.Spec.IPFamilies
(and IPFamilyPolicy) against the original Service values rather than just
checking slice lengths so the preserved dual-stack state is validated
field-for-field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 189a0b21-d750-4e52-aca4-8620ff530fd7
📒 Files selected for processing (2)
pkg/operator/controller/controller_dns_service.gopkg/operator/controller/controller_dns_service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/controller/controller_dns_service.go
bentito
changed the title
openshift-ci-robot
added
jira/severity-important
|
@bentito: This pull request references Jira Issue OCPBUGS-78085, 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@bentito: This pull request references Jira Issue OCPBUGS-78085, 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/jira refresh |
|
@lihongan: This pull request references Jira Issue OCPBUGS-78085, 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. |
|
/retest-required |
bentito
changed the title
openshift-ci-robot
removed
the
jira/severity-important
|
@bentito: This pull request references Jira Issue OCPBUGS-78053, 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: 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. |
| updated.Spec.ClusterIPs = current.Spec.ClusterIPs | ||
| updated.Spec.IPFamilies = current.Spec.IPFamilies | ||
| updated.Spec.IPFamilyPolicy = current.Spec.IPFamilyPolicy |
There was a problem hiding this comment.
You should also update serviceCmpOpts not to ignore these fields.
There was a problem hiding this comment.
@Miciah I'm not sure if I understand. We want to preserve these fields, so if they change, we shouldn't need to reconcile/change anything right? I think the current implementation is correct.
There was a problem hiding this comment.
I think Miciah might be suggesting we should strictly enforce the dual-stack state in the desired Service. If that's the case, I'm concerned that removing those fields from serviceCmpOpts would cause a reconciliation loop unless we also update desiredDNSService to dynamically resolve the full ClusterIPs list and IPFamilyPolicy from the cluster network config. Does that sound like what you had in mind, @Miciah?
There was a problem hiding this comment.
Ah, I think you are right—we want the operator to ignore clusterIP, clusterIPs, ipFamilies, and ipFamilyPolicy and let the API server set them. It is only trafficDistribution where the operator has a preference.
|
Please make sure to include details and a Jira link in the commit message. |
|
@bentito: This pull request references Jira Issue OCPBUGS-78053, 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. |
cba2295 to
04c726d
Compare
Done, squashed to one commit with (mostly) the contents of the PR description. |
|
The test is still failing in the payload jobs |
|
I see failures like this (before and after this PR): I don't doubt that Brett found a bug, but looking at the sippy failures, I think the issue causing the test failures is it that the |
|
/assign @rikatz |
|
/retest-required |
|
/test e2e-hypershift |
1 similar comment
|
/test e2e-hypershift |
|
@bentito Based on our finding that this isn't causing the CI failure, I re-opened https://issues.redhat.com/browse/OCPBUGS-78085, mind updating your commit message link and the title of this PR? |
The failure in the `DNS should answer A and AAAA queries for a dual-stack service` test on metal jobs was triggered by the recent merge of PR openshift#457 (`NE-2414: Use trafficDistribution: PreferSameNode for openshift-dns Service`). ### The Root Cause In `pkg/operator/controller/controller_dns_service.go`, the operator compares the `current` DNS Service against the `expected` (static) asset using a function called `serviceChanged()`. To prevent unnecessary updates, the comparison correctly ignores dynamic dual-stack fields. However, when PR openshift#457 added `trafficDistribution: PreferSameNode` to the static asset, it caused `expected` and `current` to differ on existing clusters, legitimately triggering a Service update. When an update is triggered, the operator blindly replaces the spec (`updated.Spec = expected.Spec`) and then tries to manually preserve dynamic fields that the API server or other controllers manage. It only preserved `ClusterIP`, dropping `ClusterIPs`, `IPFamilies`, and `IPFamilyPolicy`. ### The Impact Because `expected.Spec` (from the static YAML) has no IP families defined, the operator pushes a Service update that wipes out the secondary IPv6 Cluster IP. The `openshift-dns` service is abruptly converted back to a single-stack IPv4 service, which permanently breaks the `DNS should answer A and AAAA queries for a dual-stack service` test on metal (dual-stack) jobs. ### The Fix This explicitly preserves the dual-stack fields (`ClusterIPs`, `IPFamilies`, `IPFamilyPolicy`) during an update. It also improves the dual-stack test assertions to verify these fields are preserved field-for-field. Jira: https://issues.redhat.com/browse/OCPBUGS-78085
04c726d to
6178e04
Compare
thanks! yep, done now |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/controller_dns_service_test.go`:
- Around line 154-158: The test's mutate closure clears
IPFamilies/IPFamilyPolicy/ClusterIPs but the expected fixture still retains
ClusterIP from the original, so update the test to also clear
expected.Spec.ClusterIP (and expected.Spec.ClusterIPs if present) so the
assertion truly validates preservation behavior; locate the mutate func and the
test's expected variable in controller_dns_service_test.go and set
expected.Spec.ClusterIP = "" (and clear expected.Spec.ClusterIPs) to match the
mutated service.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 385b1096-909c-4aac-b71f-174c9b8f3a2d
📒 Files selected for processing (2)
pkg/operator/controller/controller_dns_service.gopkg/operator/controller/controller_dns_service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/controller/controller_dns_service.go
| mutate: func(service *corev1.Service) { | ||
| service.Spec.IPFamilies = nil | ||
| service.Spec.IPFamilyPolicy = nil | ||
| service.Spec.ClusterIPs = nil | ||
| service.Spec.Selector = map[string]string{"foo": "bar"} |
There was a problem hiding this comment.
Clear ClusterIP in the expected fixture too.
This case says the dynamic dual-stack fields are absent from expected, but Line 154-158 still leaves service.Spec.ClusterIP copied from original. That makes the ClusterIP assertion pass even if serviceChanged stops preserving that field, so the regression test is not fully load-bearing for ClusterIP.
💡 Proposed test tightening
mutate: func(service *corev1.Service) {
+ service.Spec.ClusterIP = ""
service.Spec.IPFamilies = nil
service.Spec.IPFamilyPolicy = nil
service.Spec.ClusterIPs = nil
service.Spec.Selector = map[string]string{"foo": "bar"}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mutate: func(service *corev1.Service) { | |
| service.Spec.IPFamilies = nil | |
| service.Spec.IPFamilyPolicy = nil | |
| service.Spec.ClusterIPs = nil | |
| service.Spec.Selector = map[string]string{"foo": "bar"} | |
| mutate: func(service *corev1.Service) { | |
| service.Spec.ClusterIP = "" | |
| service.Spec.IPFamilies = nil | |
| service.Spec.IPFamilyPolicy = nil | |
| service.Spec.ClusterIPs = nil | |
| service.Spec.Selector = map[string]string{"foo": "bar"} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/controller_dns_service_test.go` around lines 154 -
158, The test's mutate closure clears IPFamilies/IPFamilyPolicy/ClusterIPs but
the expected fixture still retains ClusterIP from the original, so update the
test to also clear expected.Spec.ClusterIP (and expected.Spec.ClusterIPs if
present) so the assertion truly validates preservation behavior; locate the
mutate func and the test's expected variable in controller_dns_service_test.go
and set expected.Spec.ClusterIP = "" (and clear expected.Spec.ClusterIPs) to
match the mutated service.
There was a problem hiding this comment.
@bentito I think code rabbit has a point here too - adding a validation for the existing ClusterIP behavior is probably worth it.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/test e2e-hypershift |
bentito
changed the title
openshift-ci-robot
added
the
jira/severity-important
|
@bentito: This pull request references Jira Issue OCPBUGS-78085, 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. |
|
/test e2e-hypershift |
|
/lgtm thanks @bentito |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/verified by @lihongan tested on dualstack cluster and looks good |
openshift-ci-robot
added
the
verified
|
@lihongan: 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. |
|
@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. |
|
@bentito: Jira Issue Verification Checks: Jira Issue OCPBUGS-78085 Jira Issue OCPBUGS-78085 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-23-022245 |
7 participants
The failure in the
DNS should answer A and AAAA queries for a dual-stack servicetest on metal jobs was triggered by the recent merge of PR #457 (NE-2414: Use trafficDistribution: PreferSameNode for openshift-dns Service) in commite2b909a5.The Root Cause
In
pkg/operator/controller/controller_dns_service.go, the operator compares thecurrentDNS Service against theexpected(static) asset using a function calledserviceChanged().To prevent unnecessary updates, the comparison correctly ignores dynamic dual-stack fields:
However, when PR #457 added
trafficDistribution: PreferSameNodeto the static asset, it causedexpectedandcurrentto differ on existing clusters, legitimately triggering a Service update.When an update is triggered, the operator blindly replaces the spec (
updated.Spec = expected.Spec) and then tries to manually preserve dynamic fields that the API server or other controllers manage. It only preservedClusterIP, droppingClusterIPs,IPFamilies, andIPFamilyPolicy:The Impact
Because
expected.Spec(from the static YAML) has no IP families defined, the operator pushes a Service update that wipes out the secondary IPv6 Cluster IP. Theopenshift-dnsservice is abruptly converted back to a single-stack IPv4 service, which permanently breaks theDNS should answer A and AAAA queries for a dual-stack servicetest on metal (dual-stack) jobs.The Fix
This explicitly preserves the dual-stack fields during an update:
Summary by CodeRabbit
Bug Fixes
Tests