Merge ~mpontillo/maas:backport--bug-1730991--bug-1730626--2.3 into maas:2.3

[Mike Pontillo]

LP: #1730626, #1730991 - Backport bond fixes from master.

 * Only configure lacp_rate for 802.3ad bonds.
 * Add ability to configure the number of peer notifications after a bond failover event.

Backports: d0273adb02ca81784fbf56e43d0290e8c3d84d56
Backports: f0fe0c483e43bc34704be65245339aa24119547a

[Mike Pontillo (mpontillo)]

Self-review backport.

review: Approve

diff --git a/src/maasserver/api/interfaces.py b/src/maasserver/api/interfaces.py
index 5e96ff4..577998d 100644
--- a/src/maasserver/api/interfaces.py
+++ b/src/maasserver/api/interfaces.py
@@ -218,6 +218,9 @@ class InterfacesHandler(OperationsHandler):
         :param bond_xmit_hash_policy: The transmit hash policy to use for
             slave selection in balance-xor, 802.3ad, and tlb modes.
             (Default: layer2)
+        :param bond_num_grat_arp: The number of peer notifications (IPv4 ARP
+            or IPv6 Neighbour Advertisements) to be issued after a failover.
+            (Default: 1)
 
         Supported bonding modes (bond-mode):
         balance-rr - Transmit packets in sequential order from the first
diff --git a/src/maasserver/forms/interface.py b/src/maasserver/forms/interface.py
index 29bea62..d5b0dae 100644
--- a/src/maasserver/forms/interface.py
+++ b/src/maasserver/forms/interface.py
@@ -422,6 +422,11 @@ class BondInterfaceForm(ChildInterfaceForm):
 
     bond_updelay = forms.IntegerField(min_value=0, initial=0, required=False)
 
+    # Note: we don't need a separate bond_num_unsol_na field, since (as of
+    # Linux kernel 3.0+) it's actually an alias for the same value.
+    bond_num_grat_arp = forms.IntegerField(
+        min_value=0, max_value=255, initial=1, required=False)
+
     bond_lacp_rate = forms.ChoiceField(
         choices=BOND_LACP_RATE_CHOICES, required=False,
         initial=BOND_LACP_RATE_CHOICES[0][0], error_messages={
diff --git a/src/maasserver/forms/tests/test_interface.py b/src/maasserver/forms/tests/test_interface.py
index 15719f4..a880467 100644
--- a/src/maasserver/forms/tests/test_interface.py
+++ b/src/maasserver/forms/tests/test_interface.py
@@ -789,6 +789,7 @@ class BondInterfaceFormTest(MAASServerTestCase):
             "bond_miimon": 100,
             "bond_downdelay": 0,
             "bond_updelay": 0,
+            "bond_num_grat_arp": 1,
             "bond_lacp_rate": "fast",
             "bond_xmit_hash_policy": "layer2",
             }, interface.params)
@@ -803,6 +804,7 @@ class BondInterfaceFormTest(MAASServerTestCase):
         bond_miimon = random.randint(0, 1000)
         bond_downdelay = random.randint(0, 1000)
         bond_updelay = random.randint(0, 1000)
+        bond_num_grat_arp = random.randint(0, 255)
         bond_lacp_rate = factory.pick_choice(BOND_LACP_RATE_CHOICES)
         bond_xmit_hash_policy = factory.pick_choice(
             BOND_XMIT_HASH_POLICY_CHOICES)
@@ -818,6 +820,7 @@ class BondInterfaceFormTest(MAASServerTestCase):
                 'bond_updelay': bond_updelay,
                 'bond_lacp_rate': bond_lacp_rate,
                 'bond_xmit_hash_policy': bond_xmit_hash_policy,
+                'bond_num_grat_arp': bond_num_grat_arp,
             })
         self.assertTrue(form.is_valid(), dict(form.errors))
         interface = form.save()
@@ -828,6 +831,7 @@ class BondInterfaceFormTest(MAASServerTestCase):
             "bond_updelay": bond_updelay,
             "bond_lacp_rate": bond_lacp_rate,
             "bond_xmit_hash_policy": bond_xmit_hash_policy,
+            "bond_num_grat_arp": bond_num_grat_arp,
             }, interface.params)
 
     def test__rejects_no_parents(self):
diff --git a/src/maasserver/preseed_network.py b/src/maasserver/preseed_network.py
index 539296d..1be72d6 100644
--- a/src/maasserver/preseed_network.py
+++ b/src/maasserver/preseed_network.py
@@ -396,6 +396,15 @@ class InterfaceConfiguration:
                     # which MAAS uses to keep consistent with bridges.
                     params[key.replace("_", "-")] = (
                         _get_param_value(value))
+        bond_mode = params.get('bond-mode')
+        if bond_mode is not None:
+            # Bug #1730626: lacp-rate should only be set on 802.3ad bonds.
+            if bond_mode != "802.3ad":
+                params.pop("bond-lacp-rate", None)
+            # Bug #1730991: these parameters only apply to active-backup mode.
+            if bond_mode != "active-backup":
+                params.pop("bond-num-grat-arp", None)
+                params.pop("bond-num-unsol-na", None)
         return params
 
     def _get_bridge_params(self, version=1):
diff --git a/src/maasserver/tests/test_preseed_network.py b/src/maasserver/tests/test_preseed_network.py
index fbbb648..8a509dc 100644
--- a/src/maasserver/tests/test_preseed_network.py
+++ b/src/maasserver/tests/test_preseed_network.py
@@ -569,7 +569,7 @@ class TestNetplan(MAASServerTestCase):
         }
         self.expectThat(netplan, Equals(expected_netplan))
 
-    def test__bond_with_params(self):
+    def test__non_lacp_bond_with_params(self):
         node = factory.make_Node()
         eth0 = factory.make_Interface(
             node=node, name='eth0', mac_address="00:01:02:03:04:05")
@@ -581,6 +581,7 @@ class TestNetplan(MAASServerTestCase):
                 "bond_mode": "active-backup",
                 "bond_lacp_rate": "slow",
                 "bond_xmit_hash_policy": "layer2",
+                "bond_num_grat_arp": 3,
             })
         netplan = self._render_netplan_dict(node)
         expected_netplan = {
@@ -604,6 +605,51 @@ class TestNetplan(MAASServerTestCase):
                         'mtu': 1500,
                         'parameters': {
                             "mode": "active-backup",
+                            "transmit-hash-policy": "layer2",
+                            "gratuitous-arp": 3,
+                        },
+                    },
+                },
+            }
+        }
+        self.expectThat(netplan, Equals(expected_netplan))
+
+    def test__lacp_bond_with_params(self):
+        node = factory.make_Node()
+        eth0 = factory.make_Interface(
+            node=node, name='eth0', mac_address="00:01:02:03:04:05")
+        eth1 = factory.make_Interface(
+            node=node, name='eth1', mac_address="02:01:02:03:04:05")
+        factory.make_Interface(
+            INTERFACE_TYPE.BOND,
+            node=node, name='bond0', parents=[eth0, eth1], params={
+                "bond_mode": "802.3ad",
+                "bond_lacp_rate": "slow",
+                "bond_xmit_hash_policy": "layer2",
+                "bond_num_grat_arp": 3,
+            })
+        netplan = self._render_netplan_dict(node)
+        expected_netplan = {
+            'network': {
+                'version': 2,
+                'ethernets': {
+                    'eth0': {
+                        'match': {'macaddress': '00:01:02:03:04:05'},
+                        'mtu': 1500,
+                        'set-name': 'eth0'
+                    },
+                    'eth1': {
+                        'match': {'macaddress': '02:01:02:03:04:05'},
+                        'mtu': 1500,
+                        'set-name': 'eth1'
+                    },
+                },
+                'bonds': {
+                    'bond0': {
+                        'interfaces': ['eth0', 'eth1'],
+                        'mtu': 1500,
+                        'parameters': {
+                            "mode": "802.3ad",
                             "lacp-rate": "slow",
                             "transmit-hash-policy": "layer2",
                         },
@@ -613,6 +659,51 @@ class TestNetplan(MAASServerTestCase):
         }
         self.expectThat(netplan, Equals(expected_netplan))
 
+    def test__active_backup_with_legacy_parameter(self):
+        node = factory.make_Node()
+        eth0 = factory.make_Interface(
+            node=node, name='eth0', mac_address="00:01:02:03:04:05")
+        eth1 = factory.make_Interface(
+            node=node, name='eth1', mac_address="02:01:02:03:04:05")
+        factory.make_Interface(
+            INTERFACE_TYPE.BOND,
+            node=node, name='bond0', parents=[eth0, eth1], params={
+                "bond_mode": "active-backup",
+                "bond_lacp_rate": "slow",
+                "bond_xmit_hash_policy": "layer2",
+                "bond_num_unsol_na": 3,
+            })
+        netplan = self._render_netplan_dict(node)
+        expected_netplan = {
+            'network': {
+                'version': 2,
+                'ethernets': {
+                    'eth0': {
+                        'match': {'macaddress': '00:01:02:03:04:05'},
+                        'mtu': 1500,
+                        'set-name': 'eth0'
+                    },
+                    'eth1': {
+                        'match': {'macaddress': '02:01:02:03:04:05'},
+                        'mtu': 1500,
+                        'set-name': 'eth1'
+                    },
+                },
+                'bonds': {
+                    'bond0': {
+                        'interfaces': ['eth0', 'eth1'],
+                        'mtu': 1500,
+                        'parameters': {
+                            "mode": "active-backup",
+                            "transmit-hash-policy": "layer2",
+                            "gratuitous-arp": 3
+                        },
+                    },
+                },
+            }
+        }
+        self.expectThat(netplan, Equals(expected_netplan))
+
     def test__bridge(self):
         node = factory.make_Node()
         eth0 = factory.make_Interface(
diff --git a/src/provisioningserver/utils/netplan.py b/src/provisioningserver/utils/netplan.py
index e538079..ab88ae5 100644
--- a/src/provisioningserver/utils/netplan.py
+++ b/src/provisioningserver/utils/netplan.py
@@ -55,11 +55,12 @@ ifenslave_to_netplan_bond_params = {
     "bond-active-slave": None,
     "bond-fail-over-mac": None,
     "bond-master": None,
-    "bond-num-unsol-na": None,  # XXX: use bond-num-grat-arp?
     "bond-primary": None,
     "bond-queue-id": None,
     "bond-slaves": None,
     "bond-use-carrier": None,
+    # This is just an internal alias for bond-num-grat-arp.
+    "bond-num-unsol-na": "gratuitous-arp",
 }