MGMT-20634: Revert mac-identifier and global DNS workarounds as RHEL-91250 and RHEL-72440 resolved#10318
Conversation
|
@linoyaslan: This pull request explicitly references no jira issue. 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
added
the
jira/valid-reference
Walkthrough
ChangesNmstate service decision and helpers
Interface and mocks
Call sites
Tests for backward compatibility logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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
size/M
|
[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)
30-39: ⚡ Quick winAdd a regression test entry for
identifier: mac-address.Since this PR’s core behavior change is dropping the mac-identifier workaround, the table should keep one explicit case proving nmstate.service is still selected on supported versions when
identifier: mac-addressis present and no auto-dns workaround condition exists.Suggested table entry
table.DescribeTable("different scenarios", func(YAML, version string, expectedResult bool) { @@ table.Entry("If the YAML contains a auto-dns: false, dhcp: true, and the version >= MinimalVersionForNmstatectl we shouldn't use the nmstate service flow.", withAutoDnsSetToFalseDhcpTrue, common.MinimalVersionForNmstatectl, false)) + table.Entry("If YAML uses identifier: mac-address and version >= MinimalVersionForNmstatectl, nmstate service flow should be used.", +`interfaces: +- name: eth0 + type: ethernet + state: up + identifier: mac-address + mac-address: "52:54:00:64:c8:96" + ipv4: + enabled: true + dhcp: false + address: + - ip: 192.168.122.10 + prefix-length: 24 +`, common.MinimalVersionForNmstatectl, true))🤖 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 30 - 39, Add a new table.Entry to the DescribeTable that covers the regression for "identifier: mac-address": create a YAML test case (e.g., withIdentifierMacAddress) that contains an interface with identifier: mac-address and no auto-dns workaround, call staticNetworkGenerator.ShouldUseNmstateService via the existing table.DescribeTable harness, use common.MinimalVersionForNmstatectl as the version, and assert the expectedResult is true so the test proves nmstate.service is selected on supported versions; place this new entry alongside the existing entries (same table) and mirror the existing pattern used by withAutoDnsSetToFalseDhcpTrue.
🤖 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 30-39: Add a new table.Entry to the DescribeTable that covers the
regression for "identifier: mac-address": create a YAML test case (e.g.,
withIdentifierMacAddress) that contains an interface with identifier:
mac-address and no auto-dns workaround, call
staticNetworkGenerator.ShouldUseNmstateService via the existing
table.DescribeTable harness, use common.MinimalVersionForNmstatectl as the
version, and assert the expectedResult is true so the test proves
nmstate.service is selected on supported versions; place this new entry
alongside the existing entries (same table) and mirror the existing pattern used
by withAutoDnsSetToFalseDhcpTrue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50a2d021-0c2b-4034-bdeb-c7084801d657
📒 Files selected for processing (2)
pkg/staticnetworkconfig/backward_compatibility.gopkg/staticnetworkconfig/backward_compatibility_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10318 +/- ##
==========================================
- Coverage 44.33% 44.33% -0.01%
==========================================
Files 417 417
Lines 72837 72762 -75
==========================================
- Hits 32294 32259 -35
+ Misses 37609 37586 -23
+ Partials 2934 2917 -17
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
linoyaslan
force-pushed
the
remove-mac-identifier-workaround
branch
from
940b93f to
e56cffd
Compare
linoyaslan
changed the title
|
@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. |
…91250 and RHEL-72440 resolved As RHEL-72440 and RHEL-91250 are now resolved, we can remove our workarounds for mac-identifir and global DNS. With this fix, if a user sets auto-dns: false and dhcp: true or mac-identifier in their nmstate.yaml, the flow with nmstate.service should correctly apply the networking configuration files.
linoyaslan
force-pushed
the
remove-mac-identifier-workaround
branch
from
e56cffd to
8e733c0
Compare
openshift-ci
Bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/staticnetworkconfig/backward_compatibility_test.go (1)
24-40: ⚡ Quick winAvoid hardcoded “below/above minimum” version literals.
Using fixed values (
"4.17","4.19") makes the test brittle ifcommon.MinimalVersionForNmstatectlchanges. Derive below/above test inputs from the minimum constant to keep this stable across future bumps.🤖 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 24 - 40, Replace hardcoded version strings in the tests that call staticNetworkGenerator.ShouldUseNmstateService with versions computed relative to common.MinimalVersionForNmstatectl: derive a "below" version by decrementing the minimal version (e.g., subtracting one minor or patch component) and an "above" version by incrementing it, then use those derived strings in the three It blocks (the calls to ShouldUseNmstateService) so the tests remain correct when common.MinimalVersionForNmstatectl changes.
🤖 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 24-40: Replace hardcoded version strings in the tests that call
staticNetworkGenerator.ShouldUseNmstateService with versions computed relative
to common.MinimalVersionForNmstatectl: derive a "below" version by decrementing
the minimal version (e.g., subtracting one minor or patch component) and an
"above" version by incrementing it, then use those derived strings in the three
It blocks (the calls to ShouldUseNmstateService) so the tests remain correct
when common.MinimalVersionForNmstatectl changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a8e9aed-0fab-4179-a7da-6e909ca74930
📒 Files selected for processing (8)
internal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/ignition/discovery.gointernal/ignition/discovery_test.gopkg/staticnetworkconfig/backward_compatibility.gopkg/staticnetworkconfig/backward_compatibility_test.gopkg/staticnetworkconfig/generator.gopkg/staticnetworkconfig/mock_generator.go
✅ Files skipped from review due to trivial changes (1)
- pkg/staticnetworkconfig/mock_generator.go
|
/lgtm |
|
/override ci/prow/edge-e2e-ai-operator-ztp |
|
@giladravid16: Overrode contexts on behalf of giladravid16: ci/prow/edge-e2e-ai-operator-ztp 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 kubernetes-sigs/prow repository. |
|
@linoyaslan: 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. |
|
/cherry-pick release-ocm-2.17 |
|
@giladravid16: new pull request created: #10336 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 kubernetes-sigs/prow repository. |
4 participants
As RHEL-72440 and RHEL-91250 are now resolved, we can remove our workarounds for mac-identifir and global DNS.
With this fix, if a user sets auto-dns: false and dhcp: true or mac-identifier in their nmstate.yaml, the flow with nmstate.service should correctly apply the networking configuration files.
/cc @giladravid16
How it was tested:
mac-identifier:
I used the following nmstate file:
mac-to-interface:
MAC address: 52:54:00:64:c8:96
interface name: t
Then I verified that network configured as expected during discovery.
global DNS:
I used the following nmstate file:
Then I verified that /etc/resolv.conf was 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