rpms
/
nmstate
7
0
Fork 0
Code Issues Pull Requests Releases Activity
nmstate/SOURCES/Fix_SRIOV_timeout.patch

1301 lines
46 KiB
Diff
Raw Blame History

From bbd091cc566743dc9e218389efadf4654e3ff65d Mon Sep 17 00:00:00 2001
From: Gris Ge <fge@redhat.com>
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 <fge@redhat.com>
---
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 <fge@redhat.com>
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 <fge@redhat.com>
---
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 <fge@redhat.com>
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 <fge@redhat.com>
---
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::<Interfaces>(
+ r#"---
+ - name: eth1
+ state: up
+ ethernet:
+ sr-iov:
+ total-vfs: 2
+ "#,
+ )
+ .unwrap();
+
+ let current = serde_yaml::from_str::<Interfaces>(
+ 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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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<NetworkState, CliError> {
pub(crate) fn run_persist_immediately(
root: &str,
+ kargsfile: Option<&str>,
action: PersistAction,
) -> Result<String, CliError> {
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<String> = 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<String> {
.map(ToOwned::to_owned)
}
-pub(crate) fn clean_up(root: &str, dry_run: bool) -> Result<String, CliError> {
+pub(crate) fn clean_up(
+ root: &str,
+ kargsfile: Option<&str>,
+ dry_run: bool,
+) -> Result<String, CliError> {
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<String, CliError> {
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<String> = 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<String, CliError> {
}
};
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<String, CliError> {
}
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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <jonathan@jlebon.com>
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 <jonathan@jlebon.com>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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<String> = 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<String> = 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 <walters@verbum.org>
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 <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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<NetworkState, CliError> {
@@ -62,18 +58,12 @@ fn gather_state() -> Result<NetworkState, CliError> {
Ok(state)
}
-pub(crate) fn run_persist_immediately(
+pub(crate) fn entrypoint(
root: &str,
kargsfile: Option<&str>,
action: PersistAction,
+ dry_run: bool,
) -> Result<String, CliError> {
- 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<String, CliError> {
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<bool, CliError> {
+) -> 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 <walters@verbum.org>
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 <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <walters@verbum.org>
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 <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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 <walters@verbum.org>
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 <walters@verbum.org>
Signed-off-by: Gris Ge <fge@redhat.com>
---
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<String, PathBuf> = 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
Reference in New Issue View Git Blame Copy Permalink