Re: [pve-devel] [PATCH proxmox-ve-rs 03/11] add intermediate fabric representation

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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