Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEMO: service: Add support hard coding iface name to MAC #2299

Closed
wants to merge 1 commit into from

Conversation

cathay4t
Copy link
Member

@cathay4t cathay4t commented Mar 29, 2023

Some OS upgrade might cause interface name changes. To solve that:

  1. Before upgrade,

    • mkdir /etc/nmstate/pin_iface_name/
    • nmstatectl show -k | sudo tee /etc/nmstate/pin_iface_name/pin.yml
    • sudo systemctl enable nmstate
  2. Reboot to new version of OS and let nmstate.service run.

  3. Reboot again.

The nmstate.service will create file like if it found any interface
changed name according to stored state in /etc/nmstate/pin_iface_name/pin.yml

/etc/systemd/network/98-ens9.link

With content:

[Match]
MACAddress=01:23:45:67:89:ab

[Link]
Name=ens9

After the second reboot, the systemd-udevd will use the old name ens9.

You may refer to the screen recording for detail: https://people.redhat.com/fge/demo/nmstate_pin_iface_name.ogv

Comment on lines +117 to +119
let mut fd = std::fs::File::open(&file_path)?;
let mut content = String::new();
fd.read_to_string(&mut content)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You almost always want to use a BufReader when reading from files. This is actually just shorter and faster as e.g.:

    let content = std::fs::read_to_string(&file_path)?;

}

let content =
format!("[Match]\nMACAddress={mac}\n\n[Link]\nName={iface_name}\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment in here like # Generated by nmstate pin-nic-name

Comment on lines +179 to +190
let mut fd = std::fs::OpenOptions::new()
.write(true)
.truncate(true)
.create(true)
.open(&file_path)
.map_err(|e| {
CliError::from(format!(
"Failed to store captured states to file {}: {e}",
file_path.display()
))
})?;
fd.write_all(content.as_bytes())?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is all just:

    std::fs::write(&file_path, content.as_bytes()).map_err(|e| ...)?;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does not override, I guess. Just copy from my code bank.

let mut fd = std::fs::File::open(&file_path)?;
let mut content = String::new();
fd.read_to_string(&mut content)?;
let pin_state: NetworkState = serde_yaml::from_str(&content)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, it's actually a bit more efficient to let serde do the reading from the file directly (not that this matters) like this:

    let mut r = std::fs::File::open(&file_path).map(std::io::BufReader::new);
    let pin_state: NetworkState = serde_yaml::from_reader(&mut r)?;

to avoid allocating an intermediate string.

for pin_iface in pin_state
.interfaces
.iter()
.filter(|i| i.iface_type() == InterfaceType::Ethernet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think about bridge/bonds here right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

virtual interface name never changes by kernel or OS upgrade.

let pin_iface_name_dir = format!("{folder}/{PIN_IFACE_NAME_FOLDER}");
let pin_iface_path = Path::new(&pin_iface_name_dir);
if pin_iface_path.exists() {
pin_iface_name(&pin_iface_path)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the opt-in mechanism here is that the user creates a file? WDYT about my proposal of a CLI command instead in the bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer no issue no change in case there is side impact on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If there is pin_iface_name folder in /etc/nmstate and pin.yml file there, nmstate do the pin work.

Some OS upgrade might cause interface name changes. To solve that:

 1. Before upgrade,
    * mkdir /etc/nmstate/pin_iface_name/
    * nmstatectl show -k | sudo tee /etc/nmstate/pin_iface_name/pin.yml
    * sudo systemctl enable nmstate

 2. Reboot to new version of OS and let nmstate.service run.
 3. Reboot again.

The `nmstate.service` will create file like if it found any interface
changed name according to stored state in /etc/nmstate/pin_iface_name/pin.yml

    /etc/systemd/network/98-ens9.link

With content:

```
[Match]
MACAddress=01:23:45:67:89:ab

[Link]
Name=ens9
```

After the second reboot, the systemd-udevd will use the old name ens9.

Signed-off-by: Gris Ge <fge@redhat.com>
@cgwalters
Copy link
Contributor

cgwalters commented Mar 29, 2023

Having a double reboot here is possible but we'll want to be careful here and e.g. ensure that on the next reboot nmstate-handle-pinning.service is ordered before NetworkManager.service; otherwise we may end up doing DHCP on NICs that were supposed to be static, etc.

I was thinking a much simpler flow here is to forcibly pin active NICs unconditionally. The flow would be:

  • running rhel8
  • run nmstatectl pin-nic-names
  • queue upgrade to rhel9
  • Optional: run rpm-ostree initramfs-etc --track /etc/systemd/network if networking is enabled in the initramfs (I think we can check for ip= on kernel cmdline)
  • reboot

@cgwalters
Copy link
Contributor

I did a PR over in #2300 with some non-functional cleanups - want to pull that into this PR?

@cathay4t
Copy link
Member Author

cathay4t commented Apr 4, 2023

Closing in the favor of #2301

@cathay4t cathay4t closed this Apr 4, 2023
@cathay4t cathay4t deleted the pin_iface_name branch April 4, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants