From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Wolfgang Bumiller <w.bumiller@proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox-ve-rs 03/11] add intermediate fabric representation
Date: Wed, 5 Mar 2025 10:03:12 +0100 [thread overview]
Message-ID: <vnvocjrv5755jm2v7pmlim4mqoc4bvlovkesuo6mowslmrwxf5@3yyieobm5q3j> (raw)
In-Reply-To: <5cvfps7jicwcvawb3hgp75276ku3rrj7uiqnlan42cwtppg4qn@j5pbctkkpos6>
On Tue, Mar 04, 2025 at 06:30:50PM +0100, Gabriel Goller wrote:
> On 28.02.2025 14:57, Thomas Lamprecht wrote:
> > Am 14.02.25 um 14:39 schrieb Gabriel Goller:
> > > This adds the intermediate, type-checked fabrics config. This one is
> > > parsed from the SectionConfig and can be converted into the
> > > Frr-Representation.
> >
> > The short description of the patch is good, but I would like to see more
> > rationale here about choosing this way, like benefits and trade-offs to other
> > options that got evaluated, if this can/will be generic for all fabrics planned,
> > ..., and definitively some more rust-documentation for public types and modules.
> >
> > One thing I noticed below, I did not managed to do a thorough review besides
> > of that yet though.
>
> I just spoke again with Stefan and there were some doubts and are
> unsure if this is actually useful or just unnecessary abstraction. Some
> feedback would be very appreciated!
>
> We planned this intermediate config as a layer between the SectionConfig
> (including Vecs, PropertyStrings, etc.) and the FrrConfig, which would
> hold Frr-specific stuff such as routers, interfaces, etc..
>
> The two outer layers, so SectionConfig and FrrConfig, are very specific
> to their respective config files, so they don't look that nice, nor are
> easy to work with.
>
> The intermediate layer acts as a layer above the SectionConfig types
> that:
> * Correct hierarchial representation (e.g.: nodes are stored in fabrics)
> * Enforces invariants and allows to include runtime-checks (e.g.:
> BTreeMap doesn't allow duplicate nodes in fabrics, check that
> router-id is unique).
> * Doesn't use section-config-specific types such as `PropertyString`.
> * Allows us to (eventually) switch config file format bit easier and
> makes proxmox-frr easier to isolate (as an independent lib).
> * Would allow us to eventually separate section-config (stored config)
> and running-config. (Intermediate Config could be parsed out of the
> running-config.)
>
> The Intermediate layer is generally written per-protocol as there are
> protocol-specific attributes that are difficult to generalize and we
> want to use the protocol-specific terminology as well. Nevertheless we
> have common types (mostly the simple ones such as Hostname, Net,
> RouterId), that are stored in the common proxmox-network-types crate and
> get used by all of the layers and protocols.
>
> ┌───────────────┐
> │ SectionConfig │
> └───────┬───────┘
> │
> ┌─────────────────────┼─────────────────────┐
> ▼ ▼ ▼
> ┌──────────────────────────────────────────────────────────┐
> │ │ SectionConfig-specific types
> │ OspfSectionConfig OpenFabricSectionConfig ... │ (proxmox-ve-config::sdn::fabric
> │ │ │ │ │ ::openfabric::OpenFabricSectionConfig)
> └─────────┼─────────────────────┼─────────────────────┼────┘
> │ │ │
> │ │ │
> ┌─────────┼─────────────────────┼─────────────────────┼────┐
> │ ▼ ▼ ▼ │ Intermediate Representation
> │ OspfConfig OpenFabricConfig ... │ (proxmox-ve-config::sdn::fabric
> │ │ │ │ │ ::openfabric::internal::OpenFabricConfig)
> └─────────┼─────────────────────┼─────────────────────┼────┘
> │ │ │
> │ │ │
> ┌─────────┼─────────────────────┼─────────────────────┼───┐
> │ ▼ ▼ ▼ │
> │ OspfRouter OpenFabricRouter ... │ Frr-representation
> │ OspfInterface OpenFabricInterface │ (proxmox-frr::openfabric::OpenFabricRouter)
> │ │
> └─────────────────────────────────────────────────────────┘
>
>
> The other, simpler option would be to parse directly from the
> SectionConfig to the FrrConfig. This would greatly reduce the amount of
> code needed and deduplicate lots of options/parameters. Downside is that
> semantic checking would be hard to do (not that we do a lot there, but
> anyway) and proxmox-frr would be kinda weird including SectionConfig
> types (gated by feature-flags, but still).
So, the question is with or without intermediate types.
If I'm reading this right, currently the intermediate types and
section-config types live in the same crate.
Currently `proxmox-frr` depends on `proxmox-ve-config`.
I think this goes a bit against the idea of potentially separating it,
or at least, if we start off this way, I'm not convinced the task would
be much easier than if we skip the intermediate rep.
For it to be effective for later, the dependency would have to be
reversed: ve-config would depend on frr, frr would *not* depend on
ve-config, for shared types such as `Hostname` we'd need some separate
crate (or frr would copy the portions it needs), and ve-config would
have the code to convert to frr types.
If the extra type layer allows a more lean implementation further down
the stack and can validate some common basic things and get rid of
having to deal with schema stuff, it might still be a win. Encoding
invariants and limitations in the type system usually helps with code
maintenance after all. (And fewer explicit dependencies on the schema
crate will make future bumps less painful...)
Also, I think *dropping* the intermediate layer later would probably be
less work on than *adding* it later.
_______________________________________________
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-05 9:03 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
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 [this message]
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=vnvocjrv5755jm2v7pmlim4mqoc4bvlovkesuo6mowslmrwxf5@3yyieobm5q3j \
--to=w.bumiller@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@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