ipspace / netlab

Making virtual networking labs suck less
https://netlab.tools
Other
456 stars 69 forks source link

Revert OS10 template to enable VRRPv3 by default, globally #1496

Closed jbemmel closed 2 weeks ago

jbemmel commented 2 weeks ago

After consideration, I think it's better to keep it simple and default to VRRPv3 across the board

This PR fixes an issue with this - admittedly contrived - topology:

---
module: [gateway]
defaults.device: dellos10
defaults.provider: clab

nodes: [n1,n2]

links:
- n1:
  n2:
  gateway.protocol: anycast # Quirk: only allowed on SVI interfaces, separate issue
  type: lan
- n1:
  n2:
  gateway.protocol: vrrp
  type: lan

loop.first won't work in this case, and vrrp config is skipped. Since the gateway module does a good job cleaning up and only populates the gateway.vrrp property globally when vrrp is used on some interface, it is better and simpler to base the logic on that.

Similarly, the hwaddr filter offers an easier way to format the MAC address

ssasso commented 2 weeks ago

Shouldn't we split this in two? One for reverting the vrrp mess and the other for the code refactoring?

Or at least two different commits on the same pr?

What do you think @ipspace ?

Ps. Please remove the word now from the caveat text, it has always been v3 😉

ipspace commented 2 weeks ago

Shouldn't we split this in two? One for reverting the vrrp mess and the other for the code refactoring?

I'm not that meticulous ;)

Or at least two different commits on the same pr?

PR is merged as a squash merge, so it doesn't matter. From the merging perspective, go for it once you're happy with the PR (and it would be nice to have the above explanation in the commit message)

ssasso commented 2 weeks ago

@jbemmel unfortunately I do not have time to test the code change, so if you are 100% sure it works for the supported case, you can merge it.

(see https://tests.netlab.tools/_html/dellos10-libvirt-gateway )

jbemmel commented 2 weeks ago

Rewrote history to remove "now" and add the comments in the commit message. I tested this with vrnetlab based OS 10 instances

ipspace commented 2 weeks ago

In principle, the template is wrong as it does not allow you to configure anycast MAC per interface. For example, your template would configure incorrect anycast MAC (the default anycast MAC, not the one specified on the interface)

module: [ gateway ]
defaults.device: frr

nodes:
 n1:
 n2:

links:
- n1:
  n2:
  gateway.protocol: anycast
  gateway.anycast.mac: aaaa.0000.0001
  type: lan

It could be that OS10 allows a single anycast MAC per device (wouldn't be the only platform with that limitation), but then we have to catch the per-interface instances and report errors. I'm also not saying the previous template was correct ;)

jbemmel commented 2 weeks ago

In principle, the template is wrong as it does not allow you to configure anycast MAC per interface. For example, your template would configure incorrect anycast MAC (the default anycast MAC, not the one specified on the interface)

module: [ gateway ]
defaults.device: frr

nodes:
 n1:
 n2:

links:
- n1:
  n2:
  gateway.protocol: anycast
  gateway.anycast.mac: aaaa.0000.0001
  type: lan

It could be that OS10 allows a single anycast MAC per device (wouldn't be the only platform with that limitation), but then we have to catch the per-interface instances and report errors. I'm also not saying the previous template was correct ;)

Yes, OS10 only allows a single global virtual MAC: https://www.dell.com/support/manuals/en-gh/dell-emc-smartfabric-os10/vxlan-evpn-ug-10-5-1-pub/ip-virtual-router-mac-address?guid=guid-4284db0b-4c89-419c-9f8b-441730cd12ea&lang=en-us

We could add a feature flag anycast.per-interface-virtual-mac?

ipspace commented 2 weeks ago

We could add a feature flag anycast.per-interface-virtual-mac?

Considering it's not a single-platform quirk (I vaguely remember seeing it somewhere else as well), it would definitely make sense, but maybe less verbose ;) and without dashes?