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 01:39:48PM +0200, Gabriel Goller wrote:
> On 25.08.2025 10:11, 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.
> 
> Umm do I understand you correctly that you want to have something like
> this:
> 
>     src/
>     ├─ bindings/
>     │  ├─ sdn/
>     │  │  ├─ fabrics.rs
>     ├─ sdn/
>     │  ├─ status.rs
> 
> ?

Yes. The bindings should just be the perl interface and its point is to
provide documentation via rustdoc, and the rustdocs should tell you how
to use it *from perl*.

The rest would be additional code we need to provide the perl interface
for the external crates.

> 
> IMO we could move all the status stuff out to
> crate::bindings::sdn::status. But I don't know about separating all the
> types, conversion methods and actual perl methods -- I'd rather keep all
> the perl-facing stuff in the same file.

You don't need to separate everything. My point was that it does *not*
contain perl *binding* specifics. As for being *perl* specific, I mean,
the entire `pve-rs` crate *is* perl specific right now...


> 
> > > +        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 ;-)
> 
> Oops, this slipped through, should have been somewhere else.
> 
> > > +        #[derive(Debug, Serialize)]
> > > +        pub struct Status {
> > > +            #[serde(rename = "type")]
> > > +            ty: String,
> > > +            status: FabricStatus,
> > > +            protocol: Protocol,
> > > +            sdn: FabricId,
> > > +            sdn_type: String,
> > > +        }
> > > +
> > > +        /// 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
> 
> Yup, I agree, changed all the TryInto impls to functions `get_routes`,
> `get_neighbors` and `get_status`.
> 
> Also fixed all the other stuff below.
> 
> Thanks for the review!


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