Re: [pve-devel] [PATCH proxmox-perl-rs v2 2/4] fabrics: add function to get status of fabric

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Aug 25, 2025 at 10:11:01AM +0200, Wolfgang Bumiller wrote:
> On Fri, Aug 22, 2025 at 11:00:36AM +0200, Gabriel Goller wrote:
> > Add a function to get the status of a fabric. This is the status which
> > will then be inserted into the pvestatd daemon and returned through the
> > resources api. In order the generate the HashMap of statuses for all
> > fabrics we need to read the fabric config and execute a vtysh (frr)
> > command to get the routes of the corresponding fabric. If there is at
> > least one route which is related to the fabric, the fabric is considered
> > "ok".
> > 
> > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> > ---
> >  pve-rs/src/bindings/sdn/fabrics.rs | 194 +++++++++++++++++++++++++++++
> >  1 file changed, 194 insertions(+)
> > 
> > diff --git a/pve-rs/src/bindings/sdn/fabrics.rs b/pve-rs/src/bindings/sdn/fabrics.rs
> > index 1dc8bf4320e6..3f70d421e582 100644
> > --- a/pve-rs/src/bindings/sdn/fabrics.rs
> > +++ b/pve-rs/src/bindings/sdn/fabrics.rs
> > @@ -9,8 +9,10 @@ pub mod pve_rs_sdn_fabrics {
> >      use std::fmt::Write;
> >      use std::net::IpAddr;
> >      use std::ops::Deref;
> > +    use std::process::Command;
> >      use std::sync::Mutex;
> >  
> > +    use anyhow::Context;
> >      use anyhow::Error;
> >      use openssl::hash::{MessageDigest, hash};
> >      use serde::{Deserialize, Serialize};
> > @@ -578,4 +580,196 @@ pub mod pve_rs_sdn_fabrics {
> >  
> >          Ok(interfaces)
> >      }
> > +
> > +    /// This module contains status-related structs that represent Routes and Neighbors for all
> > +    /// protocols
> > +    pub mod status {
> 
> ^ This seems to be a helper module which does not contain any
> perlmod/perl specifics.
> I'd argue it's time to start a `crate::sdn` module outside of the
> `bindings` submodule for this.
> 
> The `bindings` module should become rather more lean in the future and
> focus mostly on the perl/rust interaction.
> 
> > +        use std::collections::{HashMap, HashSet};
> > +
> > +        use serde::Serialize;
> > +
> > +        use proxmox_frr::de::{self};
> > +        use proxmox_ve_config::sdn::fabric::{
> > +            FabricConfig,
> > +            section_config::{fabric::FabricId, node::Node as ConfigNode},
> > +        };
> > +
> > +        /// Protocol
> > +        #[derive(Debug, Serialize, Clone, Copy)]
> > +        pub enum Protocol {
> > +            /// Openfabric
> > +            Openfabric,
> > +            /// OSPF
> > +            Ospf,
> > +        }
> > +
> > +        /// The status of a fabric.
> > +        #[derive(Debug, Serialize)]
> > +        pub enum FabricStatus {
> > +            /// The fabric exists and has a route
> > +            #[serde(rename = "ok")]
> > +            Ok,
> > +            /// The fabric does not exist or doesn't distribute any routes
> > +            #[serde(rename = "not ok")]
> > +            NotOk,
> > +        }
> > +
> > +        /// Status of a fabric.
> > +        ///
> > +        /// Check if there are any routes, if yes, then the status is ok, otherwise not ok.
> 
> ^ Not sure how this describes the *struct*, though ;-)
> 
> > +        #[derive(Debug, Serialize)]
> > +        pub struct Status {
> > +            #[serde(rename = "type")]
> > +            ty: String,
> > +            status: FabricStatus,
> > +            protocol: Protocol,
> > +            sdn: FabricId,
> > +            sdn_type: String,

^ If we already have serialization helpers, this could be an enum to
avoid some more string allocations (or use `&'static str`, but an enum
is safer...)

> > +        }
> > +
> > +        /// Parsed routes for all protocols
> > +        ///
> > +        /// These are the routes parsed from the json output of:
> > +        /// `vtysh -c 'show ip route <protocol> json'`.
> > +        #[derive(Debug, Serialize)]
> > +        pub struct RoutesParsed {
> > +            /// All openfabric routes in FRR
> > +            pub openfabric: de::Routes,
> > +            /// All ospf routes in FRR
> > +            pub ospf: de::Routes,
> > +        }
> > +
> > +        impl TryInto<HashMap<FabricId, Status>> for RoutesParsed {
> > +            type Error = anyhow::Error;
> > +
> > +            fn try_into(self) -> Result<HashMap<FabricId, Status>, Self::Error> {
> > +                let hostname = proxmox_sys::nodename();
> > +
> > +                // to associate a route to a fabric, we get all the interfaces which are associated
> > +                // with a fabric on this node and compare them with the interfaces on the route.
> > +                let raw_config = std::fs::read_to_string("/etc/pve/sdn/fabrics.cfg")?;
> 
> ^ I'm really not a fan of doing file I/O in a TryInto implementation.
> These are still supposed to be "simple"[1].
> 
> Better make this a method.
> 
> [1] https://doc.rust-lang.org/std/convert/trait.TryFrom.html
> 
> > +                let config = FabricConfig::parse_section_config(&raw_config)?;
> > +
> > +                let mut stats: HashMap<FabricId, Status> = HashMap::new();
> > +
> > +                for (nodeid, node) in config.values().flat_map(|entry| {
> > +                    entry
> > +                        .nodes()
> > +                        .map(|(id, node)| (id.to_string(), node.clone()))
> 
> ^ Unnecessary `.to_string()` and `.clone()` - the entire `.map()`
> line can be dropped if use `if nodeid.as_str() != hostname` below.
> 
> > +                }) {
> > +                    if nodeid != hostname {
> > +                        continue;
> > +                    }
> > +                    let fabric_id = node.id().fabric_id().clone();

^ same as in patch 3

> > +
> > +                    let current_protocol = match &node {
> > +                        ConfigNode::Openfabric(_) => Protocol::Openfabric,
> > +                        ConfigNode::Ospf(_) => Protocol::Ospf,
> > +                    };
> > +
> > +                    let mut all_routes = HashMap::new();

^ same as in patch 3

> > +                    match &node {
> > +                        ConfigNode::Openfabric(_) => all_routes.extend(&self.openfabric.0),
> > +                        ConfigNode::Ospf(_) => all_routes.extend(&self.ospf.0),
> > +                    }
> > +
> > +                    // get interfaces
> > +                    let interface_names: HashSet<String> = match node {
> 
> This also doesn't need to allocate the string - use &str
> 
> > +                        ConfigNode::Openfabric(n) => n
> > +                            .properties()
> > +                            .interfaces()
> > +                            .map(|i| i.name().to_string())
> 
> ^ with .as_str() here
> 
> > +                            .collect(),
> > +                        ConfigNode::Ospf(n) => n
> > +                            .properties()
> > +                            .interfaces()
> > +                            .map(|i| i.name().to_string())
> 
> ^ and here
> 
> > +                            .collect(),
> > +                    };
> > +
> > +                    // determine status by checking if any routes exist for our interfaces
> > +                    let has_routes = all_routes.iter().any(|(_, v)| {
> 
> ^ Could use
> 
>     .values().any(|v| {
> 
> (no need to `_` the key)
> 
> > +                        v.iter().any(|route| {
> > +                            route
> > +                                .nexthops
> > +                                .iter()
> > +                                .any(|nexthop| interface_names.contains(&nexthop.interface_name))
> 
> and on .interface_name here
> 
> > +                        })
> > +                    });
> > +
> > +                    let fabric = Status {
> > +                        ty: "sdn".to_owned(),
> > +                        status: if has_routes {
> > +                            FabricStatus::Ok
> > +                        } else {
> > +                            FabricStatus::NotOk
> > +                        },
> > +                        sdn_type: "fabric".to_string(),
> > +                        protocol: current_protocol,
> > +                        sdn: fabric_id.clone(),
> > +                    };
> > +                    stats.insert(fabric_id, fabric);
> > +                }
> > +
> > +                Ok(stats)
> > +            }
> > +        }
> > +    }
> > +
> > +    /// Return the status of all fabrics on this node.
> > +    ///
> > +    /// Go through all fabrics in the config, then filter out the ones that exist on this node.
> > +    /// Check if there are any routes in the routing table that use the interface specified in the
> > +    /// config. If there are, show "ok" as status, otherwise "not ok".
> > +    #[export]
> > +    fn status() -> Result<HashMap<FabricId, status::Status>, Error> {
> > +        let openfabric_ipv4_routes_string = String::from_utf8(
> > +            Command::new("sh")
> > +                .args(["-c", "vtysh -c 'show ip route openfabric json'"])
> > +                .output()?
> > +                .stdout,
> > +        )?;
> > +
> > +        let openfabric_ipv6_routes_string = String::from_utf8(
> > +            Command::new("sh")
> > +                .args(["-c", "vtysh -c 'show ipv6 route openfabric json'"])
> > +                .output()?
> > +                .stdout,
> > +        )?;
> > +
> > +        let ospf_routes_string = String::from_utf8(
> > +            Command::new("sh")
> > +                .args(["-c", "vtysh -c 'show ip route ospf json'"])
> > +                .output()?
> > +                .stdout,
> > +        )?;
> > +
> > +        let mut openfabric_routes: proxmox_frr::de::Routes =
> > +            if openfabric_ipv4_routes_string.is_empty() {
> > +                proxmox_frr::de::Routes::default()
> > +            } else {
> > +                serde_json::from_str(&openfabric_ipv4_routes_string)
> > +                    .with_context(|| "error parsing openfabric ipv4 routes")?
> > +            };
> > +        if !openfabric_ipv6_routes_string.is_empty() {
> > +            let openfabric_ipv6_routes: proxmox_frr::de::Routes =
> > +                serde_json::from_str(&openfabric_ipv6_routes_string)
> > +                    .with_context(|| "error parsing openfabric ipv6 routes")?;
> > +            openfabric_routes.0.extend(openfabric_ipv6_routes.0);
> > +        }
> > +
> > +        let ospf_routes: proxmox_frr::de::Routes = if ospf_routes_string.is_empty() {
> > +            proxmox_frr::de::Routes::default()
> > +        } else {
> > +            serde_json::from_str(&ospf_routes_string)
> > +                .with_context(|| "error parsing ospf routes")?
> > +        };
> > +
> > +        let route_status = status::RoutesParsed {
> > +            openfabric: openfabric_routes,
> > +            ospf: ospf_routes,
> > +        };
> > +
> > +        route_status.try_into()
> > +    }
> >  }
> > -- 
> > 2.47.2


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel