Repository navigation
MGMT-18579: Inject nmpolicy captures into the provided YAML and place it with INI file under the host-specific path - #6695
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
size/L
openshift-ci
Bot
added
size/XL
7446faf to
81c53bb
Compare
openshift-ci
Bot
added
size/L
906f5df to
d7623d4
Compare
openshift-ci
Bot
added
size/XXL
|
/retest |
|
/test edge-e2e-metal-assisted-static-ip-suite |
There was a problem hiding this comment.
What does fetching the binary from the rootfs have to do with the max version?
It seems to me that we would support versions over 4.17 regardless of which approach we use.
The arch is what matters for the rootfs approach or not.
I just worry that we're going to test out a 4.18 release before this gets updated and things won't work as we expect and I don't see a reason to let that happen.
|
/test ? |
|
@linoyaslan: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
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. |
|
/test edge-e2e-metal-assisted-static-ip-suite |
|
/hold |
|
/test edge-e2e-metal-assisted-static-ip-suite |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Looks good. Unhold when you've got all the feedback from others you want.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, 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 |
There was a problem hiding this comment.
It's a little strange to me that we'd maintain two different ways of validating the same input data, based on what transformation we are applying to it before putting it in the ISO.
Validating the input directly involves a lot more .(map[interface{}]interface{}) but ultimately validates the same thing as in the keyfiles that come out of nmstatectl gc. I can't see a reason to maintain two implementations.
There was a problem hiding this comment.
I agree. I'll keep the validation directly on the YAML since we don't want our code handling both YAML and nmconnection formats. The whole point of using nmstate is that we don't need to worry about its internal translation. Right now, we're using the generator for validation, but we're aware that the nmstate team is working on Go structures and adding validation, so that aspect will change in a later phase.
There was a problem hiding this comment.
We also need to ignore the missing name when the interface name appears as an interface in the NMState itself.
There was a problem hiding this comment.
I believe that the following won't work when using nmstate.service in case the MAC map doesn't contain the ports.
---
interfaces:
- name: eth0
type: ethernet
state: up
identifier: mac-address
mac-address: 00:23:45:67:89:1a
- name: eth1
type: ethernet
state: up
identifier: mac-address
mac-address: 00:23:45:67:89:1b
- name: bond0
type: bond
state: up
link-aggregation:
mode: balance-rr
port:
- eth0
- eth1
If it is indeed the case, the bond validation should remain as is.
There was a problem hiding this comment.
@AlonaKaplan As discussed offline, the above example indeed doesn't work with nmstate, but it also fails with a VerificationError when the ports are physical interface names. I've reached out to Gris to check if this is a known issue with nmstate.
There was a problem hiding this comment.
Gris said here that it would work for nmstatectl apply. Is there something different about nmstatectl service that would make it not work there? It seems to me like an issue with nmstate if this doesn't work. Forbidding this config would certainly be a regression for ABI.
There was a problem hiding this comment.
@cathay4t would the above config work using nmstate apply?
There was a problem hiding this comment.
@zaneb I’ve added a temporary condition to prevent regressions for ABI while blocking non-ABI users from using the mac-identifier
There was a problem hiding this comment.
YAML at #6695 (comment) will not works as expected, the bond still refer the bond port by name eth1 and eth2.
You may use interface.controller property for now:
---
interfaces:
- name: bond0
type: bond
link-aggregation:
mode: balance-rr
- name: bond0-port1
type: ethernet
state: up
identifier: mac-address
mac-address: 00:23:45:67:89:1a
controller: bond0Another approach would be wait nmstate/nmstate#2710
There was a problem hiding this comment.
@zaneb as Gris approved, the yaml is not supported by nmstate and not the current/planned API to refer bond ports with mac-identifier.
I understand it works with nmstate generator, but it is kind of a hack.
I think both ABI and AI should be aligned with the official nmstate API and not support this format.
|
@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. |
|
As far as I can see this should be OK for ABI now. |
Inject nmpolicy captures into the provided YAML and place it with INI file under the host-specific path
Script for the new flow using nmstate service, along with the service configurations to execute the script for both minimal and full ISO
Modify the initrd in minimal ISO to include nmpolicy files along with the script and relevant service, while maintaining backward compatibility with the current flow for versions earlier than 4.13
Modify static network flow for full ISO along with maintaining backward compatibility with the current flow for versions earlier than 4.13
|
/test edge-e2e-metal-assisted-bond |
|
/lgtm |
|
/unhold |
|
/retitle MGMT-18579: Inject nmpolicy captures into the provided YAML and place it with INI file under the host-specific path |
|
@linoyaslan: This pull request references MGMT-18579 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 "4.18.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. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-agent-installer-api-server |
10 participants
The purpose of this PR is to change the current method of creating nmconnection files from the nmstate YAML provided by users. Currently, we rely on pre-generated nmconnection files and a complex script running on the node to replace temporary interface names with actual ones. These modifications aim to simplify the process by using nmpolicy and the nmstate service, offering a more straightforward approach to generating nmconnection files. For more detailed information, please refer to the documentation here - Doc
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