From: Gabriel Goller <g.goller@proxmox.com>
To: Stefan Hanreich <s.hanreich@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH proxmox-ve-rs 02/11] add proxmox-frr crate with frr types
Date: Tue, 4 Mar 2025 17:28:23 +0100 [thread overview]
Message-ID: <2d5zwigdxnxolimbgxiqtlbvsmu7wgt3fuenhci3qyz6jtjssg@qulezubkzxr5> (raw)
In-Reply-To: <2443e9a1-a494-452b-8840-51c301582aa8@proxmox.com>
On 03.03.2025 17:29, Stefan Hanreich wrote:
>This uses stuff from a later patch, doesn't it? Shouldn't the order of
>patches 2 and 3 be flipped?
oops, yeah mb.
>> [snip]
>> diff --git a/proxmox-frr/src/lib.rs b/proxmox-frr/src/lib.rs
>> new file mode 100644
>> index 000000000000..ceef82999619
>> --- /dev/null
>> +++ b/proxmox-frr/src/lib.rs
>> @@ -0,0 +1,223 @@
>> +pub mod common;
>> +pub mod openfabric;
>> +pub mod ospf;
>> +
>> +use std::{collections::{hash_map::Entry, HashMap}, fmt::Display, str::FromStr};
>> +
>> +use common::{FrrWord, FrrWordError};
>> +use proxmox_ve_config::sdn::fabric::common::Hostname;
>
>Maybe move it to network-types, if it is always needed? Seems like a
>better fit. Especially since the dependency is optional.
Agree.
>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, DeserializeFromStr, SerializeDisplay)]
>> +pub enum RouterName {
>> + OpenFabric(openfabric::OpenFabricRouterName),
>> + Ospf(ospf::OspfRouterName),
>> +}
>> +
>> +impl From<openfabric::OpenFabricRouterName> for RouterName {
>> + fn from(value: openfabric::OpenFabricRouterName) -> Self {
>> + Self::OpenFabric(value)
>> + }
>> +}
>> +
>> +impl Display for RouterName {
>> + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> + match self {
>> + Self::OpenFabric(r) => r.fmt(f),
>> + Self::Ospf(r) => r.fmt(f),
>> + }
>> + }
>> +}
>> +
>> +impl FromStr for RouterName {
>> + type Err = RouterNameError;
>> +
>> + fn from_str(s: &str) -> Result<Self, Self::Err> {
>> + if let Ok(router) = s.parse() {
>> + return Ok(Self::OpenFabric(router));
>> + }
>
>does this make sense here? can we actually make a clear distinction on
>whether this is a OpenFabric / OSPF router name (and for all other
>future RouterNames) from the string alone? I think it might be better to
>explicitly construct the specific type and then make a RouterName out of
>it and do not implement FromStr at all. Or get rid of RouterName
>altogether (see below).
>
>It's also constructing an OpenFabric RouterName but never an OSPF router
>name.
nope, this doesn't make any sense at all, no idea how it got in here.
Removed it and the Deserialize derive for all the parent types as it
isn't needed anyway.
>> + Err(RouterNameError::InvalidName)
>> + }
>> +}
>> +
>> +/// The interface name is the same on ospf and openfabric, but it is an enum so we can have two
>> +/// different entries in the hashmap. This allows us to have an interface in an ospf and openfabric
>> +/// fabric.
>> +#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, Hash)]
>> +pub enum InterfaceName {
>> + OpenFabric(FrrWord),
>> + Ospf(FrrWord),
>> +}
>
>maybe this should be a struct representing a linux interface name
>(nul-terminated 16byte string) instead of an FrrWord?
Makes sense, added a different InterfaceName struct that wraps a String and
checks the length upon creation.
>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Default, Deserialize, Serialize)]
>> +pub struct FrrConfig {
>> + router: HashMap<RouterName, Router>,
>> + interfaces: HashMap<InterfaceName, Interface>,
>
>are we ever querying the frr router/interface by name? judging from the
>public API we don't and only iterate over it. we could move the name
>into the Router/Interface then, this would ensure that the name always
>fits the concrete router. We could probably also make converting from
>the section config easier then and possibly save us the whole RouterName
>struct.
>
>Are duplicates possible with how the ID in the SectionConfig works? If
>we want to avoid that, we could probably use other ways.
The problem here are duplicate interfaces over different fabrics
(areas), which are not allowed.
These are not checked by SectionConfig because the key is:
"<area>_<interface-name>".
We could simply implement checks when inserting, sure, but why not just
use HashMaps and seal the deal? :)
>> +}
>> +
>> +impl FrrConfig {
>> + pub fn new() -> Self {
>> + Self::default()
>> + }
>> +
>> + #[cfg(feature = "config-ext")]
>> + pub fn builder() -> FrrConfigBuilder {
>> + FrrConfigBuilder::default()
>> + }
>
>see above for if we really need a builder here or if implementing
>conversion traits suffices.
My idea is that we can add the bgp stuff here as well (soonTM).
I wanted to avoid a TryFrom<(FabricConfig, BgpConfig, ...)>
implementation to be honest.
>> + pub fn router(&self) -> impl Iterator<Item = (&RouterName, &Router)> + '_ {
>> + self.router.iter()
>> + }
>> +
>> + pub fn interfaces(&self) -> impl Iterator<Item = (&InterfaceName, &Interface)> + '_ {
>> + self.interfaces.iter()
>> + }
>> +}
>> +
>> +#[derive(Default)]
>> +#[cfg(feature = "config-ext")]
>> +pub struct FrrConfigBuilder {
>> + fabrics: FabricConfig,
>> + //bgp: Option<internal::BgpConfig>
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl FrrConfigBuilder {
>> + pub fn add_fabrics(mut self, fabric: FabricConfig) -> FrrConfigBuilder {
>> + self.fabrics = fabric;
>> + self
>> + }
>
>From<FabricConfig> might be better if it replaces self.fabrics /
>consumes FabricConfig anyway? Maybe even TryFrom<FabricConfig> for
>FrrConfig itself?
see above.
>> +
>> + pub fn build(self, current_node: &str) -> Result<FrrConfig, anyhow::Error> {
>> + let mut router: HashMap<RouterName, Router> = HashMap::new();
>> + let mut interfaces: HashMap<InterfaceName, Interface> = HashMap::new();
>> +
>> + if let Some(openfabric) = self.fabrics.openfabric() {
>> + // openfabric
>> + openfabric
>> + .fabrics()
>> + .iter()
>> + .try_for_each(|(fabric_id, fabric_config)| {
>> + let node_config = fabric_config.nodes().get(&Hostname::new(current_node));
>> + if let Some(node_config) = node_config {
>> + let ofr = openfabric::OpenFabricRouter::from((fabric_config, node_config));
>> + let router_item = Router::OpenFabric(ofr);
>> + let router_name = RouterName::OpenFabric(
>> + openfabric::OpenFabricRouterName::try_from(fabric_id)?,
>> + );
>> + router.insert(router_name.clone(), router_item);
>> + node_config.interfaces().try_for_each(|interface| {
>> + let mut openfabric_interface: openfabric::OpenFabricInterface =
>> + (fabric_id, interface).try_into()?;
>
>The only fallible thing here is constructing the RouterName, so we could
>just clone it from above and make a
>OpenFabricInterface::from_section_config() method that accepts the name
>+ sectionconfig structs?
fair – or just have the
TryFrom<(&internal::FabricId,&internal::Interface)> for OpenFabricInterface
be a
TryFrom<(&OpenFabricRouterName, &internal::Interface)> for OpenFabricInterface
imo that's even better.
>> + // If no specific hello_interval is set, get default one from fabric
>> + // config
>> + if openfabric_interface.hello_interval().is_none() {
>> + openfabric_interface
>> + .set_hello_interval(fabric_config.hello_interval().clone());
>> + }
>> + let interface_name = InterfaceName::OpenFabric(FrrWord::from_str(interface.name())?);
>> + // Openfabric doesn't allow an interface to be in multiple openfabric
>> + // fabrics. Frr will just ignore it and take the first one.
>> + if let Entry::Vacant(e) = interfaces.entry(interface_name) {
>> + e.insert(openfabric_interface.into());
>> + } else {
>> + tracing::warn!("An interface cannot be in multiple openfabric fabrics");
>> + }
>
>if let Err(_) = interfaces.try_insert(..) maybe (if we keep the HashMap)?
try_insert is unstable :(
>> + Ok::<(), anyhow::Error>(())
>> + })?;
>> + } else {
>> + tracing::warn!("no node configuration for fabric \"{fabric_id}\" – this fabric is not configured for this node.");
>
>Maybe it would make sense to split this into two functions, where we
>could just return early if there is no configuration for this node?
do you mean one function for ospf and one for openfabric?
I'd agree to that.
>Then here an early return would suffice, since otherwise the log gets
>spammed on nodes that are simply not part of a fabric (which is
>perfectly valid)?
True, did not consider that. I think it's ok if we just make this a
debug print. No need to add some fancy checks imo.
>> [snip]
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct OpenFabricInterface {
>> + // Note: an interface can only be a part of a single fabric (so no vec needed here)
>> + fabric_id: OpenFabricRouterName,
>> + passive: Option<bool>,
>> + hello_interval: Option<openfabric::HelloInterval>,
>> + csnp_interval: Option<openfabric::CsnpInterval>,
>> + hello_multiplier: Option<openfabric::HelloMultiplier>,
>> +}
>> +
>> +impl OpenFabricInterface {
>> + pub fn fabric_id(&self) -> &OpenFabricRouterName {
>> + &self.fabric_id
>> + }
>> + pub fn passive(&self) -> &Option<bool> {
>> + &self.passive
>> + }
>> + pub fn hello_interval(&self) -> &Option<openfabric::HelloInterval> {
>> + &self.hello_interval
>> + }
>> + pub fn csnp_interval(&self) -> &Option<openfabric::CsnpInterval> {
>> + &self.csnp_interval
>> + }
>> + pub fn hello_multiplier(&self) -> &Option<openfabric::HelloMultiplier> {
>> + &self.hello_multiplier
>> + }
>
>If we implement Copy for those types it's usually just easier to return
>them owned.
true
>> + pub fn set_hello_interval(&mut self, interval: Option<openfabric::HelloInterval>) {
>
>nit: I usually like impl Into<Option<..>> because it makes the API nicer
>(don't have to write Some(..) all the time ...)
yep.
>> [snip]
>> +#[cfg(feature = "config-ext")]
>> +impl TryFrom<(&internal::FabricId, &internal::Interface)> for OpenFabricInterface {
>> + type Error = OpenFabricInterfaceError;
>> +
>> + fn try_from(value: (&internal::FabricId, &internal::Interface)) -> Result<Self, Self::Error> {
>> + Ok(Self {
>> + fabric_id: OpenFabricRouterName::try_from(value.0)?,
>> + passive: value.1.passive(),
>> + hello_interval: value.1.hello_interval().clone(),
>> + csnp_interval: value.1.csnp_interval().clone(),
>> + hello_multiplier: value.1.hello_multiplier().clone(),
>
>We can easily implement Copy for those values, since they're u16.
nice
>> + })
>> + }
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl TryFrom<&internal::FabricId> for OpenFabricRouterName {
>> + type Error = RouterNameError;
>> +
>> + fn try_from(value: &internal::FabricId) -> Result<Self, Self::Error> {
>> + Ok(OpenFabricRouterName::new(FrrWord::new(value.to_string())?))
>> + }
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +impl From<(&internal::FabricConfig, &internal::NodeConfig)> for OpenFabricRouter {
>> + fn from(value: (&internal::FabricConfig, &internal::NodeConfig)) -> Self {
>> + Self {
>> + net: value.1.net().to_owned(),
>> + }
>> + }
>> +}
>
>We never use value.0 here, but we might if we have some global options,
>right?
Exactly. Although I've been thinking about that. Maybe we should just
all chuck them in to the Intermediate Representation (and expand the
properties to every node) and don't bother with them in the FrrConfig?
>> [snip]
>> +/// The name of the ospf frr router. There is only one ospf fabric possible in frr (ignoring
>> +/// multiple invocations of the ospfd daemon) and the separation is done with areas. Still,
>> +/// different areas have the same frr router, so the name of the router is just "ospf" in "router
>> +/// ospf". This type still contains the Area so that we can insert it in the Hashmap.
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct OspfRouterName(Area);
>> +
>> +impl From<Area> for OspfRouterName {
>> + fn from(value: Area) -> Self {
>> + Self(value)
>> + }
>> +}
>> +
>> +impl Display for OspfRouterName {
>> + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> + write!(f, "ospf")
>
>this is missing the area, or?
Nope, this is fine, you can't have more than one ospf router except if
your having multiple ospfd daemons. Note that the router-id also
per-daemon, not per-area.
>> [snip]
>> +/// The OSPF Area. Most commonly, this is just a number, e.g. 5, but sometimes also a
>> +/// pseudo-ipaddress, e.g. 0.0.0.0
>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]
>> +pub struct Area(FrrWord);
>> +
>> +impl TryFrom<FrrWord> for Area {
>> + type Error = AreaParsingError;
>> +
>> + fn try_from(value: FrrWord) -> Result<Self, Self::Error> {
>> + Area::new(value)
>> + }
>> +}
>> +
>> +impl Area {
>> + pub fn new(name: FrrWord) -> Result<Self, AreaParsingError> {
>> + if name.as_ref().parse::<i32>().is_ok() || name.as_ref().parse::<Ipv4Addr>().is_ok() {
>
>use u32 here? otherwise area ID can be negative, which isn't allowed afaict?
true.
Thanks for the review!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2025-03-04 16:28 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-14 13:39 [pve-devel] [RFC cluster/manager/network/proxmox{-ve-rs, -perl-rs} 00/11] Add SDN Fabrics Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH proxmox-ve-rs 01/11] add crate with common network types Gabriel Goller
2025-03-03 15:08 ` Stefan Hanreich
2025-03-05 8:28 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH proxmox-ve-rs 02/11] add proxmox-frr crate with frr types Gabriel Goller
2025-03-03 16:29 ` Stefan Hanreich
2025-03-04 16:28 ` Gabriel Goller [this message]
2025-02-14 13:39 ` [pve-devel] [PATCH proxmox-ve-rs 03/11] add intermediate fabric representation Gabriel Goller
2025-02-28 13:57 ` Thomas Lamprecht
2025-02-28 16:19 ` Gabriel Goller
2025-03-04 17:30 ` Gabriel Goller
2025-03-05 9:03 ` Wolfgang Bumiller
2025-03-04 8:45 ` Stefan Hanreich
2025-03-05 9:09 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH proxmox-perl-rs 04/11] fabrics: add CRUD and generate fabrics methods Gabriel Goller
2025-03-04 9:28 ` Stefan Hanreich
2025-03-05 10:20 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-cluster 05/11] cluster: add sdn fabrics config files Gabriel Goller
2025-02-28 12:19 ` Thomas Lamprecht
2025-02-28 12:52 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-network 06/11] add config file and common read/write methods Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-network 07/11] merge the frr config with the fabrics frr config on apply Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-network 08/11] add api endpoints for fabrics Gabriel Goller
2025-03-04 9:51 ` Stefan Hanreich
2025-02-14 13:39 ` [pve-devel] [PATCH pve-manager 09/11] sdn: add Fabrics view Gabriel Goller
2025-03-04 9:57 ` Stefan Hanreich
2025-03-07 15:57 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-manager 10/11] sdn: add fabric edit/delete forms Gabriel Goller
2025-03-04 10:07 ` Stefan Hanreich
2025-03-07 16:04 ` Gabriel Goller
2025-02-14 13:39 ` [pve-devel] [PATCH pve-manager 11/11] network: return loopback interface on network endpoint Gabriel Goller
2025-03-03 16:58 ` [pve-devel] [RFC cluster/manager/network/proxmox{-ve-rs, -perl-rs} 00/11] Add SDN Fabrics Stefan Hanreich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2d5zwigdxnxolimbgxiqtlbvsmu7wgt3fuenhci3qyz6jtjssg@qulezubkzxr5 \
--to=g.goller@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=s.hanreich@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal