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

@@ -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']:

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)

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:

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

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"

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

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

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: