From cbf59f2e6013c18a54a65a7acd883c5b62f8ff8c Mon Sep 17 00:00:00 2001 From: Gris Ge Date: Wed, 7 Jun 2023 17:12:46 +0800 Subject: [PATCH] Fix regression on SRIOV timeout Resolves: RHBZ#2212380 Signed-off-by: Gris Ge --- ...-with-auto-IP-address-on-STP-enabled.patch | 67 - BZ_2182769-skip_if_ifnetifnames_0.patch | 62 - BZ_2212380_fix_sriov_timeout.patch | 1300 +++++++++++++++++ nmstate.spec | 6 +- 4 files changed, 1305 insertions(+), 130 deletions(-) delete mode 100644 0002-Fix-error-when-DHCP-with-auto-IP-address-on-STP-enabled.patch delete mode 100644 BZ_2182769-skip_if_ifnetifnames_0.patch create mode 100644 BZ_2212380_fix_sriov_timeout.patch diff --git a/0002-Fix-error-when-DHCP-with-auto-IP-address-on-STP-enabled.patch b/0002-Fix-error-when-DHCP-with-auto-IP-address-on-STP-enabled.patch deleted file mode 100644 index 9424326..0000000 --- a/0002-Fix-error-when-DHCP-with-auto-IP-address-on-STP-enabled.patch +++ /dev/null @@ -1,67 +0,0 @@ -From 333e82445c048812e3e85fb9f3cb7558dc3f2aeb Mon Sep 17 00:00:00 2001 -From: Gris Ge -Date: Tue, 25 Apr 2023 14:10:34 +0800 -Subject: [PATCH] ip: Fix error when DHCP with auto IP address on STP enabled - bridge - -When DHCP enabled with auto IP address on STP enabled bridge, nmstate -will fail with verification error: - - Verification failure: br0.interface.ipv4.address desire '[]', - current 'null' - -The root cause is STP suspended the DHCP action which cause current -state shows null IP address. And nmstate incorrectly treat [] != null -for IP address. - -Fixed in `sanitize_current_for_verify()` to set empty array if None. - -To reproduce this problem, we just enable DHCP with auto IP address -where no DHCP server exists. Integration test case included. - -Signed-off-by: Gris Ge ---- - rust/src/lib/query_apply/ip.rs | 13 ++++++++++--- - 1 file changed, 10 insertions(+), 3 deletions(-) - -diff --git a/rust/src/lib/query_apply/ip.rs b/rust/src/lib/query_apply/ip.rs -index b2d6ac49..a6df740b 100644 ---- a/rust/src/lib/query_apply/ip.rs -+++ b/rust/src/lib/query_apply/ip.rs -@@ -12,6 +12,11 @@ impl InterfaceIpv4 { - if self.dhcp_custom_hostname.is_none() { - self.dhcp_custom_hostname = Some(String::new()); - } -+ -+ // No IP address means empty. -+ if self.enabled && self.addresses.is_none() { -+ self.addresses = Some(Vec::new()); -+ } - } - - // Sort addresses and dedup -@@ -89,6 +94,11 @@ impl InterfaceIpv6 { - if self.dhcp_custom_hostname.is_none() { - self.dhcp_custom_hostname = Some(String::new()); - } -+ -+ // No IP address means empty. -+ if self.enabled && self.addresses.is_none() { -+ self.addresses = Some(Vec::new()); -+ } - } - - // Sort addresses and dedup -@@ -96,9 +106,6 @@ impl InterfaceIpv6 { - if let Some(addrs) = self.addresses.as_mut() { - addrs.sort_unstable(); - addrs.dedup(); -- if addrs.is_empty() { -- self.addresses = None; -- } - } - } - pub(crate) fn update(&mut self, other: &Self) { --- -2.40.0 - diff --git a/BZ_2182769-skip_if_ifnetifnames_0.patch b/BZ_2182769-skip_if_ifnetifnames_0.patch deleted file mode 100644 index 8d1f549..0000000 --- a/BZ_2182769-skip_if_ifnetifnames_0.patch +++ /dev/null @@ -1,62 +0,0 @@ -From d8b4c29ab17a53bad6f189acf76bab78e29ee333 Mon Sep 17 00:00:00 2001 -From: Gris Ge -Date: Sun, 23 Apr 2023 16:01:29 +0800 -Subject: [PATCH] cli: Do nothing for `persist-nic-names` when got - `net.ifnames=0` - -When `net.ifnames=0` is defined in kernel argument, systemd will disable -the predicable network interface name which make no sense for nmstate to -pin the interface names. Hence we do nothing in `persist-nic-names` in -that case. - -Manually tested in CentOS stream 9 VM. - -Signed-off-by: Gris Ge ---- - rust/src/cli/persist_nic.rs | 19 +++++++++++++++++++ - 1 file changed, 19 insertions(+) - -diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs -index 6b126d58..61b07dbb 100644 ---- a/rust/src/cli/persist_nic.rs -+++ b/rust/src/cli/persist_nic.rs -@@ -7,6 +7,8 @@ - //! - //! The logic currently is: - //! -+//! - Do nothing if kernel argument contains `net.ifnames=0` which disabled the -+//! predicable network interface name, hence not fit our use case here. - //! - Iterate over all active NICs - //! - Pin every ethernet interface to its MAC address (prefer permanent MAC - //! address) -@@ -70,6 +72,14 @@ pub(crate) fn run_persist_immediately( - PersistAction::CleanUpDryRun => return clean_up(root, true), - }; - -+ if is_prediable_ifname_disabled() { -+ log::info!( -+ "Systemd predicable network interface name is disabled \ -+ by kernel argument `net.ifnames=0`, will do nothing" -+ ); -+ return Ok("".to_string()); -+ } -+ - let stamp_path = Path::new(root) - .join(SYSTEMD_NETWORK_LINK_FOLDER) - .join(NMSTATE_PERSIST_STAMP); -@@ -317,3 +327,12 @@ fn is_nmstate_generated_systemd_link_file(file_path: &PathBuf) -> bool { - .map(|_| buff == PERSIST_GENERATED_BY.as_bytes()) - .unwrap_or_default() - } -+ -+const KERNEL_CMDLINE_FILE: &str = "/proc/cmdline"; -+ -+fn is_prediable_ifname_disabled() -> bool { -+ std::fs::read(KERNEL_CMDLINE_FILE) -+ .map(|v| String::from_utf8(v).unwrap_or_default()) -+ .map(|c| c.contains("net.ifnames=0")) -+ .unwrap_or_default() -+} --- -2.40.0 - diff --git a/BZ_2212380_fix_sriov_timeout.patch b/BZ_2212380_fix_sriov_timeout.patch new file mode 100644 index 0000000..e4a6eb8 --- /dev/null +++ b/BZ_2212380_fix_sriov_timeout.patch @@ -0,0 +1,1300 @@ +From bbd091cc566743dc9e218389efadf4654e3ff65d Mon Sep 17 00:00:00 2001 +From: Gris Ge +Date: Tue, 6 Jun 2023 18:18:52 +0800 +Subject: [PATCH 01/13] sriov: Fix regression on waiting SRIOV + +The 20871bac introduced a regression causing SR-IOV verification error +when enabling large mount(60+) VF. + +The root cause is we only retry verification +`VERIFY_RETRY_COUNT_SRIOV(60)` times when VF changed with missing ethernet +VF. Fixed by use `VERIFY_RETRY_COUNT_SRIOV` when we have VF count +changed. + +In test with ixgbe, we noticed 60 seconds is not enough, hence increase +to 300 seconds for verification retry. + +Integration test case included and manually tested on ixgbe card. + +Signed-off-by: Gris Ge +--- + rust/src/lib/query_apply/ethernet.rs | 2 +- + rust/src/lib/query_apply/net_state.rs | 23 ++++++++++++++--------- + 2 files changed, 15 insertions(+), 10 deletions(-) + +diff --git a/rust/src/lib/query_apply/ethernet.rs b/rust/src/lib/query_apply/ethernet.rs +index a5281061..d1fc5c24 100644 +--- a/rust/src/lib/query_apply/ethernet.rs ++++ b/rust/src/lib/query_apply/ethernet.rs +@@ -116,7 +116,7 @@ impl NetworkState { + self.has_vf_count_change(current) && self.has_missing_eth(current) + } + +- fn has_vf_count_change(&self, current: &Self) -> bool { ++ pub(crate) fn has_vf_count_change(&self, current: &Self) -> bool { + for iface in + self.interfaces.kernel_ifaces.values().filter(|i| i.is_up()) + { +diff --git a/rust/src/lib/query_apply/net_state.rs b/rust/src/lib/query_apply/net_state.rs +index 7afbdb63..bef8f3ff 100644 +--- a/rust/src/lib/query_apply/net_state.rs ++++ b/rust/src/lib/query_apply/net_state.rs +@@ -13,7 +13,7 @@ use crate::{ + const DEFAULT_ROLLBACK_TIMEOUT: u32 = 60; + const VERIFY_RETRY_INTERVAL_MILLISECONDS: u64 = 1000; + const VERIFY_RETRY_COUNT: usize = 5; +-const VERIFY_RETRY_COUNT_SRIOV: usize = 60; ++const VERIFY_RETRY_COUNT_SRIOV: usize = 300; + const VERIFY_RETRY_COUNT_KERNEL_MODE: usize = 5; + const RETRY_NM_COUNT: usize = 2; + const RETRY_NM_INTERVAL_MILLISECONDS: u64 = 2000; +@@ -123,7 +123,16 @@ impl NetworkState { + None + }; + +- let timeout = self.timeout.unwrap_or(DEFAULT_ROLLBACK_TIMEOUT); ++ let mut timeout = self.timeout.unwrap_or(DEFAULT_ROLLBACK_TIMEOUT); ++ let verify_count = if self.has_vf_count_change(&cur_net_state) { ++ timeout = VERIFY_RETRY_COUNT_SRIOV as u32 ++ * VERIFY_RETRY_INTERVAL_MILLISECONDS as u32 ++ / 1000; ++ VERIFY_RETRY_COUNT_SRIOV ++ } else { ++ VERIFY_RETRY_COUNT ++ }; ++ + let checkpoint = match nm_checkpoint_create(timeout) { + Ok(c) => c, + Err(e) => { +@@ -141,12 +150,6 @@ impl NetworkState { + + log::info!("Created checkpoint {}", &checkpoint); + +- let verify_count = if pf_state.is_some() { +- VERIFY_RETRY_COUNT_SRIOV +- } else { +- VERIFY_RETRY_COUNT +- }; +- + with_nm_checkpoint(&checkpoint, self.no_commit, || { + if let Some(pf_state) = pf_state { + let pf_merged_state = MergedNetworkState::new( +@@ -160,6 +163,7 @@ impl NetworkState { + &cur_net_state, + &checkpoint, + verify_count, ++ timeout, + )?; + // Refresh current state + cur_net_state.retrieve()?; +@@ -178,6 +182,7 @@ impl NetworkState { + &cur_net_state, + &checkpoint, + verify_count, ++ timeout, + ) + }) + } +@@ -188,8 +193,8 @@ impl NetworkState { + cur_net_state: &Self, + checkpoint: &str, + retry_count: usize, ++ timeout: u32, + ) -> Result<(), NmstateError> { +- let timeout = self.timeout.unwrap_or(DEFAULT_ROLLBACK_TIMEOUT); + // NM might have unknown race problem found by verify stage, + // we try to apply the state again if so. + with_retry(RETRY_NM_INTERVAL_MILLISECONDS, RETRY_NM_COUNT, || { +-- +2.41.0 + + +From c4b25a77c821ce45d861ec5cf70ebe0180b0fa2c Mon Sep 17 00:00:00 2001 +From: Gris Ge +Date: Tue, 6 Jun 2023 21:08:29 +0800 +Subject: [PATCH 02/13] apply: Perform pre-apply check before creating + checkpoint + +Do pre-apply check before creating the checkpoint when the desire does +not contains future VF(in this case the pre-check only happens after +SRIOV VF count changed). + +It is hard to test this in auto test case. Manual checked using +overbook bond port. + +Signed-off-by: Gris Ge +--- + rust/src/lib/query_apply/net_state.rs | 36 +++++++++++++++++++++------ + 1 file changed, 29 insertions(+), 7 deletions(-) + +diff --git a/rust/src/lib/query_apply/net_state.rs b/rust/src/lib/query_apply/net_state.rs +index bef8f3ff..772cc75b 100644 +--- a/rust/src/lib/query_apply/net_state.rs ++++ b/rust/src/lib/query_apply/net_state.rs +@@ -7,7 +7,7 @@ use crate::{ + nm_checkpoint_rollback, nm_checkpoint_timeout_extend, nm_retrieve, + }, + ovsdb::{ovsdb_apply, ovsdb_is_running, ovsdb_retrieve}, +- MergedNetworkState, NetworkState, NmstateError, ++ ErrorKind, MergedNetworkState, NetworkState, NmstateError, + }; + + const DEFAULT_ROLLBACK_TIMEOUT: u32 = 60; +@@ -97,6 +97,7 @@ impl NetworkState { + } + + fn apply_with_nm_backend(&self) -> Result<(), NmstateError> { ++ let mut merged_state = None; + let mut cur_net_state = NetworkState::new(); + cur_net_state.set_kernel_only(self.kernel_only); + cur_net_state.set_include_secrets(true); +@@ -123,6 +124,16 @@ impl NetworkState { + None + }; + ++ if pf_state.is_none() { ++ // Do early pre-apply validation before checkpoint. ++ merged_state = Some(MergedNetworkState::new( ++ self.clone(), ++ cur_net_state.clone(), ++ false, ++ self.memory_only, ++ )?); ++ } ++ + let mut timeout = self.timeout.unwrap_or(DEFAULT_ROLLBACK_TIMEOUT); + let verify_count = if self.has_vf_count_change(&cur_net_state) { + timeout = VERIFY_RETRY_COUNT_SRIOV as u32 +@@ -167,15 +178,26 @@ impl NetworkState { + )?; + // Refresh current state + cur_net_state.retrieve()?; ++ merged_state = Some(MergedNetworkState::new( ++ self.clone(), ++ cur_net_state.clone(), ++ false, ++ self.memory_only, ++ )?); + } + ++ let merged_state = if let Some(merged_state) = merged_state { ++ merged_state ++ } else { ++ return Err(NmstateError::new( ++ ErrorKind::Bug, ++ "Got unexpected None for merged_state in \ ++ apply_with_nm_backend()" ++ .into(), ++ )); ++ }; ++ + self.interfaces.check_sriov_capability()?; +- let merged_state = MergedNetworkState::new( +- self.clone(), +- cur_net_state.clone(), +- false, +- self.memory_only, +- )?; + + self.apply_with_nm_backend_and_under_checkpoint( + &merged_state, +-- +2.41.0 + + +From 8764f4500fb2d58cd6a5358be6cf3b6431c73a7f Mon Sep 17 00:00:00 2001 +From: Gris Ge +Date: Tue, 6 Jun 2023 21:21:15 +0800 +Subject: [PATCH 03/13] sriov: Handle unknown interface type for SRIOV + +When changing SRIOV VF change without defining interface type, nmstate +will incorrectly treat it as non-SRIOV interface which lead to incorrect +verification retry count (5). + +To support that use case, we need to use `MergedNetworkState` to resolve +the unknown interface type, then check whether we have SRIOV VF changes +to determine the timeout and verification retry count. + +Unit test case included. +Integration test case `test_change_vf_from_62_to_63` does not have +interface type defined which is enough to reproduce this issue after +multiple retry. + +Signed-off-by: Gris Ge +--- + rust/src/lib/query_apply/ethernet.rs | 48 ++++++++++++++++++++------- + rust/src/lib/query_apply/net_state.rs | 10 +++++- + rust/src/lib/unit_tests/sriov.rs | 31 +++++++++++++++++ + 3 files changed, 76 insertions(+), 13 deletions(-) + +diff --git a/rust/src/lib/query_apply/ethernet.rs b/rust/src/lib/query_apply/ethernet.rs +index d1fc5c24..9de3814f 100644 +--- a/rust/src/lib/query_apply/ethernet.rs ++++ b/rust/src/lib/query_apply/ethernet.rs +@@ -2,7 +2,8 @@ + + use crate::{ + ErrorKind, EthernetConfig, EthernetInterface, Interface, InterfaceType, +- Interfaces, NetworkState, NmstateError, SrIovConfig, VethConfig, ++ Interfaces, MergedInterfaces, NetworkState, NmstateError, SrIovConfig, ++ VethConfig, + }; + + impl EthernetInterface { +@@ -50,6 +51,20 @@ impl EthernetInterface { + } + Ok(()) + } ++ ++ pub(crate) fn is_vf_count_changed(&self, cur_iface: &Self) -> bool { ++ let pf_count = self ++ .ethernet ++ .as_ref() ++ .and_then(|e| e.sr_iov.as_ref()) ++ .and_then(|s| s.total_vfs); ++ let cur_pf_count = cur_iface ++ .ethernet ++ .as_ref() ++ .and_then(|e| e.sr_iov.as_ref()) ++ .and_then(|s| s.total_vfs); ++ pf_count.is_some() && pf_count != cur_pf_count ++ } + } + + impl EthernetConfig { +@@ -125,17 +140,7 @@ impl NetworkState { + Some(Interface::Ethernet(cur_iface)), + ) = (iface, current.interfaces.kernel_ifaces.get(iface.name())) + { +- let pf_count = iface +- .ethernet +- .as_ref() +- .and_then(|e| e.sr_iov.as_ref()) +- .and_then(|s| s.total_vfs); +- let cur_pf_count = cur_iface +- .ethernet +- .as_ref() +- .and_then(|e| e.sr_iov.as_ref()) +- .and_then(|s| s.total_vfs); +- if pf_count.is_some() && pf_count != cur_pf_count { ++ if iface.is_vf_count_changed(cur_iface) { + return true; + } + } +@@ -195,3 +200,22 @@ impl NetworkState { + } + } + } ++ ++impl MergedInterfaces { ++ pub(crate) fn has_vf_count_change(&self) -> bool { ++ for iface in self.kernel_ifaces.values().filter(|i| { ++ i.is_desired() && i.merged.iface_type() == InterfaceType::Ethernet ++ }) { ++ if let ( ++ Some(Interface::Ethernet(des_iface)), ++ Some(Interface::Ethernet(cur_iface)), ++ ) = (iface.for_apply.as_ref(), iface.current.as_ref()) ++ { ++ if des_iface.is_vf_count_changed(cur_iface) { ++ return true; ++ } ++ } ++ } ++ false ++ } ++} +diff --git a/rust/src/lib/query_apply/net_state.rs b/rust/src/lib/query_apply/net_state.rs +index 772cc75b..4653b830 100644 +--- a/rust/src/lib/query_apply/net_state.rs ++++ b/rust/src/lib/query_apply/net_state.rs +@@ -135,7 +135,15 @@ impl NetworkState { + } + + let mut timeout = self.timeout.unwrap_or(DEFAULT_ROLLBACK_TIMEOUT); +- let verify_count = if self.has_vf_count_change(&cur_net_state) { ++ // We need to use merge state in case PF does not have ++ // interface type defined, we need merged_state to have `unknown` type ++ // resolved ++ let verify_count = if pf_state.is_some() ++ || merged_state ++ .as_ref() ++ .map(|s| s.interfaces.has_vf_count_change()) ++ == Some(true) ++ { + timeout = VERIFY_RETRY_COUNT_SRIOV as u32 + * VERIFY_RETRY_INTERVAL_MILLISECONDS as u32 + / 1000; +diff --git a/rust/src/lib/unit_tests/sriov.rs b/rust/src/lib/unit_tests/sriov.rs +index 09a8355d..4fa55232 100644 +--- a/rust/src/lib/unit_tests/sriov.rs ++++ b/rust/src/lib/unit_tests/sriov.rs +@@ -789,3 +789,34 @@ fn test_sriov_vf_revert_to_default() { + panic!("Expecting a Ethernet interface, but got {:?}", verify_iface); + } + } ++ ++#[test] ++fn test_has_vf_change_with_unknown_iface_type() { ++ let desired = serde_yaml::from_str::( ++ r#"--- ++ - name: eth1 ++ state: up ++ ethernet: ++ sr-iov: ++ total-vfs: 2 ++ "#, ++ ) ++ .unwrap(); ++ ++ let current = serde_yaml::from_str::( ++ r#"--- ++ - name: eth1 ++ type: ethernet ++ state: up ++ ethernet: ++ sr-iov: ++ total-vfs: 1 ++ "#, ++ ) ++ .unwrap(); ++ ++ let merged_ifaces = ++ MergedInterfaces::new(desired, current, false, false).unwrap(); ++ ++ assert!(merged_ifaces.has_vf_count_change()); ++} +-- +2.41.0 + + +From 18a9456f929843c37b707687251bf4c46f4aef5f Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Tue, 23 May 2023 13:35:01 -0400 +Subject: [PATCH 04/13] cli: Teach `persist-nic-names` to pin via kargs too + +Pinning by kargs has the additional benefit of also covering the +initramfs, which is relevant if networking is necessary there (for +e.g. a Tang-pinned rootfs), and network kargs are in use that rely on a +specific interface name. + +To keep it simpler, we don't have nmstate itself call out to rpm-ostree. +Instead, a new `--kargs-out` flag is added where nmstate can write out +the kargs that the MCO should add/remove. + +Related: https://github.com/openshift/machine-config-operator/pull/3684 +Related: https://github.com/openshift/machine-config-operator/pull/3706 + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/ncl.rs | 11 +++++++ + rust/src/cli/persist_nic.rs | 60 ++++++++++++++++++++++++++++++++++--- + 2 files changed, 67 insertions(+), 4 deletions(-) + +diff --git a/rust/src/cli/ncl.rs b/rust/src/cli/ncl.rs +index 7670c34c..7cd59e58 100644 +--- a/rust/src/cli/ncl.rs ++++ b/rust/src/cli/ncl.rs +@@ -343,6 +343,16 @@ fn main() { + which has no effect", + ), + ) ++ .arg( ++ clap::Arg::new("KARGSFILE") ++ .long("kargs-out") ++ .takes_value(true) ++ .help( ++ "When pinning, write kargs to append; \ ++ when cleaning up, write kargs to delete \ ++ (space-separated)", ++ ), ++ ) + .arg( + clap::Arg::new("ROOT") + .long("root") +@@ -448,6 +458,7 @@ fn main() { + }; + print_result_and_exit(crate::persist_nic::run_persist_immediately( + matches.value_of("ROOT").unwrap(), ++ matches.value_of("KARGSFILE"), + action, + )); + } +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 61b07dbb..1e3e3fa4 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -63,13 +63,14 @@ fn gather_state() -> Result { + + pub(crate) fn run_persist_immediately( + root: &str, ++ kargsfile: Option<&str>, + action: PersistAction, + ) -> Result { + let dry_run = match action { + PersistAction::Save => false, + PersistAction::DryRun => true, +- PersistAction::CleanUp => return clean_up(root, false), +- PersistAction::CleanUpDryRun => return clean_up(root, true), ++ PersistAction::CleanUp => return clean_up(root, kargsfile, false), ++ PersistAction::CleanUpDryRun => return clean_up(root, kargsfile, true), + }; + + if is_prediable_ifname_disabled() { +@@ -88,6 +89,7 @@ pub(crate) fn run_persist_immediately( + return Ok("".to_string()); + } + ++ let mut kargs: Vec = Vec::new(); + let state = gather_state()?; + let mut changed = false; + for iface in state +@@ -109,9 +111,13 @@ pub(crate) fn run_persist_immediately( + "Would persist the interface {} with MAC {mac}", + iface.name() + ); ++ let iface_name = iface.name(); ++ let karg = format_ifname_karg(iface_name, mac); ++ log::info!("Would append kernel argument {karg}"); + if !dry_run { + changed |= +- persist_iface_name_via_systemd_link(root, mac, iface.name())?; ++ persist_iface_name_via_systemd_link(root, mac, iface_name)?; ++ kargs.push(karg); + } + } + +@@ -121,6 +127,11 @@ pub(crate) fn run_persist_immediately( + + if !dry_run { + std::fs::write(stamp_path, b"")?; ++ if !kargs.is_empty() { ++ if let Some(path) = kargsfile { ++ std::fs::write(path, kargs.join(" "))?; ++ } ++ } + } + + Ok("".to_string()) +@@ -139,7 +150,11 @@ fn extract_iface_names_from_link_file(file_name: &str) -> Option { + .map(ToOwned::to_owned) + } + +-pub(crate) fn clean_up(root: &str, dry_run: bool) -> Result { ++pub(crate) fn clean_up( ++ root: &str, ++ kargsfile: Option<&str>, ++ dry_run: bool, ++) -> Result { + let netdir = Path::new(root).join(SYSTEMD_NETWORK_LINK_FOLDER); + + if !netdir.exists() { +@@ -179,6 +194,21 @@ pub(crate) fn clean_up(root: &str, dry_run: bool) -> Result { + return Ok("".to_string()); + } + ++ let state = gather_state()?; ++ let macs: HashMap<&str, &str> = state ++ .interfaces ++ .iter() ++ .filter(|i| i.iface_type() == InterfaceType::Ethernet) ++ .filter_map(|i| { ++ i.base_iface() ++ .permanent_mac_address ++ .as_deref() ++ .or_else(|| i.base_iface().mac_address.as_deref()) ++ .map(|m| (i.name(), m)) ++ }) ++ .collect(); ++ ++ let mut kargs: Vec = Vec::new(); + for (iface_name, file_path) in pinned_ifaces { + if !is_nmstate_generated_systemd_link_file(&file_path) { + log::info!( +@@ -199,6 +229,19 @@ pub(crate) fn clean_up(root: &str, dry_run: bool) -> Result { + } + }; + if systemd_iface_name == iface_name { ++ if let Some(mac) = macs.get(iface_name.as_str()) { ++ let karg = format_ifname_karg(&iface_name, mac); ++ if dry_run { ++ log::info!( ++ "Will remove kernel arg {} if not in dry-run mode", ++ karg ++ ); ++ } else { ++ kargs.push(karg); ++ } ++ } else { ++ log::error!("Pinned iface {iface_name} has no MAC address"); ++ } + if dry_run { + log::info!( + "The generated {} file has no effect for \ +@@ -225,10 +268,19 @@ pub(crate) fn clean_up(root: &str, dry_run: bool) -> Result { + } + if !dry_run { + std::fs::remove_file(stamp_path)?; ++ if !kargs.is_empty() { ++ if let Some(path) = kargsfile { ++ std::fs::write(path, kargs.join(" "))?; ++ } ++ } + } + Ok("".to_string()) + } + ++fn format_ifname_karg(ifname: &str, mac: &str) -> String { ++ format!("ifname={ifname}:{mac}") ++} ++ + // With `NamePolicy=keep kernel database onboard slot path` in systemd configure + // in RHEL 8 and 9. Assuming `keep, kernel and database` all return NULL, + // Systemd will use interface name in the order of: +-- +2.41.0 + + +From 87058539c828ed2dac99bac06075d8e078152fc8 Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Wed, 24 May 2023 11:56:01 -0400 +Subject: [PATCH 05/13] cli: Mention kargs in persist_nic doc and minor tweaks + +Add kernel arguments to the `persist_nic` module docstring. Fix a typo +in the predictable ifname function name. Don't capitalize systemd at the +beginning of sentences. + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 21 +++++++++++---------- + 1 file changed, 11 insertions(+), 10 deletions(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 1e3e3fa4..3e0f0faf 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -3,20 +3,21 @@ + //! # Handling writing .link files for NICs + //! + //! This module implements logic for generating systemd [`.link`] files +-//! based on active networking state. ++//! and kernel arguments based on active networking state. + //! + //! The logic currently is: + //! + //! - Do nothing if kernel argument contains `net.ifnames=0` which disabled the +-//! predicable network interface name, hence not fit our use case here. ++//! predictable network interface name, hence not fit our use case here. + //! - Iterate over all active NICs +-//! - Pin every ethernet interface to its MAC address (prefer permanent MAC +-//! address) ++//! - Pin every Ethernet interface to its MAC address (prefer permanent MAC ++//! address) using link files and the [`ifname=`] kernel argument. + //! - After booting to new environment, use `udevadm test-builtin net_id` to + //! check whether pined interface name is different from systemd UDEV + //! Generated one. If still the same, remove the `.link` file. + //! + //! [`.link`]: https://www.freedesktop.org/software/systemd/man/systemd.link.html ++//! [`ifname=`]: https://www.man7.org/linux/man-pages/man7/dracut.cmdline.7.html + use std::collections::HashMap; + use std::io::Read; + use std::path::{Path, PathBuf}; +@@ -73,9 +74,9 @@ pub(crate) fn run_persist_immediately( + PersistAction::CleanUpDryRun => return clean_up(root, kargsfile, true), + }; + +- if is_prediable_ifname_disabled() { ++ if is_predictable_ifname_disabled() { + log::info!( +- "Systemd predicable network interface name is disabled \ ++ "systemd predictable network interface name is disabled \ + by kernel argument `net.ifnames=0`, will do nothing" + ); + return Ok("".to_string()); +@@ -259,7 +260,7 @@ pub(crate) fn clean_up( + } + } else { + log::info!( +- "Systemd generate interface name \ ++ "systemd generated interface name \ + '{systemd_iface_name}' != pinned name '{iface_name}', \ + will keep config file {}", + file_path.display() +@@ -283,7 +284,7 @@ fn format_ifname_karg(ifname: &str, mac: &str) -> String { + + // With `NamePolicy=keep kernel database onboard slot path` in systemd configure + // in RHEL 8 and 9. Assuming `keep, kernel and database` all return NULL, +-// Systemd will use interface name in the order of: ++// systemd will use interface name in the order of: + // * `ID_NET_NAME_ONBOARD` + // * `ID_NET_NAME_SLOT` + // * `ID_NET_NAME_PATH` +@@ -364,7 +365,7 @@ fn persist_iface_name_via_systemd_link( + )) + })?; + log::info!( +- "Systemd network link file created at {}", ++ "systemd network link file created at {}", + file_path.display() + ); + Ok(true) +@@ -382,7 +383,7 @@ fn is_nmstate_generated_systemd_link_file(file_path: &PathBuf) -> bool { + + const KERNEL_CMDLINE_FILE: &str = "/proc/cmdline"; + +-fn is_prediable_ifname_disabled() -> bool { ++fn is_predictable_ifname_disabled() -> bool { + std::fs::read(KERNEL_CMDLINE_FILE) + .map(|v| String::from_utf8(v).unwrap_or_default()) + .map(|c| c.contains("net.ifnames=0")) +-- +2.41.0 + + +From 1da7f70d8840974b248d3a70b067c5081367132f Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Wed, 24 May 2023 11:59:07 -0400 +Subject: [PATCH 06/13] cli: Simplify and make consistent logging in + `persist-nic-names` + +In the pinning path, we used "would", which would be more appropriate +for dry run mode, but we also used it in non-dry run mode. In the +cleanup path, we used "will" only in dry run mode. + +Since "will" works for both dry run mode and non-dry run mode, tweak +things so we always say "will". This simplifies things since we don't +need different logging for each mode. + +In the cleanup path, log when we actually remove the link file for +consistency with the pinning path. + +While we're here, merge the kernel arg and link file logging. + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 50 ++++++++++++++++--------------------- + 1 file changed, 22 insertions(+), 28 deletions(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 3e0f0faf..ed218e04 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -108,16 +108,16 @@ pub(crate) fn run_persist_immediately( + Some(m) => m, + None => continue, + }; +- log::info!( +- "Would persist the interface {} with MAC {mac}", +- iface.name() +- ); + let iface_name = iface.name(); + let karg = format_ifname_karg(iface_name, mac); +- log::info!("Would append kernel argument {karg}"); ++ log::info!( ++ "Will persist the interface {iface_name} with MAC {mac} \ ++ using link file and kernel argument {karg}" ++ ); + if !dry_run { + changed |= + persist_iface_name_via_systemd_link(root, mac, iface_name)?; ++ log::info!("Kernel argument {karg} appended"); + kargs.push(karg); + } + } +@@ -230,33 +230,27 @@ pub(crate) fn clean_up( + } + }; + if systemd_iface_name == iface_name { +- if let Some(mac) = macs.get(iface_name.as_str()) { +- let karg = format_ifname_karg(&iface_name, mac); +- if dry_run { +- log::info!( +- "Will remove kernel arg {} if not in dry-run mode", +- karg +- ); +- } else { +- kargs.push(karg); ++ log::info!("Interface name {iface_name} is unchanged"); ++ let mac = match macs.get(iface_name.as_str()) { ++ Some(mac) => mac, ++ None => { ++ log::error!("Interface {iface_name} has no MAC address"); ++ continue; + } +- } else { +- log::error!("Pinned iface {iface_name} has no MAC address"); +- } +- if dry_run { +- log::info!( +- "The generated {} file has no effect for \ +- interface {iface_name}, will remove if not \ +- in dry-run mode", +- file_path.display() +- ); +- } else { ++ }; ++ let karg = format_ifname_karg(&iface_name, mac); ++ log::info!( ++ "Will remove generated file {} and kernel argument {karg}", ++ file_path.display() ++ ); ++ if !dry_run { ++ std::fs::remove_file(&file_path)?; + log::info!( +- "The generated {} file has no effect for \ +- interface {iface_name}, removing", ++ "Removed systemd network link file {}", + file_path.display() + ); +- std::fs::remove_file(file_path)?; ++ log::info!("Kernel argument {karg} removed"); ++ kargs.push(karg); + } + } else { + log::info!( +-- +2.41.0 + + +From d75ff094dadd708a5c58b2d9d62d68b39ea9fbe5 Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Wed, 24 May 2023 17:25:35 -0400 +Subject: [PATCH 07/13] cli: Strengthen karg checking for net.ifnames=0 + +A karg like `foonet.ifnames=0` will make this function think that +predictable ifnames are disabled. We need the check to be word boundary- +aware. + +Split the kernel cmdline on whitespace and check for each element. + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index ed218e04..adf19566 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -380,6 +380,6 @@ const KERNEL_CMDLINE_FILE: &str = "/proc/cmdline"; + fn is_predictable_ifname_disabled() -> bool { + std::fs::read(KERNEL_CMDLINE_FILE) + .map(|v| String::from_utf8(v).unwrap_or_default()) +- .map(|c| c.contains("net.ifnames=0")) ++ .map(|c| c.split(' ').any(|x| x == "net.ifnames=0")) + .unwrap_or_default() + } +-- +2.41.0 + + +From aa1b79f7f89bde8e1514a117159b5e81b8bd65e7 Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Wed, 24 May 2023 17:27:47 -0400 +Subject: [PATCH 08/13] cli: Split out `has_any_kargs` helper function + +Prep for checking for another karg. + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 6 +++++- + 1 file changed, 5 insertions(+), 1 deletion(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index adf19566..4ad7373f 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -378,8 +378,12 @@ fn is_nmstate_generated_systemd_link_file(file_path: &PathBuf) -> bool { + const KERNEL_CMDLINE_FILE: &str = "/proc/cmdline"; + + fn is_predictable_ifname_disabled() -> bool { ++ has_any_kargs(&["net.ifnames=0"]) ++} ++ ++fn has_any_kargs(kargs: &[&str]) -> bool { + std::fs::read(KERNEL_CMDLINE_FILE) + .map(|v| String::from_utf8(v).unwrap_or_default()) +- .map(|c| c.split(' ').any(|x| x == "net.ifnames=0")) ++ .map(|c| c.split(' ').any(|x| kargs.contains(&x))) + .unwrap_or_default() + } +-- +2.41.0 + + +From 851640fec5cdd9bd695565f8eb1d0a79b79599bd Mon Sep 17 00:00:00 2001 +From: Jonathan Lebon +Date: Wed, 24 May 2023 21:42:22 -0400 +Subject: [PATCH 09/13] cli: Only append `ifname` kargs if `rd.neednet` is used + +If networking isn't required in the initramfs, then there's no need to +append `ifname` kargs. Key off of the `rd.neednet` karg to know if that's +the case. + +Signed-off-by: Jonathan Lebon +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 42 ++++++++++++++++++++++++++----------- + 1 file changed, 30 insertions(+), 12 deletions(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 4ad7373f..71df8bd4 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -91,6 +91,11 @@ pub(crate) fn run_persist_immediately( + } + + let mut kargs: Vec = Vec::new(); ++ let with_kargs = is_initrd_networking_enabled(); ++ if with_kargs { ++ log::info!("Host uses initrd networking"); ++ } ++ + let state = gather_state()?; + let mut changed = false; + for iface in state +@@ -110,15 +115,17 @@ pub(crate) fn run_persist_immediately( + }; + let iface_name = iface.name(); + let karg = format_ifname_karg(iface_name, mac); +- log::info!( +- "Will persist the interface {iface_name} with MAC {mac} \ +- using link file and kernel argument {karg}" +- ); ++ log::info!("Will persist the interface {iface_name} with MAC {mac}"); ++ if with_kargs { ++ log::info!("Will append kernel argument {karg}"); ++ } + if !dry_run { + changed |= + persist_iface_name_via_systemd_link(root, mac, iface_name)?; +- log::info!("Kernel argument {karg} appended"); +- kargs.push(karg); ++ if with_kargs { ++ log::info!("Kernel argument {karg} appended"); ++ kargs.push(karg); ++ } + } + } + +@@ -210,6 +217,11 @@ pub(crate) fn clean_up( + .collect(); + + let mut kargs: Vec = Vec::new(); ++ let with_kargs = is_initrd_networking_enabled(); ++ if with_kargs { ++ log::info!("Host uses initrd networking"); ++ } ++ + for (iface_name, file_path) in pinned_ifaces { + if !is_nmstate_generated_systemd_link_file(&file_path) { + log::info!( +@@ -239,18 +251,20 @@ pub(crate) fn clean_up( + } + }; + let karg = format_ifname_karg(&iface_name, mac); +- log::info!( +- "Will remove generated file {} and kernel argument {karg}", +- file_path.display() +- ); ++ log::info!("Will remove generated file {}", file_path.display()); ++ if with_kargs { ++ log::info!("Will remove kernel argument {karg}"); ++ } + if !dry_run { + std::fs::remove_file(&file_path)?; + log::info!( + "Removed systemd network link file {}", + file_path.display() + ); +- log::info!("Kernel argument {karg} removed"); +- kargs.push(karg); ++ if with_kargs { ++ log::info!("Kernel argument {karg} removed"); ++ kargs.push(karg); ++ } + } + } else { + log::info!( +@@ -381,6 +395,10 @@ fn is_predictable_ifname_disabled() -> bool { + has_any_kargs(&["net.ifnames=0"]) + } + ++fn is_initrd_networking_enabled() -> bool { ++ has_any_kargs(&["rd.neednet=1", "rd.neednet"]) ++} ++ + fn has_any_kargs(kargs: &[&str]) -> bool { + std::fs::read(KERNEL_CMDLINE_FILE) + .map(|v| String::from_utf8(v).unwrap_or_default()) +-- +2.41.0 + + +From acd1e9e6e7ea682c367f2cf0d1f96f0a10fdd428 Mon Sep 17 00:00:00 2001 +From: Colin Walters +Date: Tue, 30 May 2023 16:27:42 -0400 +Subject: [PATCH 10/13] cli: Readd `perist-nic-names --inspect`, rework + internals + +Today the logs from this invocation may end up either +in the systemd journal or in a container log. + +And after that one run, they will quickly get lost back in the +shuffle. + +The original motivation behind `--inspect` is to be able +to quickly and conveniently see the state of the system; it's +something we can run every time the OpenShift machine-config-daemon +starts up on a node. + +That way, we'll always be able to look at the logs and see what +happened with NIC persistence. + +In the implementation of this, rework things such that "dry_run" +is a bool passed down instead of augmenting the action. + +This makes the code flow much more clearly; instead of having +an early return in `run_persist_immediately` if the action is +to cleanup we separate the persist action into its own function +too, and then just dispatch to one or the other. + +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/ncl.rs | 24 ++++++++++--------- + rust/src/cli/persist_nic.rs | 48 +++++++++++++++++++++---------------- + 2 files changed, 40 insertions(+), 32 deletions(-) + +diff --git a/rust/src/cli/ncl.rs b/rust/src/cli/ncl.rs +index 7cd59e58..fd9d1e5a 100644 +--- a/rust/src/cli/ncl.rs ++++ b/rust/src/cli/ncl.rs +@@ -334,6 +334,12 @@ fn main() { + .takes_value(false) + .help("Only output changes that would be made"), + ) ++ .arg( ++ clap::Arg::new("INSPECT") ++ .long("inspect") ++ .takes_value(false) ++ .help("Output information about prior state, if any"), ++ ) + .arg( + clap::Arg::new("CLEAN_UP") + .long("cleanup") +@@ -442,24 +448,20 @@ fn main() { + if let Some(matches) = + matches.subcommand_matches(SUB_CMD_PERSIST_NIC_NAMES) + { +- let action = if matches +- .try_contains_id("DRY_RUN") +- .unwrap_or_default() +- { +- if matches.try_contains_id("CLEAN_UP").unwrap_or_default() { +- persist_nic::PersistAction::CleanUpDryRun +- } else { +- persist_nic::PersistAction::DryRun +- } +- } else if matches.try_contains_id("CLEAN_UP").unwrap_or_default() { ++ // --inspect is now equivalent to --cleanup --dry-run and kept for backwards compatibility ++ // with the logic that originally landed in https://github.com/openshift/machine-config-operator/ ++ let have_inspect = matches.contains_id("INSPECT"); ++ let dry_run = matches.contains_id("DRY_RUN") || have_inspect; ++ let action = if matches.contains_id("CLEAN_UP") || have_inspect { + persist_nic::PersistAction::CleanUp + } else { + persist_nic::PersistAction::Save + }; +- print_result_and_exit(crate::persist_nic::run_persist_immediately( ++ print_result_and_exit(crate::persist_nic::entrypoint( + matches.value_of("ROOT").unwrap(), + matches.value_of("KARGSFILE"), + action, ++ dry_run, + )); + } + } +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 71df8bd4..8fc2b0ab 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -46,12 +46,8 @@ const ID_NET_NAME_PATH: &str = "ID_NET_NAME_PATH"; + pub(crate) enum PersistAction { + /// Persist NIC name state + Save, +- /// Print what we would do in Save mode +- DryRun, + /// Remove link files not required + CleanUp, +- /// Print what we would do in clean up mode +- CleanUpDryRun, + } + + fn gather_state() -> Result { +@@ -62,18 +58,12 @@ fn gather_state() -> Result { + Ok(state) + } + +-pub(crate) fn run_persist_immediately( ++pub(crate) fn entrypoint( + root: &str, + kargsfile: Option<&str>, + action: PersistAction, ++ dry_run: bool, + ) -> Result { +- let dry_run = match action { +- PersistAction::Save => false, +- PersistAction::DryRun => true, +- PersistAction::CleanUp => return clean_up(root, kargsfile, false), +- PersistAction::CleanUpDryRun => return clean_up(root, kargsfile, true), +- }; +- + if is_predictable_ifname_disabled() { + log::info!( + "systemd predictable network interface name is disabled \ +@@ -82,6 +72,19 @@ pub(crate) fn run_persist_immediately( + return Ok("".to_string()); + } + ++ match action { ++ PersistAction::Save => { ++ run_persist_immediately(root, kargsfile, dry_run) ++ } ++ PersistAction::CleanUp => clean_up(root, kargsfile, dry_run), ++ } ++} ++ ++fn run_persist_immediately( ++ root: &str, ++ kargsfile: Option<&str>, ++ dry_run: bool, ++) -> Result { + let stamp_path = Path::new(root) + .join(SYSTEMD_NETWORK_LINK_FOLDER) + .join(NMSTATE_PERSIST_STAMP); +@@ -113,6 +116,14 @@ pub(crate) fn run_persist_immediately( + Some(m) => m, + None => continue, + }; ++ let file_path = gen_link_file_path(root, iface.name()); ++ if file_path.exists() { ++ log::info!( ++ "Network link file {} already exists", ++ file_path.display() ++ ); ++ continue; ++ } + let iface_name = iface.name(); + let karg = format_ifname_karg(iface_name, mac); + log::info!("Will persist the interface {iface_name} with MAC {mac}"); +@@ -120,13 +131,13 @@ pub(crate) fn run_persist_immediately( + log::info!("Will append kernel argument {karg}"); + } + if !dry_run { +- changed |= +- persist_iface_name_via_systemd_link(root, mac, iface_name)?; ++ persist_iface_name_via_systemd_link(root, mac, iface_name)?; + if with_kargs { + log::info!("Kernel argument {karg} appended"); + kargs.push(karg); + } + } ++ changed = true; + } + + if !changed { +@@ -350,15 +361,10 @@ fn persist_iface_name_via_systemd_link( + root: &str, + mac: &str, + iface_name: &str, +-) -> Result { ++) -> Result<(), CliError> { + let link_dir = Path::new(root).join(SYSTEMD_NETWORK_LINK_FOLDER); + + let file_path = gen_link_file_path(root, iface_name); +- if file_path.exists() { +- log::info!("Network link file {} already exists", file_path.display()); +- return Ok(false); +- } +- + if !link_dir.exists() { + std::fs::create_dir(&link_dir)?; + } +@@ -376,7 +382,7 @@ fn persist_iface_name_via_systemd_link( + "systemd network link file created at {}", + file_path.display() + ); +- Ok(true) ++ Ok(()) + } + + fn is_nmstate_generated_systemd_link_file(file_path: &PathBuf) -> bool { +-- +2.41.0 + + +From 1b85892b28e59b13842be30306beaa3afb4dd582 Mon Sep 17 00:00:00 2001 +From: Colin Walters +Date: Wed, 31 May 2023 13:12:41 -0400 +Subject: [PATCH 11/13] cli: Ensure stamp path parent exists + +Fixes https://issues.redhat.com/browse/OCPBUGS-14298 + +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 8fc2b0ab..37a10f27 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -145,6 +145,11 @@ fn run_persist_immediately( + } + + if !dry_run { ++ if let Some(parent) = stamp_path.parent() { ++ if !parent.exists() { ++ std::fs::create_dir(parent)?; ++ } ++ } + std::fs::write(stamp_path, b"")?; + if !kargs.is_empty() { + if let Some(path) = kargsfile { +-- +2.41.0 + + +From 54e9e09210d58647dc9e5d0fd1c54a5d5bba437d Mon Sep 17 00:00:00 2001 +From: Colin Walters +Date: Wed, 31 May 2023 15:37:05 -0400 +Subject: [PATCH 12/13] cli: Deduplicate karg logging + +In `--dry-run` mode we don't actually write to `--karg-file` +anyways, so move the kargs handling outside of the `!dry_run` +conditional, which allows us to deduplicate the logging. + +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 23 +++++++++-------------- + 1 file changed, 9 insertions(+), 14 deletions(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 37a10f27..55ae3cd8 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -127,15 +127,12 @@ fn run_persist_immediately( + let iface_name = iface.name(); + let karg = format_ifname_karg(iface_name, mac); + log::info!("Will persist the interface {iface_name} with MAC {mac}"); +- if with_kargs { +- log::info!("Will append kernel argument {karg}"); +- } + if !dry_run { + persist_iface_name_via_systemd_link(root, mac, iface_name)?; +- if with_kargs { +- log::info!("Kernel argument {karg} appended"); +- kargs.push(karg); +- } ++ } ++ if with_kargs { ++ log::info!("Kernel argument added: {karg}"); ++ kargs.push(karg); + } + changed = true; + } +@@ -268,19 +265,17 @@ pub(crate) fn clean_up( + }; + let karg = format_ifname_karg(&iface_name, mac); + log::info!("Will remove generated file {}", file_path.display()); +- if with_kargs { +- log::info!("Will remove kernel argument {karg}"); +- } ++ + if !dry_run { + std::fs::remove_file(&file_path)?; + log::info!( + "Removed systemd network link file {}", + file_path.display() + ); +- if with_kargs { +- log::info!("Kernel argument {karg} removed"); +- kargs.push(karg); +- } ++ } ++ if with_kargs { ++ log::info!("Kernel argument removed: {karg}"); ++ kargs.push(karg); + } + } else { + log::info!( +-- +2.41.0 + + +From 8f2d8ef20866ec264946f85a55d9203fa8b56d17 Mon Sep 17 00:00:00 2001 +From: Colin Walters +Date: Thu, 1 Jun 2023 08:50:19 -0400 +Subject: [PATCH 13/13] cli: Make `persist-nic-names --cleanup` still print + state + +This way, we get the equivalent of `--inspect`. The MCO can just +run `--cleanup` on every boot and not have to care about +any implementation details. + +Signed-off-by: Colin Walters +Signed-off-by: Gris Ge +--- + rust/src/cli/persist_nic.rs | 12 +++++++++--- + 1 file changed, 9 insertions(+), 3 deletions(-) + +diff --git a/rust/src/cli/persist_nic.rs b/rust/src/cli/persist_nic.rs +index 55ae3cd8..7c6dcb54 100644 +--- a/rust/src/cli/persist_nic.rs ++++ b/rust/src/cli/persist_nic.rs +@@ -182,12 +182,12 @@ pub(crate) fn clean_up( + log::info!("{} does not exist, no need to clean up", netdir.display()); + } + let stamp_path = netdir.join(NMSTATE_PERSIST_STAMP); +- if !stamp_path.exists() { ++ let cleanup_pending = stamp_path.exists(); ++ if !cleanup_pending { + log::info!( +- "{} does not exist, no prior persisted state, no need to clean up", ++ "{} does not exist, no need to clean up", + stamp_path.display() + ); +- return Ok("".to_string()); + } + + let mut pinned_ifaces: HashMap = HashMap::new(); +@@ -215,6 +215,12 @@ pub(crate) fn clean_up( + return Ok("".to_string()); + } + ++ // If there wasn't a stamp file, at this point we've just printed out ++ // whether there were any persisted NICs found, and we're done. ++ if !cleanup_pending { ++ return Ok("".to_string()); ++ } ++ + let state = gather_state()?; + let macs: HashMap<&str, &str> = state + .interfaces +-- +2.41.0 + diff --git a/nmstate.spec b/nmstate.spec index 15ed944..253a80f 100644 --- a/nmstate.spec +++ b/nmstate.spec @@ -4,7 +4,7 @@ Name: nmstate Version: 2.2.12 -Release: 1%{?dist} +Release: 2%{?dist} Summary: Declarative network manager API License: LGPLv2+ URL: https://github.com/%{srcname}/%{srcname} @@ -12,6 +12,7 @@ Source0: https://github.com/nmstate/nmstate/releases/download/v%{version} Source1: https://github.com/nmstate/nmstate/releases/download/v%{version}/nmstate-%{version}.tar.gz.asc Source2: https://nmstate.io/nmstate.gpg Source3: https://github.com/nmstate/nmstate/releases/download/v%{version}/nmstate-vendor-%{version}.tar.xz +Patch1: BZ_2212380_fix_sriov_timeout.patch BuildRequires: python3-devel BuildRequires: python3-setuptools BuildRequires: gnupg2 @@ -150,6 +151,9 @@ popd /sbin/ldconfig %changelog +* Wed Jun 07 2023 Gris Ge - 2.2.12-2 +- Fix regression on SRIOV timeout. RHBZ#2212380 + * Thu Jun 01 2023 Fernando Fernandez Mancera - 2.2.12-1 - Upgrade to 2.2.12