From 2360d28d0ade3136fbfc73134180b988037c2c3e Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Tue, 5 Apr 2022 15:05:03 +0000 Subject: [PATCH] Add the ability to set any mode/lb on bonds This requires a schema change, adding 'mode' and 'load-balance' fields, a semantic invariant that 'load-balance' can only be set in the case of LACP and XOR bonds, a mapper from the mode/lb strings, ie. "round-robin" to their VPP numeric counterparts, a bunch of unit tests. Then in the reconciler, changing bonds (__bond_has_diff()) will invalidate any LCP or sub-interfaces built on them, so those will have to be pruned. create_bondethernet() will now create (or re-create) the bond with the correct flags. Unit-tests, YAML tests and the integration test all pass. Updated config-guide. --- config/bondethernet.py | 67 ++++++++++++++++++++++++++ config/test_bondethernet.py | 29 ++++++++++- docs/config-guide.md | 14 ++++-- example.yaml | 2 + intest/hippo10.yaml | 2 + intest/hippo12.yaml | 2 + intest/hippo3.yaml | 1 + schema.yaml | 2 + unittest/test_bondethernet.yaml | 25 ++++++++++ unittest/yaml/error-bondethernet8.yaml | 44 +++++++++++++++++ vpp/reconciler.py | 66 +++++++++++++++++++++---- 11 files changed, 241 insertions(+), 13 deletions(-) create mode 100644 unittest/yaml/error-bondethernet8.yaml diff --git a/config/bondethernet.py b/config/bondethernet.py index 68c1afa..2d7d364 100644 --- a/config/bondethernet.py +++ b/config/bondethernet.py @@ -52,6 +52,70 @@ def is_bond_member(yaml, ifname): return False +def get_mode(yaml, ifname): + """ Return the mode of the BondEthernet as a string, defaulting to 'lacp' + if no mode is given. Return None if the bond interface doesn't exist. + + Return values: 'round-robin','active-backup','broadcast','lacp','xor' + """ + ifname, iface = get_by_name(yaml, ifname) + if not iface: + return None + + if not 'mode' in iface: + return 'lacp' + return iface['mode'] + + +def mode_to_int(mode): + """ Returns the integer representation in VPP of a given bondethernet mode, + or -1 if 'mode' is not a valid string. + + See src/vnet/bonding/bond.api and schema.yaml for valid pairs. """ + + ret = { 'round-robin': 1, 'active-backup': 2, 'xor': 3, 'broadcast': 4, 'lacp': 5 } + try: + return ret[mode] + except: + pass + return -1 + + +def get_lb(yaml, ifname): + """ Return the loadbalance strategy of the BondEthernet as a string. Only + 'xor' and 'lacp' modes have loadbalance strategies, so return None if + those modes are not used. + + Return values: 'l2', 'l23', 'l34', with 'l34' being the default if + the bond is in xor/lacp mode without a load-balance strategy set + explicitly.""" + ifname, iface = get_by_name(yaml, ifname) + if not iface: + return None + mode = get_mode(yaml, ifname) + if not mode in ['xor','lacp']: + return None + + if not 'load-balance' in iface: + return 'l34' + return iface['load-balance'] + + +def lb_to_int(lb): + """ Returns the integer representation in VPP of a given load-balance strategy, + or -1 if 'lb' is not a valid string. + + See src/vnet/bonding/bond.api and schema.yaml for valid pairs, although + bond.api defined more than we use in vppcfg. """ + + ret = { 'l2': 0, 'l34': 1, 'l23': 2, 'round-robin': 3, 'broadcast': 4, 'active-backup': 5 } + try: + return ret[lb] + except: + pass + return -1 + + def validate_bondethernets(yaml): result = True msgs = [] @@ -74,6 +138,9 @@ def validate_bondethernets(yaml): if instance > 4294967294: msgs.append("bondethernet %s has instance %d which is too large" % (ifname, instance)) result = False + if not get_mode(yaml, bond_ifname) in ['xor','lacp'] and 'load-balance' in iface: + msgs.append("bondethernet %s can only have load-balance if in mode XOR or LACP" % (ifname)) + result = False for member in iface['interfaces']: if (None, None) == interface.get_by_name(yaml, member): diff --git a/config/test_bondethernet.py b/config/test_bondethernet.py index 58c9af4..c394b9f 100644 --- a/config/test_bondethernet.py +++ b/config/test_bondethernet.py @@ -31,7 +31,34 @@ class TestBondEthernetMethods(unittest.TestCase): def test_enumerators(self): ifs = bondethernet.get_bondethernets(self.cfg) - self.assertEqual(len(ifs), 1) + self.assertEqual(len(ifs), 3) self.assertIn("BondEthernet0", ifs) + self.assertIn("BondEthernet1", ifs) + self.assertIn("BondEthernet2", ifs) self.assertNotIn("BondEthernet-noexist", ifs) + def test_get_mode(self): + self.assertEqual('lacp', bondethernet.get_mode(self.cfg, "BondEthernet0")) + self.assertEqual('xor', bondethernet.get_mode(self.cfg, "BondEthernet1")) + + def test_mode_to_int(self): + self.assertEqual(1, bondethernet.mode_to_int("round-robin")) + self.assertEqual(2, bondethernet.mode_to_int("active-backup")) + self.assertEqual(3, bondethernet.mode_to_int("xor")) + self.assertEqual(4, bondethernet.mode_to_int("broadcast")) + self.assertEqual(5, bondethernet.mode_to_int("lacp")) + self.assertEqual(-1, bondethernet.mode_to_int("not-exist")) + + def test_get_lb(self): + self.assertEqual('l34', bondethernet.get_lb(self.cfg, "BondEthernet0")) + self.assertEqual('l2', bondethernet.get_lb(self.cfg, "BondEthernet1")) + self.assertIsNone(bondethernet.get_lb(self.cfg, "BondEthernet2")) + + def test_lb_to_int(self): + self.assertEqual(0, bondethernet.lb_to_int("l2")) + self.assertEqual(1, bondethernet.lb_to_int("l34")) + self.assertEqual(2, bondethernet.lb_to_int("l23")) + self.assertEqual(3, bondethernet.lb_to_int("round-robin")) + self.assertEqual(4, bondethernet.lb_to_int("broadcast")) + self.assertEqual(5, bondethernet.lb_to_int("active-backup")) + self.assertEqual(-1, bondethernet.lb_to_int("not-exist")) diff --git a/docs/config-guide.md b/docs/config-guide.md index a7c8a6d..4fd6810 100644 --- a/docs/config-guide.md +++ b/docs/config-guide.md @@ -156,20 +156,28 @@ BondEthernets are required to be named `BondEthernetN` (note the camelcase) wher * ***interfaces***: A list of zero or more interfaces that are bond members. The interfaces must be PHYs, and in their `interface` configuration, members are allowed only to set the MTU. +* ***mode***: A mode to run the LAG in. Can be one of 'round-robin', 'active-backup', 'xor', + 'broadcast' or 'lacp'. The default is LACP. +* ***load-balance***: A loadbalancing strategy to use, if the mode is either XOR or LACP. + Can be one of 'l2', 'l23', or 'l34'. The default is l34, which hashes on the source and + destination IPs and ports. Note that the configuration object here only specifies the link aggregation and its members. BondEthernets are expected to occur as well in the `interfaces` section, where their sub-interfaces and IP addresses and so on are specified. -*Caveat*: Currently, BondEthernets are always created as `LACP` typed devices with a loadbalance -strategy of `l34`. In a future release of `vppcfg`, the type and strategy will be configurable. - Examples: ``` bondethernets: BondEthernet0: description: "Core: LACP to fsw0.lab.ipng.ch" + interfaces: [ GigabitEthernet1/0/0, GigabitEthernet1/0/1 ] + mode: lacp + load-balance: l2 + BondEthernet1: + description: "Core: RR LAG" interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: round-robin ``` ### VXLAN Tunnels diff --git a/example.yaml b/example.yaml index 0a03139..b194243 100644 --- a/example.yaml +++ b/example.yaml @@ -1,6 +1,8 @@ bondethernets: BondEthernet0: interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: xor + load-balance: l2 interfaces: GigabitEthernet3/0/0: diff --git a/intest/hippo10.yaml b/intest/hippo10.yaml index d13207d..1b4d7f9 100644 --- a/intest/hippo10.yaml +++ b/intest/hippo10.yaml @@ -1,6 +1,8 @@ bondethernets: BondEthernet0: interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: lacp + load-balance: l2 interfaces: GigabitEthernet3/0/0: diff --git a/intest/hippo12.yaml b/intest/hippo12.yaml index 0a03139..b194243 100644 --- a/intest/hippo12.yaml +++ b/intest/hippo12.yaml @@ -1,6 +1,8 @@ bondethernets: BondEthernet0: interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: xor + load-balance: l2 interfaces: GigabitEthernet3/0/0: diff --git a/intest/hippo3.yaml b/intest/hippo3.yaml index 4195324..cc7911c 100644 --- a/intest/hippo3.yaml +++ b/intest/hippo3.yaml @@ -1,6 +1,7 @@ bondethernets: BondEthernet1: interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: round-robin interfaces: GigabitEthernet3/0/0: diff --git a/schema.yaml b/schema.yaml index a9f370c..58aaf1f 100644 --- a/schema.yaml +++ b/schema.yaml @@ -35,6 +35,8 @@ loopback: bondethernet: description: str(exclude='\'"',len=64,required=False) interfaces: list(str(matches='.*GigabitEthernet[0-9]+/[0-9]+/[0-9]+')) + mode: enum('round-robin','active-backup','broadcast','lacp','xor',required=False) + load-balance: enum('l2','l23','l34',required=False) --- interface: description: str(exclude='\'"',len=64,required=False) diff --git a/unittest/test_bondethernet.yaml b/unittest/test_bondethernet.yaml index 7086f1d..746b1dc 100644 --- a/unittest/test_bondethernet.yaml +++ b/unittest/test_bondethernet.yaml @@ -2,6 +2,15 @@ bondethernets: BondEthernet0: interfaces: [ GigabitEthernet1/0/0, GigabitEthernet1/0/1 ] + BondEthernet1: + interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: xor + load-balance: l2 + + BondEthernet2: + interfaces: [ GigabitEthernet4/0/0, GigabitEthernet4/0/1 ] + mode: round-robin + interfaces: GigabitEthernet1/0/0: mtu: 3000 @@ -14,6 +23,16 @@ interfaces: 100: mtu: 2000 + GigabitEthernet3/0/0: + mtu: 3000 + GigabitEthernet3/0/1: + mtu: 3000 + + GigabitEthernet4/0/0: + mtu: 3000 + GigabitEthernet4/0/1: + mtu: 3000 + BondEthernet0: mtu: 3000 lcp: "be012345678" @@ -22,3 +41,9 @@ interfaces: 100: mtu: 2000 addresses: [ 192.0.2.9/29, 2001:db8:1::1/64 ] + + BondEthernet1: + mtu: 3000 + + BondEthernet2: + mtu: 3000 diff --git a/unittest/yaml/error-bondethernet8.yaml b/unittest/yaml/error-bondethernet8.yaml new file mode 100644 index 0000000..512649a --- /dev/null +++ b/unittest/yaml/error-bondethernet8.yaml @@ -0,0 +1,44 @@ +test: + description: "BondEthernet can only have loadbalance if XOR or LACP" + errors: + expected: + - "bondethernet BondEthernet2 can only have load-balance if in mode XOR or LACP" + count: 1 +--- +bondethernets: + BondEthernet0: + interfaces: [ GigabitEthernet1/0/0, GigabitEthernet1/0/1 ] + mode: xor + load-balance: l34 + + BondEthernet1: + interfaces: [ GigabitEthernet2/0/0, GigabitEthernet2/0/1 ] + mode: lacp + load-balance: l34 + + BondEthernet2: + interfaces: [ GigabitEthernet3/0/0, GigabitEthernet3/0/1 ] + mode: round-robin + load-balance: l34 + +interfaces: + GigabitEthernet1/0/0: + mtu: 3000 + GigabitEthernet1/0/1: + mtu: 3000 + BondEthernet0: + mtu: 3000 + + GigabitEthernet2/0/0: + mtu: 3000 + GigabitEthernet2/0/1: + mtu: 3000 + BondEthernet1: + mtu: 3000 + + GigabitEthernet3/0/0: + mtu: 3000 + GigabitEthernet3/0/1: + mtu: 3000 + BondEthernet2: + mtu: 3000 diff --git a/vpp/reconciler.py b/vpp/reconciler.py index 0e92e00..52e9271 100644 --- a/vpp/reconciler.py +++ b/vpp/reconciler.py @@ -236,6 +236,33 @@ class Reconciler(): self.vpp.cache_remove_l2xc(l2xc) return True + def __bond_has_diff(self, ifname): + """ Returns True if the given ifname (BondEthernet0) have different attributes, + or if either does not exist. + + Returns False if they are identical. + """ + if not ifname in self.vpp.cache['interface_names']: + return True + + vpp_iface = self.vpp.cache['interface_names'][ifname] + if not vpp_iface.sw_if_index in self.vpp.cache['bondethernets']: + return True + + config_ifname, config_iface = bondethernet.get_by_name(self.cfg, ifname) + if not config_iface: + return True + + vpp_bond = self.vpp.cache['bondethernets'][vpp_iface.sw_if_index] + mode = bondethernet.mode_to_int(bondethernet.get_mode(self.cfg, config_ifname)) + if mode != vpp_bond.mode: + return True + lb = bondethernet.lb_to_int(bondethernet.get_lb(self.cfg, config_ifname)) + if lb != vpp_bond.lb: + return True + + return False + def prune_bondethernets(self): """ Remove all BondEthernets from VPP, if they are not in the config. If the bond has members, remove those from the bond before removing the bond. """ @@ -244,7 +271,8 @@ class Reconciler(): for idx, bond in self.vpp.cache['bondethernets'].items(): vpp_ifname = bond.interface_name config_ifname, config_iface = bondethernet.get_by_name(self.cfg, vpp_ifname) - if not config_iface: + + if self.__bond_has_diff(vpp_ifname): self.prune_addresses(vpp_ifname, []) for member in self.vpp.cache['bondethernet_members'][idx]: member_ifname = self.vpp.cache['interfaces'][member].interface_name @@ -255,6 +283,7 @@ class Reconciler(): self.cli['prune'].append(cli); removed_interfaces.append(vpp_ifname) continue + for member in self.vpp.cache['bondethernet_members'][idx]: member_ifname = self.vpp.cache['interfaces'][member].interface_name if 'interfaces' in config_iface and not member_ifname in config_iface['interfaces']: @@ -326,7 +355,8 @@ class Reconciler(): return match def prune_sub_interfaces(self): - """ Remove interfaces from VPP if they are not in the config, or if their encapsulation is different. + """ Remove interfaces from VPP if they are not in the config, if their encapsulation is different, + or if the BondEthernet they reside on is different. Start with inner-most (QinQ/QinAD), then Dot1Q/Dot1AD.""" removed_interfaces=[] for numtags in [ 2, 1 ]: @@ -338,21 +368,27 @@ class Reconciler(): if self.__tap_is_lcp(vpp_iface.sw_if_index): continue + prune=False config_ifname, config_iface = interface.get_by_name(self.cfg, vpp_ifname) if not config_iface: - self.prune_addresses(vpp_ifname, []) - cli="delete sub %s" % (vpp_ifname) - self.cli['prune'].append(cli); - removed_interfaces.append(vpp_ifname) - continue + prune = True + elif vpp_iface.interface_dev_type=='bond' and vpp_iface.sub_number_of_tags > 0: + config_parent_ifname, config_parent_iface = interface.get_parent_by_name(self.cfg, vpp_ifname) + if self.__bond_has_diff(config_parent_ifname): + prune = True + config_encap = interface.get_encapsulation(self.cfg, vpp_ifname) vpp_encap = self.__get_encapsulation(vpp_iface) if config_encap != vpp_encap: + prune = True + + if prune: self.prune_addresses(vpp_ifname, []) cli="delete sub %s" % (vpp_ifname) self.cli['prune'].append(cli); removed_interfaces.append(vpp_ifname) continue + addresses = [] if 'addresses' in config_iface: addresses = config_iface['addresses'] @@ -482,7 +518,7 @@ class Reconciler(): removed_lcps.append(lcp.host_if_name) continue - if vpp_iface.sub_number_of_tags > 1: + if vpp_iface.sub_number_of_tags > 0: config_encap = interface.get_encapsulation(self.cfg, config_ifname) vpp_encap = self.__get_encapsulation(vpp_iface) if config_encap != vpp_encap: @@ -495,6 +531,14 @@ class Reconciler(): if vpp_iface.interface_dev_type=='Loopback': ## Loopbacks will not have a PHY to check. continue + if vpp_iface.interface_dev_type=='bond': + bond_iface = self.vpp.cache['interfaces'][vpp_iface.sup_sw_if_index] + if self.__bond_has_diff(bond_iface.interface_name): + ## If BondEthernet changed, it has to be re-created, so all LCPs must be removed. + cli="lcp delete %s" % (vpp_iface.interface_name) + self.cli['prune'].append(cli); + removed_lcps.append(lcp.host_if_name) + continue phy_lcp = lcps[vpp_iface.sup_sw_if_index] config_phy_ifname, config_phy_iface = interface.get_by_lcp_name(self.cfg, phy_lcp.host_if_name) @@ -578,7 +622,11 @@ class Reconciler(): continue ifname, iface = bondethernet.get_by_name(self.cfg, ifname) instance = int(ifname[12:]) - cli="create bond mode lacp load-balance l34 id %d" % (instance) + mode = bondethernet.get_mode(self.cfg, ifname) + cli="create bond id %d mode %s" % (instance, mode) + lb = bondethernet.get_lb(self.cfg, ifname) + if lb: + cli += " load-balance %s" % lb self.cli['create'].append(cli); return True