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.gol...@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
