Cumulus_nvue: Add OSPFv2 in vrf support

[jbemmel]

• Passes integration/vrf/11-multi-vrf-ospf
• Cannot do integration/vrf/33-vrf-common-ospf yet; add quirk check for missing vrf route leaking

Note: NVUE VRF aware loopback support was added in a previous PR, but not updated in docs yet

[ipspace]

The Cumulus NVUE part looks reasonable (did not try whether it works).

However, I keep wondering why you can't focus on a problem you're supposedly solving and keep throwing in random stuff like the check for the VPNvX address families and VRF-aware BGP. I don't want to waste time trying to figure out whether that change would work (because the existing code works well for all current VRF-enabled platforms) and it definitely does NOT belong into a "CL NVUE OSPFv2 VRF support"

If you keep doing that, I'll just close the PRs with no additional explanation. I'm too old to keep wasting time on the same topic with every second PR you open.

[jbemmel]

The Cumulus NVUE part looks reasonable (did not try whether it works).

However, I keep wondering why you can't focus on a problem you're supposedly solving and keep throwing in random stuff like the check for the VPNvX address families and VRF-aware BGP. I don't want to waste time trying to figure out whether that change would work (because the existing code works well for all current VRF-enabled platforms) and it definitely does NOT belong into a "CL NVUE OSPFv2 VRF support"

If you keep doing that, I'll just close the PRs with no additional explanation. I'm too old to keep wasting time on the same topic with every second PR you open.

Here is how I think about this: I'm adding support for a feature (OSPFv2 in VRFs for Cumulus NVUE) so after I implement that I go through the integration test cases that are related to that. There are 2 (that I could find): integration/vrf/11-multi-vrf-ospf and integration/vrf/33-vrf-common-ospf

Before adding this feature, both fail with "OSPFv2 within VRFs is unsupported". After I added that, both successfully deploy but one works and the other does not. I figured it's because there is still missing functionality related to the vrf.bgp which is set for a bunch of platforms, but never checked (as far as I could tell)

So I add the check such that users still get properly warned when trying topologies requiring vrf.bgp

The problem I am focused on, is to add support for feature XYZ while helping users to understand what is going on where reasonably possible. My vision and ambition for Netlab is to have it be a tool that ultimately helps users understand, not merely generating configs that "just work". We do this by building a strong data model, including feature flags and checks and warnings around those feature flags. Note that I did not invent a new feature flag here - I merely started using the one that is already there

[jbemmel]

I do see your point about submitting code that impacts all platforms, requiring extensive validation. I have moved the check to a device quirk

[ipspace]

I'm adding support for a feature (OSPFv2 in VRFs for Cumulus NVUE) so after I implement that I go through the integration test cases that are related to that. There are 2 (that I could find): integration/vrf/11-multi-vrf-ospf and integration/vrf/33-vrf-common-ospf

OK.

Before adding this feature, both fail with "OSPFv2 within VRFs is unsupported".

Correct.

After I added that, both successfully deploy but one works and the other does not.

Because you did not complete your work. That's why we have integration tests -- I learned this lesson the very hard way when fixing your SR Linux stuff.

In this particular case, VRF route leaking is described in https://docs.nvidia.com/networking-ethernet-software/cumulus-linux-510/Layer-3/VRFs/Virtual-Routing-and-Forwarding-VRF/#vrf-route-leaking. Whether you feel like implementing it or not is a different story (and I don't care either way), but it's a very simple "do it right or document a quirk/caveat" decision.

I figured it's because there is still missing functionality related to the vrf.bgp which is set for a bunch of platforms, but never checked (as far as I could tell)

You figured it wrong (and yes, we should add 'features' description to vrf.yml). The various VRF features specify which in-VRF routing protocols are supported.

The vrf.bgp feature flag is checked in https://github.com/ipspace/netlab/blob/dev/netsim/modules/bgp.py#L659

So I add the check such that users still get properly warned when trying topologies requiring vrf.bgp

In the wrong place (a check, if any, would be useful in validate_vrf_route_leaking) with a confusing error message.

The problem I am focused on, is to add support for feature XYZ while helping users to understand what is going on where reasonably possible.

That is OK, and if you encounter something that you think should be fixed you're most welcome to open an issue and we can figure out how to fix that. Throwing in some random stuff based on incomplete understanding of how things work is a waste of everyone's time. Speaking of that, I would much prefer working on new stuff than arguing with you why you should NOT do things I told you not to do a bunch of times.

My vision and ambition for Netlab is to have it be a tool that ultimately helps users understand, not merely generating configs that "just work".

If a user wants to have inter-VRF route leaking, then the configuration on most platforms will include BGP. There is no other way, and there's nothing the user can or cannot do.

We do this by building a strong data model, including feature flags and checks and warnings around those feature flags.

The ultimate goal is: we should not generate stuff that does not work. Whether that warrants a warning, a feature flag, or a device quirk is an implementation decision, and where one or two platforms deviate from the norm (be it due to NOS limitations or due to netlab developer not implementing all features in a configuration template), a device quirk is the way to go.

Note that I did not invent a new feature flag here - I merely started using the one that is already there

... without understanding why it's there an what it does.