From: Gabriel Goller <g.goller@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH proxmox-ve-rs v4 17/22] config: sdn: fabrics: add fabric config
Date: Fri, 11 Jul 2025 11:04:50 +0200 [thread overview]
Message-ID: <2wlpflcuiqpat37zvzgkb6tpg6boemdzjkbjb74llsnsvj5oci@vrukwot5ovlu> (raw)
In-Reply-To: <vzywmli7o2rm53ijdqzl6zhdp455ssgqbhca3icu3njmpyu3ly@xoff4vvmzg55>
>> [snip]
>> diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs b/proxmox-ve-config/src/sdn/fabric/mod.rs
>> index 007be6a3fd8e..3342a7053d3f 100644
>> --- a/proxmox-ve-config/src/sdn/fabric/mod.rs
>> +++ b/proxmox-ve-config/src/sdn/fabric/mod.rs
>> @@ -1 +1,517 @@
>> pub mod section_config;
>> +
>> +use std::collections::BTreeMap;
>> +use std::marker::PhantomData;
>> +use std::ops::Deref;
>> +
>> +use serde::{Deserialize, Serialize};
>> +
>> +use crate::sdn::fabric::section_config::{
>> + fabric::{
>> + Fabric, FabricDeletableProperties, FabricId, FabricSection, FabricSectionUpdater,
>> + FabricUpdater,
>> + },
>> + node::{
>> + api::{NodeDataUpdater, NodeDeletableProperties, NodeUpdater},
>> + Node, NodeId, NodeSection,
>> + },
>> + protocol::{
>> + openfabric::{
>> + OpenfabricDeletableProperties, OpenfabricNodeDeletableProperties,
>> + OpenfabricNodeProperties, OpenfabricNodePropertiesUpdater, OpenfabricProperties,
>> + OpenfabricPropertiesUpdater,
>> + },
>> + ospf::{
>> + OspfDeletableProperties, OspfNodeDeletableProperties, OspfNodeProperties,
>> + OspfNodePropertiesUpdater, OspfProperties, OspfPropertiesUpdater,
>> + },
>> + },
>> +};
>> +
>> +#[derive(thiserror::Error, Debug)]
>> +pub enum FabricConfigError {
>> + #[error("fabric '{0}' does not exist in configuration")]
>> + FabricDoesNotExist(String),
>> + #[error("node '{0}' does not exist in fabric '{1}'")]
>> + NodeDoesNotExist(String, String),
>
>^ So, in the "usual" cases, `.get()` methods return an `Option`. Where
>this is not the case, we often have an empty error.
>Eg.: `io::ErrorKind::NotFound` has no content, `std::num::ParseIntError`
>does not include the number/digits.
>The names are usually added via `.context()` or some such.
>On the other hand, sometimes there are "functional" errors, like
>`NulError` in `CString`, since it takes an `Into<Vec<u8>>` it includes
>the allocated vector in case the user wants to reuse it in the error
>case. Similarly, `Mutex::lock()`'s error is a
>`PoisonError<MutexGuard<T>>` which can be turned into the guard to still
>access the data...
>
>The use of this below may as well just return an `Option`, but I get
>that we want more context, pretty much all cases. I'm just worried we
>might end up with more inconsistencies across our various code bases
>(callers adding context because they did not expect it to already be
>there), so we should probably declare some rules for how to design error
>types in our coding style at some point... (This is long overdue!)
>
>Anyway... until we actually define how we want to deal with error types,
>we can leave it like this for now...
Yeah I know this is kind of weird. My intention was here to 'force' the
user to add some context to the error. The problem IMO with `.context`
is that sometimes it's forgotten and then you just get e.g. `fabric doesn't
exist`.
>> + #[error("node has a different protocol than the referenced fabric")]
>> + ProtocolMismatch,
>> + #[error("fabric '{0}' already exists in config")]
>> + DuplicateFabric(String),
>> + #[error("node '{0}' already exists in config for fabric {1}")]
>> + DuplicateNode(String, String),
>> + // should usually not occur, but we still check for it nonetheless
>> + #[error("mismatched fabric_id")]
>> + FabricIdMismatch,
>> +}
>> +
>> +/// An entry in a [`FabricConfig`].
>> +///
>> +/// It enforces compatible types for its containing [`FabricSection`] and [`NodeSection`] via the
>> +/// generic parameters, so only Nodes and Fabrics with compatible types can be inserted into an
>> +/// entry.
>> +#[derive(Debug, Clone, Serialize, Deserialize, Hash)]
>> +pub struct Entry<F, N> {
>> + // we want to store the enum structs Fabric & Node here, in order to have access to the
>> + // properties and methods defined on the enum itself.
>> + // In order to still be able to type-check that an Entry contains the right combination of
>> + // NodeSection and FabricSection, we type hint the actual types wrapped into Fabric & Node here
>> + // via PhantomData and only allow insertion of the proper types via the provided methods.
>> + #[serde(skip)]
>> + _phantom_fabric: PhantomData<FabricSection<F>>,
>> + #[serde(skip)]
>> + _phantom_node: PhantomData<NodeSection<N>>,
>
>Technically, since this struct *contains* neither N nor F, this could
>use `PhantomData<fn() -> FooSection<X>>`. Here this probably won't make
>a difference. In general, though, the way you use the generic type of
>`PhantomData` influences auto-traits (Send/Sync), variance and
>dropchecking.
>
>Since this struct's Send/Sync don't depend on neither N nor F, the
>`fn() -> …` version would be suitable.
>
>See https://doc.rust-lang.org/nomicon/phantom-data.html
Ooh, good point!
Variance stays the same and dropchecking is irrelevant afaict because we don't store the F and N.
>> +
>> + fabric: Fabric,
>> + nodes: BTreeMap<NodeId, Node>,
>> +}
>> +
>> +impl<F, N> Entry<F, N>
>> +where
>> + Fabric: From<FabricSection<F>>,
>> + Node: From<NodeSection<N>>,
>> +{
>> + /// Create a new [`Entry`] from the passed [`FabricSection<F>`] with no nodes.
>> + fn new(fabric: FabricSection<F>) -> Self {
>> + Self {
>> + fabric: fabric.into(),
>> + nodes: Default::default(),
>> + _phantom_fabric: Default::default(),
>> + _phantom_node: Default::default(),
>
>^ `PhantomData` is shorter, const and more obvious than `Default::default()`.
Agree.
>> + }
>> + }
>> +
>> + /// Adds a node to this entry
>> + ///
>> + /// # Errors
>
>Please include a newline here.
Done.
>> + /// Returns an error if the node's fabric_id doesn't match this entry's fabric_id
>> + /// or if a node with the same ID already exists in this entry.
>> + fn add_node(&mut self, node: NodeSection<N>) -> Result<(), FabricConfigError> {
>> + if self.nodes.contains_key(node.id().node_id()) {
>> + return Err(FabricConfigError::DuplicateNode(
>> + node.id().node_id().to_string(),
>> + self.fabric.id().to_string(),
>> + ));
>> + }
>> +
>> + if node.id().fabric_id() != self.fabric.id() {
>> + return Err(FabricConfigError::FabricIdMismatch);
>> + }
>> +
>> + self.nodes.insert(node.id().node_id().clone(), node.into());
>> +
>> + Ok(())
>> + }
>> +
>> [snip]
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-07-11 9:04 UTC|newest]
Thread overview: 129+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-02 14:49 [pve-devel] [PATCH access-control/cluster/docs/gui-tests/manager/network/proxmox{, -firewall, -ve-rs, -perl-rs, -widget-toolkit} v4 00/76] Add SDN Fabrics Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox v4 1/5] network-types: initial commit Gabriel Goller
2025-07-03 13:11 ` Wolfgang Bumiller
2025-07-03 13:46 ` Stefan Hanreich
2025-07-03 13:48 ` Stefan Hanreich
2025-07-04 7:55 ` Wolfgang Bumiller
2025-07-04 12:19 ` Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox v4 2/5] network-types: make cidr and mac-address types usable by the api Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox v4 3/5] network-types: add api types for ipv4/6 Gabriel Goller
2025-07-03 13:18 ` Wolfgang Bumiller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox v4 4/5] network-types: add CIDR overlap detection for IPv4 and IPv6 Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox v4 5/5] api-macro: add allof schema to enum Gabriel Goller
2025-07-04 13:48 ` [pve-devel] applied: " Wolfgang Bumiller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-firewall v4 1/1] firewall: nftables: migrate to proxmox-network-types Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 01/22] config: use proxmox_serde perl helpers Gabriel Goller
2025-07-04 14:09 ` [pve-devel] applied: " Wolfgang Bumiller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 02/22] ve-config: move types to proxmox-network-types Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 03/22] sdn-types: initial commit Gabriel Goller
2025-07-04 14:09 ` Wolfgang Bumiller
2025-07-04 14:40 ` Gabriel Goller
2025-07-07 7:53 ` Wolfgang Bumiller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 04/22] frr: create proxmox-frr crate Gabriel Goller
2025-07-07 11:12 ` Wolfgang Bumiller
2025-07-07 12:52 ` Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 05/22] frr: add common frr types Gabriel Goller
2025-07-07 11:18 ` Wolfgang Bumiller
2025-07-07 14:43 ` Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 06/22] frr: add openfabric types Gabriel Goller
2025-07-07 11:25 ` Wolfgang Bumiller
2025-07-07 15:19 ` Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 07/22] frr: add ospf types Gabriel Goller
2025-07-07 11:28 ` Wolfgang Bumiller
2025-07-07 17:07 ` Gabriel Goller
2025-07-02 14:49 ` [pve-devel] [PATCH proxmox-ve-rs v4 08/22] frr: add route-map types Gabriel Goller
2025-07-07 11:52 ` Wolfgang Bumiller
2025-07-07 17:13 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 09/22] frr: add generic types over openfabric and ospf Gabriel Goller
2025-07-07 11:58 ` Wolfgang Bumiller
2025-07-07 17:20 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 10/22] config: sdn: fabrics: add section types Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 11/22] config: sdn: fabrics: add node " Gabriel Goller
2025-07-07 12:19 ` Wolfgang Bumiller
2025-07-07 17:22 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 12/22] config: sdn: fabrics: add interface name struct Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 13/22] config: sdn: fabrics: add openfabric properties Gabriel Goller
2025-07-08 8:09 ` Wolfgang Bumiller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 14/22] config: sdn: fabrics: add ospf properties Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 15/22] config: sdn: fabrics: add api types Gabriel Goller
2025-07-08 8:15 ` Wolfgang Bumiller
2025-07-08 9:50 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 16/22] config: sdn: fabrics: add section config Gabriel Goller
2025-07-08 8:18 ` Wolfgang Bumiller
2025-07-11 8:33 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 17/22] config: sdn: fabrics: add fabric config Gabriel Goller
2025-07-08 8:46 ` Wolfgang Bumiller
2025-07-11 9:04 ` Gabriel Goller [this message]
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 18/22] common: sdn: fabrics: implement validation Gabriel Goller
2025-07-08 11:01 ` Wolfgang Bumiller
2025-07-11 9:40 ` Gabriel Goller
2025-07-11 11:38 ` Wolfgang Bumiller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 19/22] sdn: fabrics: config: add conversion from / to section config Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 20/22] sdn: fabrics: implement FRR configuration generation Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 21/22] ve-config: add integrations tests Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-ve-rs v4 22/22] frr: add global ipv6 forwarding Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-perl-rs v4 1/5] pve-rs: Add PVE::RS::SDN::Fabrics module Gabriel Goller
2025-07-04 12:16 ` Wolfgang Bumiller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-perl-rs v4 2/5] pve-rs: sdn: fabrics: add api methods Gabriel Goller
2025-07-04 12:57 ` Wolfgang Bumiller
2025-07-04 15:56 ` Gabriel Goller
2025-07-07 7:48 ` Wolfgang Bumiller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-perl-rs v4 3/5] pve-rs: sdn: fabrics: add frr config generation Gabriel Goller
2025-07-04 13:14 ` Wolfgang Bumiller
2025-07-04 13:23 ` Stefan Hanreich
2025-07-04 13:31 ` Wolfgang Bumiller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-perl-rs v4 4/5] pve-rs: sdn: fabrics: add helper to generate ifupdown2 configuration Gabriel Goller
2025-07-04 13:29 ` Wolfgang Bumiller
2025-07-04 16:15 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-perl-rs v4 5/5] pve-rs: sdn: fabrics: add helper for network API endpoint Gabriel Goller
2025-07-04 13:34 ` Wolfgang Bumiller
2025-07-04 16:31 ` Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-cluster v4 1/1] cfs: add fabrics.cfg to observed files Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-access-control v4 1/1] permissions: add ACL paths for SDN fabrics Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 01/21] sdn: fix value returned by pending_config Gabriel Goller
2025-07-04 14:29 ` Stefan Hanreich
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 02/21] debian: add dependency to proxmox-perl-rs Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 03/21] fabrics: add fabrics module Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 04/21] refactor: controller: move frr methods into helper Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 05/21] frr: add new helpers for reloading frr configuration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 06/21] controllers: define new api for frr config generation Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 07/21] sdn: add frr config generation helpers Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 08/21] sdn: api: add check for rewriting frr configuration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 09/21] test: isis: add test for standalone configuration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 10/21] sdn: frr: add daemon status to frr helper Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 11/21] sdn: commit fabrics config to running configuration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 12/21] fabrics: generate ifupdown configuration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 13/21] fabrics: add jsonschema for fabrics and nodes Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 14/21] api: fabrics: add root-level module Gabriel Goller
2025-07-04 14:32 ` Stefan Hanreich
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 15/21] api: fabrics: add fabric submodule Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 16/21] api: fabrics: add node submodule Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 17/21] api: fabrics: add fabricnode submodule Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 18/21] controller: evpn: add fabrics integration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 19/21] zone: vxlan: " Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 20/21] test: fabrics: add test cases for ospf and openfabric + evpn Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-network v4 21/21] frr: bump frr config version to 10.3.1 Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH proxmox-widget-toolkit v4 1/1] network selector: add type parameter Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 01/17] api: use new sdn config generation functions Gabriel Goller
2025-07-04 14:27 ` Stefan Hanreich
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 02/17] ui: fabrics: add model definitions for fabrics Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 03/17] fabric: add common interface panel Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 04/17] fabric: add OpenFabric interface properties Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 05/17] fabric: add OSPF " Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 06/17] fabric: add generic node edit panel Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 07/17] fabric: add OpenFabric node edit Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 08/17] fabric: add OSPF " Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 09/17] fabric: add generic fabric edit panel Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 10/17] fabric: add OpenFabric " Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 11/17] fabric: add OSPF " Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 12/17] fabrics: Add main FabricView Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 13/17] ui: permissions: add ACL path for fabrics Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 14/17] api: network: add include_sdn / fabric type Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 15/17] ui: add sdn networks to ceph / migration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 16/17] ui: sdn: add evpn controller fabric integration Gabriel Goller
2025-07-02 14:50 ` [pve-devel] [PATCH pve-manager v4 17/17] ui: sdn: vxlan: add fabric property Gabriel Goller
2025-07-02 14:51 ` [pve-devel] [PATCH pve-gui-tests v4 1/1] pve: add sdn/fabrics screenshots Gabriel Goller
2025-07-02 14:51 ` [pve-devel] [PATCH pve-docs v4 1/1] fabrics: add initial documentation for sdn fabrics Gabriel Goller
2025-07-03 9:57 ` [pve-devel] [PATCH access-control/cluster/docs/gui-tests/manager/network/proxmox{, -firewall, -ve-rs, -perl-rs, -widget-toolkit} v4 00/76] Add SDN Fabrics Gabriel Goller
2025-07-09 11:27 ` Gabriel Goller
2025-07-16 13:09 ` Gabriel Goller
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=2wlpflcuiqpat37zvzgkb6tpg6boemdzjkbjb74llsnsvj5oci@vrukwot5ovlu \
--to=g.goller@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=w.bumiller@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