Merge pull request jcronenberg#37 from jcronenberg/failure-handling

Add options to handle migration failure

rust/migrate-wicked/Cargo.toml

@@ -22,6 +22,7 @@ strum = "0.25.0"
 strum_macros = "0.25.2"
 serde_with = "3.3.0"
 tokio = { version = "1.33.0", features = ["macros", "rt-multi-thread"] }
+serde_ignored = "0.1.9"
 
 [[bin]]
 name = "migrate-wicked"

rust/migrate-wicked/src/interface.rs

@@ -325,8 +325,8 @@ where
     Ok(Slaves::deserialize(deserializer)?.slave)
 }
 
-impl From<Bond> for model::BondConfig {
-    fn from(bond: Bond) -> model::BondConfig {
+impl From<&Bond> for model::BondConfig {
+    fn from(bond: &Bond) -> model::BondConfig {
         let mut h: HashMap<String, String> = HashMap::new();
 
         h.insert(String::from("mode"), bond.mode.to_string());
@@ -433,73 +433,89 @@ impl From<Bond> for model::BondConfig {
 
         model::BondConfig {
             options: h,
-            hwaddr: match bond.address {
+            hwaddr: match &bond.address {
                 Some(s) => model::MacAddr::try_from(s.as_ref()).ok(),
                 _ => None,
             },
         }
     }
 }
 
-impl From<Interface> for model::Connection {
-    fn from(ifc: Interface) -> model::Connection {
+pub struct ConnectionResult {
+    pub connection: model::Connection,
+    pub warnings: Vec<anyhow::Error>,
+}
+
+pub struct IpConfigResult {
+    ip_config: IpConfig,
+    warnings: Vec<anyhow::Error>,
+}
+
+impl Interface {
+    pub fn to_connection(&self) -> Result<ConnectionResult, anyhow::Error> {
+        let ip_config = self.to_ip_config()?;
         let mut base = model::BaseConnection {
-            id: ifc.name.clone(),
-            interface: ifc.name.clone(),
-            ip_config: (&ifc).into(),
+            id: self.name.clone(),
+            interface: self.name.clone(),
+            ip_config: ip_config.ip_config,
             ..Default::default()
         };
 
-        if ifc.link.kind.is_some() && ifc.link.parent.is_some() {
-            let interface = ifc.link.parent.clone().unwrap();
-            let kind = match ifc.link.kind {
+        if self.link.kind.is_some() && self.link.parent.is_some() {
+            let interface = self.link.parent.clone().unwrap();
+            let kind = match &self.link.kind {
                 Some(p) => match &p {
                     ParentKind::Bond => model::ParentKind::Bond,
                 },
-                None => panic!("Missing ParentType"),
+                None => return Err(anyhow::anyhow!("Missing ParentType for {}", self.name)),
             };
             base.parent = Some(Parent { interface, kind });
         }
 
-        if let Some(b) = ifc.bond {
-            model::Connection::Bond(model::BondConnection {
-                base,
-                bond: b.into(),
-            })
-        } else {
-            model::Connection::Ethernet(model::EthernetConnection { base })
-        }
+        Ok(ConnectionResult {
+            connection: if let Some(b) = &self.bond {
+                model::Connection::Bond(model::BondConnection {
+                    base,
+                    bond: b.into(),
+                })
+            } else {
+                model::Connection::Ethernet(model::EthernetConnection { base })
+            },
+            warnings: ip_config.warnings,
+        })
     }
-}
 
-impl From<&Interface> for IpConfig {
-    fn from(val: &Interface) -> Self {
-        let method4 = Ipv4Method::from_str(if val.ipv4.enabled && val.ipv4_static.is_some() {
-            "manual"
-        } else if !val.ipv4.enabled {
-            "disabled"
+    pub fn to_ip_config(&self) -> Result<IpConfigResult, anyhow::Error> {
+        let mut connection_result = IpConfigResult {
+            ip_config: IpConfig {
+                ..Default::default()
+            },
+            warnings: vec![],
+        };
+        let method4 = if self.ipv4.enabled && self.ipv4_static.is_some() {
+            Ipv4Method::Manual
+        } else if !self.ipv4.enabled {
+            Ipv4Method::Disabled
         } else {
-            "auto"
-        })
-        .unwrap();
-        let method6 = Ipv6Method::from_str(if val.ipv6.enabled && val.ipv6_static.is_some() {
-            "manual"
-        } else if val.ipv6.enabled
-            && val.ipv6_dhcp.is_some()
-            && val.ipv6_dhcp.as_ref().unwrap().mode == "managed"
+            Ipv4Method::Auto
+        };
+        let method6 = if self.ipv6.enabled && self.ipv6_static.is_some() {
+            Ipv6Method::Manual
+        } else if self.ipv6.enabled
+            && self.ipv6_dhcp.is_some()
+            && self.ipv6_dhcp.as_ref().unwrap().mode == "managed"
         {
-            "dhcp"
-        } else if !val.ipv6.enabled {
-            "disabled"
+            Ipv6Method::Dhcp
+        } else if !self.ipv6.enabled {
+            Ipv6Method::Disabled
         } else {
-            "auto"
-        })
-        .unwrap();
+            Ipv6Method::Auto
+        };
 
         let mut addresses: Vec<IpInet> = vec![];
         let mut gateway4 = None;
         let mut gateway6 = None;
-        if let Some(ipv4_static) = &val.ipv4_static {
+        if let Some(ipv4_static) = &self.ipv4_static {
             if let Some(addresses_in) = &ipv4_static.addresses {
                 for addr in addresses_in {
                     addresses.push(IpInet::from_str(addr.local.as_str()).unwrap());
@@ -512,15 +528,17 @@ impl From<&Interface> for IpConfig {
                         // the logged warning isn't really true for multiple hops
                         // as gateways just can't have multiple nexthops AFAICT
                         if gateway4.is_some() || nexthops.len() > 1 {
-                            log::warn!("Multiple gateways aren't supported yet");
+                            connection_result.warnings.push(anyhow::anyhow!(
+                                "Multipath routing isn't natively supported by NetworkManager"
+                            ));
                         } else {
                             gateway4 = Some(IpAddr::from_str(&nexthops[0].gateway).unwrap());
                         }
                     }
                 }
             }
         }
-        if let Some(ipv6_static) = &val.ipv6_static {
+        if let Some(ipv6_static) = &self.ipv6_static {
             if let Some(addresses_in) = &ipv6_static.addresses {
                 for addr in addresses_in {
                     addresses.push(IpInet::from_str(addr.local.as_str()).unwrap());
@@ -533,7 +551,9 @@ impl From<&Interface> for IpConfig {
                         // the logged warning isn't really true for multiple hops
                         // as gateways just can't have multiple nexthops AFAICT
                         if gateway6.is_some() || nexthops.len() > 1 {
-                            log::warn!("Multiple gateways aren't supported yet");
+                            connection_result.warnings.push(anyhow::anyhow!(
+                                "Multipath routing isn't natively supported by NetworkManager"
+                            ));
                         } else {
                             gateway6 = Some(IpAddr::from_str(&nexthops[0].gateway).unwrap());
                         }
@@ -542,14 +562,15 @@ impl From<&Interface> for IpConfig {
             }
         }
 
-        IpConfig {
+        connection_result.ip_config = IpConfig {
             addresses,
             method4,
             method6,
             gateway4,
             gateway6,
             ..Default::default()
-        }
+        };
+        Ok(connection_result)
     }
 }
 
@@ -593,7 +614,8 @@ mod tests {
             ..Default::default()
         };
 
-        let static_connection: model::Connection = static_interface.into();
+        let static_connection: model::Connection =
+            static_interface.to_connection().unwrap().connection;
         assert_eq!(
             static_connection.base().ip_config.method4,
             Ipv4Method::Manual
@@ -652,7 +674,8 @@ mod tests {
             ..Default::default()
         };
 
-        let static_connection: model::Connection = static_interface.into();
+        let static_connection: model::Connection =
+            static_interface.to_connection().unwrap().connection;
         assert_eq!(static_connection.base().ip_config.method4, Ipv4Method::Auto);
         assert_eq!(static_connection.base().ip_config.method6, Ipv6Method::Auto);
         assert_eq!(static_connection.base().ip_config.addresses.len(), 0);
@@ -694,7 +717,7 @@ mod tests {
             address: Some(String::from("02:11:22:33:44:55")),
         };
 
-        let bondconfig: model::BondConfig = bond.into();
+        let bondconfig: model::BondConfig = (&bond).into();
         let s = HashMap::from([
             ("xmit_hash_policy", String::from("encap34")),
             ("packets_per_slave", 23.to_string()),

rust/migrate-wicked/src/main.rs

@@ -8,6 +8,7 @@ use log::*;
 use migrate::migrate;
 use reader::read as wicked_read;
 use std::process::{ExitCode, Termination};
+use tokio::sync::OnceCell;
 
 #[derive(Parser)]
 #[command(name = "migrate-wicked", version, about, long_about = None)]
@@ -40,6 +41,10 @@ pub enum Commands {
     Migrate {
         /// Wicked XML Files or directories where the wicked xml configs are located
         paths: Vec<String>,
+
+        /// Continue migration if warnings are encountered
+        #[arg(short, long, global = true)]
+        continue_migration: bool,
     },
 }
 
@@ -56,20 +61,37 @@ pub enum Format {
 async fn run_command(cli: Cli) -> anyhow::Result<()> {
     match cli.command {
         Commands::Show { paths, format } => {
-            let interfaces = wicked_read(paths)?;
+            MIGRATION_SETTINGS
+                .set(MigrationSettings {
+                    continue_migration: true,
+                })
+                .expect("MIGRATION_SETTINGS was set too early");
+
+            let interfaces_result = wicked_read(paths)?;
             let output: String = match format {
-                Format::Json => serde_json::to_string(&interfaces)?,
-                Format::PrettyJson => serde_json::to_string_pretty(&interfaces)?,
-                Format::Yaml => serde_yaml::to_string(&interfaces)?,
-                Format::Xml => quick_xml::se::to_string_with_root("interface", &interfaces)?,
-                Format::Text => format!("{:?}", interfaces),
+                Format::Json => serde_json::to_string(&interfaces_result.interfaces)?,
+                Format::PrettyJson => serde_json::to_string_pretty(&interfaces_result.interfaces)?,
+                Format::Yaml => serde_yaml::to_string(&interfaces_result.interfaces)?,
+                Format::Xml => {
+                    quick_xml::se::to_string_with_root("interface", &interfaces_result.interfaces)?
+                }
+                Format::Text => format!("{:?}", interfaces_result.interfaces),
             };
             println!("{}", output);
             Ok(())
         }
-        Commands::Migrate { paths } => {
-            migrate(paths).await.unwrap();
-            Ok(())
+        Commands::Migrate {
+            paths,
+            continue_migration,
+        } => {
+            MIGRATION_SETTINGS
+                .set(MigrationSettings { continue_migration })
+                .expect("MIGRATION_SETTINGS was set too early");
+
+            match migrate(paths).await {
+                Ok(()) => Ok(()),
+                Err(e) => Err(anyhow::anyhow!("Migration failed: {:?}", e)),
+            }
         }
     }
 }
@@ -88,6 +110,13 @@ impl Termination for CliResult {
     }
 }
 
+#[derive(Debug)]
+struct MigrationSettings {
+    continue_migration: bool,
+}
+
+static MIGRATION_SETTINGS: OnceCell<MigrationSettings> = OnceCell::const_new();
+
 #[tokio::main]
 async fn main() -> CliResult {
     let cli = Cli::parse();
@@ -104,5 +133,6 @@ async fn main() -> CliResult {
         eprintln!("{:?}", error);
         return CliResult::Error;
     }
+
     CliResult::Ok
 }

rust/migrate-wicked/src/migrate.rs

@@ -1,7 +1,6 @@
-use crate::reader::read as wicked_read;
+use crate::{reader::read as wicked_read, MIGRATION_SETTINGS};
 use agama_dbus_server::network::{model, Adapter, NetworkManagerAdapter, NetworkState};
 use std::error::Error;
-use tokio::{runtime::Handle, task};
 
 struct WickedAdapter {
     paths: Vec<String>,
@@ -15,18 +14,32 @@ impl WickedAdapter {
 
 impl Adapter for WickedAdapter {
     fn read(&self) -> Result<model::NetworkState, Box<dyn std::error::Error>> {
-        task::block_in_place(|| {
-            Handle::current().block_on(async {
-                let interfaces = wicked_read(self.paths.clone())?;
-                let mut state = NetworkState::new(vec![], vec![]);
-
-                for interface in interfaces {
-                    let conn: model::Connection = interface.into();
-                    state.add_connection(conn)?;
+        let interfaces = wicked_read(self.paths.clone())?;
+        let settings = MIGRATION_SETTINGS.get().unwrap();
+
+        if !settings.continue_migration && interfaces.warning.is_some() {
+            return Err(interfaces.warning.unwrap().into());
+        }
+
+        let mut state = NetworkState::new(vec![], vec![]);
+
+        for interface in interfaces.interfaces {
+            let connection_result = interface.to_connection()?;
+            if !connection_result.warnings.is_empty() {
+                for connection_error in &connection_result.warnings {
+                    log::warn!("{}", connection_error);
+                }
+                if !settings.continue_migration {
+                    return Err(anyhow::anyhow!(
+                        "Migration of {} failed",
+                        connection_result.connection.id()
+                    )
+                    .into());
                 }
-                Ok(state)
-            })
-        })
+            }
+            state.add_connection(connection_result.connection)?;
+        }
+        Ok(state)
     }
 
     fn write(&self, _network: &model::NetworkState) -> Result<(), Box<dyn std::error::Error>> {