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

[Gabriel Goller <g.goller@proxmox.com>]

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

?

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.

>> +        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