From 2415d30c0a380041c313180764e7830356efde05 Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Sun, 27 Mar 2022 20:39:19 +0000 Subject: [PATCH] Second part of a BVI refactor The handling of BVI is awkward, with the autoderived interface name "bviXX" based on the bridgedomain bd_id. Lots of special casing happens on account of this decision, and to make matters worse there is poor interaction (leading to VPP crashes) when BVIs and Loopbacks are used at the same time: https://lists.fd.io/g/vpp-dev/message/21116 In this commit, I reintroduce the ability to set bridgedomain virtual interfaces by means of the 'bvi' keyword. The 'bvi' must: - be a Loopback interface - must be used at most once (bvi_unique()) When pruning, I now need to prune bridgedomains before pruning loopbacks, because any given loopback might be a BVI for a bridge. So, I'll remove the loop/BVI from the bridge (by setting it to L3) and only then removing the loopback from VPP. In the reconciler, remove BVIs that have changed in prune_bridgedomains() and set it in sync_bridgedomains(). --- config/bridgedomain.py | 20 ++++++++++++++++++++ config/test_bridgedomain.py | 7 ++++++- schema.yaml | 1 + unittest/test_bridgedomain.yaml | 16 ++++++++++++++++ unittest/yaml/correct-bridgedomain.yaml | 6 ++++++ unittest/yaml/error-bridgedomain5.yaml | 18 ++++++++++++++++++ unittest/yaml/error-bridgedomain8.yaml | 11 +++++++++++ vpp/reconciler.py | 22 +++++++++++++++++++--- 8 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 unittest/yaml/error-bridgedomain5.yaml create mode 100644 unittest/yaml/error-bridgedomain8.yaml diff --git a/config/bridgedomain.py b/config/bridgedomain.py index ad3de89..b860257 100644 --- a/config/bridgedomain.py +++ b/config/bridgedomain.py @@ -13,6 +13,7 @@ # import logging import config.interface as interface +import config.loopback as loopback import config.lcp as lcp import config.address as address @@ -69,6 +70,17 @@ def is_bridge_interface(yaml, ifname): return ifname in get_bridge_interfaces(yaml) +def bvi_unique(yaml, bviname): + """ Returns True if the BVI identified by bviname is unique among all BridgeDomains. """ + if not 'bridgedomains' in yaml: + return True + n = 0 + for ifname, iface in yaml['bridgedomains'].items(): + if 'bvi' in iface and iface['bvi'] == bviname: + n += 1 + return n<2 + + def validate_bridgedomains(yaml): result = True msgs = [] @@ -87,6 +99,14 @@ def validate_bridgedomains(yaml): if instance == 0: msgs.append("bridgedomain %s is reserved" % ifname) result = False + if 'bvi' in iface: + bviname = iface['bvi'] + if (None,None) == loopback.get_by_name(yaml, bviname): + msgs.append("bridgedomain %s BVI %s does not exist" % (ifname, bviname)) + result = False + if not bvi_unique(yaml, bviname): + msgs.append("bridgedomain %s BVI %s is not unique" % (ifname, bviname)) + result = False if 'interfaces' in iface: for member in iface['interfaces']: diff --git a/config/test_bridgedomain.py b/config/test_bridgedomain.py index 2c93a43..d32e28e 100644 --- a/config/test_bridgedomain.py +++ b/config/test_bridgedomain.py @@ -41,6 +41,11 @@ class TestBridgeDomainMethods(unittest.TestCase): self.assertIn("GigabitEthernet1/0/0", ifs) self.assertIn("GigabitEthernet2/0/0.100", ifs) + def test_bvi_unique(self): + self.assertTrue(bridgedomain.bvi_unique(self.cfg, "loop0")) + self.assertFalse(bridgedomain.bvi_unique(self.cfg, "loop1")) + self.assertTrue(bridgedomain.bvi_unique(self.cfg, "loop2")) + def test_get_bridgedomains(self): ifs = bridgedomain.get_bridgedomains(self.cfg) - self.assertEqual(len(ifs), 3) + self.assertEqual(len(ifs), 6) diff --git a/schema.yaml b/schema.yaml index d2f6f04..254f27d 100644 --- a/schema.yaml +++ b/schema.yaml @@ -13,6 +13,7 @@ vxlan: bridgedomain: description: str(exclude='\'"',len=64,required=False) mtu: int(min=128,max=9216,required=False) + bvi: str(matches='loop[0-9]+',required=False) interfaces: list(str(),required=False) --- loopback: diff --git a/unittest/test_bridgedomain.yaml b/unittest/test_bridgedomain.yaml index a5700f4..84e2493 100644 --- a/unittest/test_bridgedomain.yaml +++ b/unittest/test_bridgedomain.yaml @@ -38,10 +38,17 @@ interfaces: 100: mtu: 2000 +loopbacks: + loop0: + description: "BVI for bd10" + loop1: + description: "BVI for bd13 and bd14" + bridgedomains: bd10: description: "Bridge Domain 10" mtu: 3000 + bvi: loop0 interfaces: [ GigabitEthernet1/0/0, GigabitEthernet1/0/1, BondEthernet0 ] bd11: description: "Bridge Domain 11, with sub-interfaces" @@ -51,3 +58,12 @@ bridgedomains: description: "Bridge Domain 12, invalid because it has Gi1/0/0 as well" mtu: 9000 interfaces: [ GigabitEthernet4/0/0, GigabitEthernet1/0/0 ] + bd13: + description: "Bridge Domain 13 and 14 cannot have the same BVI" + bvi: loop1 + bd14: + description: "Bridge Domain 13 and 14 cannot have the same BVI" + bvi: loop1 + bd15: + description: "Bridge Domain 15 has a non-existant BVI" + bvi: loop2 diff --git a/unittest/yaml/correct-bridgedomain.yaml b/unittest/yaml/correct-bridgedomain.yaml index 3b5335f..335ada5 100644 --- a/unittest/yaml/correct-bridgedomain.yaml +++ b/unittest/yaml/correct-bridgedomain.yaml @@ -40,10 +40,16 @@ interfaces: 100: mtu: 2000 +loopbacks: + loop0: + lcp: "bvi0" + addresses: [ 192.0.2.1/29, 2001:db8:1::1/64 ] + bridgedomains: bd10: description: "Bridge Domain 10" mtu: 3000 + bvi: loop0 interfaces: [ GigabitEthernet1/0/0, GigabitEthernet1/0/1, BondEthernet0 ] bd11: description: "Bridge Domain 11" diff --git a/unittest/yaml/error-bridgedomain5.yaml b/unittest/yaml/error-bridgedomain5.yaml new file mode 100644 index 0000000..2bf6627 --- /dev/null +++ b/unittest/yaml/error-bridgedomain5.yaml @@ -0,0 +1,18 @@ +test: + description: "BridgeDomain BVIs must be unique" + errors: + expected: + - "bridgedomain .* BVI loop0 is not unique" + count: 2 +--- +loopbacks: + loop0: + description: "Cannot be BVI for both bd10 and bd11" + +bridgedomains: + bd10: + description: "Bridge Domain 10" + bvi: loop0 + bd11: + description: "Bridge Domain 11" + bvi: loop0 diff --git a/unittest/yaml/error-bridgedomain8.yaml b/unittest/yaml/error-bridgedomain8.yaml new file mode 100644 index 0000000..4a1a692 --- /dev/null +++ b/unittest/yaml/error-bridgedomain8.yaml @@ -0,0 +1,11 @@ +test: + description: "BridgeDomain BVIs must exist" + errors: + expected: + - "bridgedomain .* BVI .* does not exist" + count: 1 +--- +bridgedomains: + bd10: + description: "Bridge Domain 10" + bvi: loop0 diff --git a/vpp/reconciler.py b/vpp/reconciler.py index 974a640..eeb3233 100644 --- a/vpp/reconciler.py +++ b/vpp/reconciler.py @@ -69,12 +69,12 @@ class Reconciler(): if not self.prune_lcps(): self.logger.warning("Could not prune LCPs from VPP") ret = False - if not self.prune_loopbacks(): - self.logger.warning("Could not prune Loopbacks from VPP") - ret = False if not self.prune_bridgedomains(): self.logger.warning("Could not prune BridgeDomains from VPP") ret = False + if not self.prune_loopbacks(): + self.logger.warning("Could not prune Loopbacks from VPP") + ret = False if not self.prune_l2xcs(): self.logger.warning("Could not prune L2 Cross Connects from VPP") ret = False @@ -165,11 +165,16 @@ class Reconciler(): members = [] if not config_iface: for member in bridge.sw_if_details: + if member.sw_if_index == bridge.bvi_sw_if_index: + continue member_iface = self.vpp.config['interfaces'][member.sw_if_index] member_ifname = member_iface.interface_name if member_iface.sub_id > 0: self.logger.info("1> set interface l2 tag-rewrite %s disable" % member_ifname) self.logger.info("1> set interface l3 %s" % member_ifname) + if bridge.bvi_sw_if_index in self.vpp.config['interfaces']: + bviname = self.vpp.config['interfaces'][bridge.bvi_sw_if_index].interface_name + self.logger.info("1> set interface l3 %s" % bviname) self.logger.info("1> create bridge-domain %d del" % idx) else: self.logger.debug("BridgeDomain OK: %s" % (bridgename)) @@ -179,6 +184,11 @@ class Reconciler(): if interface.is_sub(self.cfg, member_ifname): self.logger.info("1> set interface l2 tag-rewrite %s disable" % member_ifname) self.logger.info("1> set interface l3 %s" % member_ifname) + if 'bvi' in config_iface: + bviname = self.vpp.config['interfaces'][bridge.bvi_sw_if_index].interface_name + if bviname != config_iface['bvi']: + self.logger.info("2> set interface l3 %s" % bviname) + return True def prune_l2xcs(self): @@ -736,6 +746,12 @@ class Reconciler(): config_bridge_ifname, config_bridge_iface = bridgedomain.get_by_name(self.cfg, "bd%d"%instance) if not 'interfaces' in config_bridge_iface: continue + if 'bvi' in config_bridge_iface: + bviname = config_bridge_iface['bvi'] + if bviname in self.vpp.config['interface_names'] and self.vpp.config['interface_names'][bviname].sw_if_index == bvi_sw_if_index: + continue + self.logger.info("1> set interface l2 bridge %s %d bvi" % (bviname, instance)) + for member_ifname in config_bridge_iface['interfaces']: member_ifname, member_iface = interface.get_by_name(self.cfg, member_ifname) if not member_ifname in bridge_members: