MGMT-20634: Revert global DNS workaround as RHEL-91250 resolved#10317
MGMT-20634: Revert global DNS workaround as RHEL-91250 resolved#10317linoyaslan wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@linoyaslan: This pull request references MGMT-20634 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
As RHEL-91250 is now resolved, we can remove our workaround for global DNS. With this fix, if a user sets auto-dns: false and dhcp: true in their nmstate.yaml, the flow with nmstate.service should correctly apply the networking configuration files.
linoyaslan
force-pushed
the
remove_auto_dns_workaround
branch
from
d1b51ee to
acce747
Compare
openshift-ci
Bot
added
the
size/M
WalkthroughRemoved global DNS workaround detection logic from static network configuration backward compatibility handling. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: linoyaslan 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
![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/staticnetworkconfig/backward_compatibility_test.go (1)
47-50: ⚡ Quick winAdd an explicit positive regression case for the reverted DNS workaround.
The removed workaround is the main behavior change here, but the table no longer exercises
auto-dns: false+dhcp: trueon a supported version. Keeping one entry that expectstruewould protect this PR from silently drifting back to the old behavior.Suggested test addition
withoutMacIdentifier = `interfaces: - name: eth0 type: ethernet state: up ipv4: enabled: true address: - ip: 192.0.2.1 prefix-length: 24` + withDhcpAndAutoDnsDisabled = `interfaces: +- name: eth0 + type: ethernet + state: up + ipv4: + enabled: true + dhcp: true + auto-dns: false` ) @@ table.Entry("If the YAML contains a mac-identifier, and the version is < MinimalVersionForNmstatectl, we shouldn't use the nmstate service flow", withMacIdentifier, "4.12", false), table.Entry("If the YAML doesn't contain a mac-identifier and the version is >= MinimalVersionForNmstatectl, we should use the nmstate service flow", withoutMacIdentifier, common.MinimalVersionForNmstatectl, true), + table.Entry("If the YAML disables auto-dns while using DHCP and the version is >= MinimalVersionForNmstatectl, we should still use the nmstate service flow", withDhcpAndAutoDnsDisabled, common.MinimalVersionForNmstatectl, true), table.Entry("If the YAML doesn't contain a mac-identifier, and the version < MinimalVersionForNmstatectl we shouldn't use the nmstate service flow.", withoutMacIdentifier, "4.12", false))🤖 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/staticnetworkconfig/backward_compatibility_test.go` around lines 47 - 50, Add an explicit positive regression table entry to cover the reverted DNS workaround: in the table.Entry list in backward_compatibility_test.go add a case that uses common.MinimalVersionForNmstatectl with a test fixture/config that sets auto-dns: false and dhcp: true (reuse the existing fixtures like withMacIdentifier/withoutMacIdentifier as appropriate) and assert the expected boolean is true; place the new table.Entry alongside the other table.Entry calls so the test suite exercises the supported-version + (auto-dns:false, dhcp:true) path and prevents regressing to the old behavior.
🤖 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/staticnetworkconfig/backward_compatibility_test.go`:
- Around line 47-50: Add an explicit positive regression table entry to cover
the reverted DNS workaround: in the table.Entry list in
backward_compatibility_test.go add a case that uses
common.MinimalVersionForNmstatectl with a test fixture/config that sets
auto-dns: false and dhcp: true (reuse the existing fixtures like
withMacIdentifier/withoutMacIdentifier as appropriate) and assert the expected
boolean is true; place the new table.Entry alongside the other table.Entry calls
so the test suite exercises the supported-version + (auto-dns:false, dhcp:true)
path and prevents regressing to the old behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e0c71dd-7a54-49a8-8c41-92a748036e13
📒 Files selected for processing (2)
pkg/staticnetworkconfig/backward_compatibility.gopkg/staticnetworkconfig/backward_compatibility_test.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10317 +/- ##
==========================================
- Coverage 44.33% 44.32% -0.01%
==========================================
Files 417 417
Lines 72837 72795 -42
==========================================
- Hits 32294 32269 -25
+ Misses 37609 37599 -10
+ Partials 2934 2927 -7
🚀 New features to boost your workflow:
|
|
@linoyaslan: 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. |
|
This PR was closed because the changes were combined into another PR |
2 participants
As RHEL-91250 is now resolved, we can remove our workaround for global DNS.
With this fix, if a user sets
auto-dns: falseanddhcp: truein theirnmstate.yaml, the flow withnmstate.serviceshould correctly apply the networking configuration files./cc @giladravid16
How it was tested:
I used the following
nmstatefile:Then I verified that
/etc/resolv.confwas correctly configured both during discovery and after installation.List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Release Notes