Add a hidden persist-nic-names command
#2301
Conversation
|
Hi @cgwalters. Thanks for your PR. I'm waiting for a nmstate member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
|
IMHO, this command is not in scope of the nmstatectl tool since Nmstate is about defining a state and applying it. In the past, there also used to be a persistent name support in - see for example https://github.com/lu-zero/udev/blob/master/src/rule_generator/write_net_rules - which created udev rules files and was hooked into udev. This could also be a good reason to keep calling this "persistent names" instead of "pinning names" btw. |
| [Unit] | ||
| Description=Pin active network interfaces | ||
| Documentation=man:nmstate.service(8) https://www.nmstate.io | ||
| After=NetworkManager.service |
There was a problem hiding this comment.
This might be too early; maybe use network-online.target instead?
I think everyone would agree this isn't a perfect fit, but creating a new tool implies some overhead. |
|
We should debate the overall architecture; I mentioned this here #2299 (comment) One important thing here is all current designs call for running this process once (or at least, by default). We're trying to create something that can assist the admin in creating Also at the current time in order to aid bootstrap problems my plan is to run this from a privileged container, not the host. That's the idea behind having this be runnable from
Yeah, makes sense. So combining these threads we could split this off as a separate binary not branded nmstate; maybe something boring like Also, we could for now make this an opt-in Rust feature. Again the thought here is to fast track this as a one-off for OCP rhel8->rhel9 upgrades and it will likely only need to be supported in that context only at least initially. |
|
(Added an off-by-default feature and split into a separate module) |
cgwalters
force-pushed
the
pin-nic-command2
branch
from
334051f
to
02d2331
Compare
|
OK a few more changes here, most crucially switching to skipping interfaces using DHCP; I'm 87.3% sure that persisting them is unnecessary in the OCP context. I also added a container build of this code pushed at ghcr.io/cgwalters/nmstate:latest. |
|
One point @dustymabe raised with the "only static IP address" filtering is that it will break e.g. bonds over DHCP. |
|
I was also worried about |
|
Also chatted about Tang and |
|
@cgwalters and I were discussing this topic (note I haven't reviewed the code in this PR so this is just comments on the general problem and the approach for a solution). I wonder if we can generalize the solution to this to not worry about static IP/DHCP or not. Can we just look for nm connections that are of ethernet type that are matched based on an interface name. For all that are we add the Here's an example (taken from the FCOS docs): In this case we just loop over the connections.. Here's the flow:
In this flow we don't have to worry about DHCP or static IP and also ignore any device that isn't a physical NIC. Would this work? What are the flaws here (I'm sure there are some)? |
|
Tentatively, I think matching on the connection information makes sense...it looks like to me we could use the |
We try to hide backend detail in the top API. We have |
|
xref openshift/machine-config-operator#3650 (comment) for skipping some OVS types |
|
I provisioned a rhel8 system Also...there seems to be this virtual/emulated USB Ethernet device that we're trying to do DHCP on and not getting a reply. And this shows another bug - we're apparently trying to persist this NIC just because we didn't succeed in DHCP on it. It's using the default/virtual "Wired connection 1". EDIT: Also of interest, Equinix defaults to injecting Equinix and rhel9: They don't have official RHEL9, but AlmaLinux 9. I noticed that the Ethernet device names are the same Equinix: It looks like Equinix: |
We want to document the Rust library. This fixes invoking `cargo doc`. Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
pin-nic-command2
branch
from
ab3b184
to
6ab48ac
Compare
I'd like to change to requiring a special |
cgwalters
force-pushed
the
pin-nic-command2
branch
from
6ab48ac
to
2643ad0
Compare
rust/src/cli/Cargo.toml
Outdated
| query_apply = ["nmstate/query_apply", "ctrlc"] | ||
| gen_conf = ["nmstate/gen_conf"] | ||
| persist_nic = [] |
There was a problem hiding this comment.
I guess now that the CLI option is hidden, we can drop the feature flag?
.github/workflows/container.yml
Outdated
| @@ -0,0 +1,42 @@ | |||
| name: Create and publish a container image | |||
There was a problem hiding this comment.
This is optional, but I found it useful
There was a problem hiding this comment.
We have https://quay.io/organization/nmstate , how about we place it to quay and autobuild on every merge?
|
OK we did some testing and actually in OpenShift OVN setups move the primary NIC into the OVS bridge, and we definitely need to pin the NIC name in this case. |
cgwalters
force-pushed
the
pin-nic-command2
branch
from
2643ad0
to
d3f4882
Compare
|
Changelog:
|
When upgrading rhel8 -> rhel9, some NIC names may change; e.g. the `mlx5` driver started emitting port information. This breaks static IP addressing which references those interfaces. We're going to land code in nmstate to address this; see nmstate/nmstate#2301 - For a few reasons; one is that this isn't an OCP specific problem - nmstate is in Rust - nmstate already parses network interface information - Adding more glue into the MCO is undesirable However...to solve the bootstrap problem here in that we don't have this nmstate code on 4.12, add it to the MCD binary.
xref https://issues.redhat.com//browse/OCPBUGS-10787 In RHEL8 to RHEL9 upgrades, some NIC names may change; we saw this in the wild with `mlx5` based interfaces at least. RHEL Leapp has logic to handle this, but adapting it to work with rpm-ostree/coreos systems would be a heavy lift. This logic is designed to be executed from a privileged container *before* the host upgrades, which fits in well with current OCP logic in the Machine Config Operator. Specifically, openshift/machine-config-operator#3650 will run this command. Co-authored-by: Gris Ge <fge@redhat.com> Signed-off-by: Colin Walters <walters@verbum.org>
|
OK, did both of those. Dealing with conditional compilation and unused variables is a bit annoying. I also dropped the feature and just made it gate on |
cgwalters
force-pushed
the
pin-nic-command2
branch
from
d3f4882
to
45a382e
Compare
When upgrading rhel8 -> rhel9, some NIC names may change; e.g. the `mlx5` driver started emitting port information. This breaks static IP addressing which references those interfaces. We're going to land code in nmstate to address this; see nmstate/nmstate#2301 - For a few reasons; one is that this isn't an OCP specific problem - nmstate is in Rust - nmstate already parses network interface information - Adding more glue into the MCO is undesirable However...to solve the bootstrap problem here in that we don't have this nmstate code on 4.12, add it to the MCD binary.
|
I am intend to merge this PR. Do we need more discussion here? |
Yep, SGTM! |
The goal
Provide a CLI tool that can be executed as part of a privileged container to persist network NIC names. The tool should also support
--dry-run(for ease of debugging) and crucially--inspectthat can be invoked at any time afterwards to help us understand what it did.Also we want to support
--rootfor the case where the host is mounted separately from the container image.The interface
Which NIC names are persisted
It'd be nice to avoid persisting in the default case of DHCP (particularly in cloud) but I'm open to debate.
Dusty suggested below that we only persist NIC names that are mentioned in NM connection files.
Overall design: one-shot persistence
Original PoC code required a "double reboot", doing a diff between the detected NICs pre and post OS upgrade. This is possible but will be somewhat problematic for existing OCP flows.
Test scenarios: Should not persist
Default cloud with DHCP
e.g. AWS, GCP, qemu, etc.
Test scenarios: May or may not persist
Test scenarios: Must persist
Original PR description
This builds on top of #2299 and #2300 and adds a new interface:
nmstatectl pin-nic-namesas well as a corresponding systemd unitnmstatectl-pin-nic-names.servicethat can be enabled.This is aiming to implement #2299 (comment)