sts-sponsors team mailing list archive

[Alberto Donato <mp+437417@xxxxxxxxxxxxxxxxxx>]

Alberto Donato has proposed merging ~ack/maas:beacon-cleanups into maas:master.

Commit message:
small cleanups in code related to beaconing



Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~ack/maas/+git/maas/+merge/437417
-- 
Your team MAAS Maintainers is requested to review the proposed merge of ~ack/maas:beacon-cleanups into maas:master.

diff --git a/src/provisioningserver/utils/services.py b/src/provisioningserver/utils/services.py
index 4433fe4..dbce466 100644
--- a/src/provisioningserver/utils/services.py
+++ b/src/provisioningserver/utils/services.py
@@ -514,24 +514,6 @@ class BeaconingSocketProtocol(DatagramProtocol):
             all_hints |= hints
         return all_hints
 
-    def getJSONTopologyHints(self):
-        """Returns all topology hints as a list of dictionaries.
-
-        This method is used for sending data via the RPC layer, so be cautious
-        when modifying. In addition, keys with no value are filtered out
-        of the resulting dictionary, so that the hints are smaller on the wire.
-        """
-        all_hints = self.getAllTopologyHints()
-        json_hints = [
-            {
-                key: value
-                for key, value in hint._asdict().items()
-                if value is not None
-            }
-            for hint in all_hints
-        ]
-        return json_hints
-
     def stopProtocol(self):
         super().stopProtocol()
         if self.listen_port is not None:
@@ -599,10 +581,10 @@ class BeaconingSocketProtocol(DatagramProtocol):
         :param verbose: If True, will log the payload of each beacon sent.
         """
         for ifname, ifdata in interfaces.items():
-            if self.debug:
-                log.msg("Sending multicast beacon on '%s'." % ifname)
             if not ifdata["enabled"]:
                 continue
+            if self.debug:
+                log.msg(f"Sending multicast beacon on '{ifname}'.")
             remote = interface_info_to_beacon_remote_payload(ifname, ifdata)
             # We'll make slight adjustments to the beacon payload depending
             # on the configured source subnet (if any), but the basic payload
@@ -610,7 +592,7 @@ class BeaconingSocketProtocol(DatagramProtocol):
             payload = {"remote": remote}
             if_index = socket.if_nametoindex(ifname)
             links = ifdata["links"]
-            if len(links) == 0:
+            if not links:
                 # No configured links, so try sending out a link-local IPv6
                 # multicast beacon.
                 beacon = create_beacon_payload(beacon_type, payload)
@@ -646,7 +628,7 @@ class BeaconingSocketProtocol(DatagramProtocol):
         Replies to each soliciation with a corresponding advertisement.
         """
         remote = interface_info_to_beacon_remote_payload(
-            beacon.ifname, beacon.ifinfo, beacon.vid
+            beacon.ifname, beacon.ifinfo, rx_vid=beacon.vid
         )
         # Construct the reply payload.
         payload = {"remote": remote, "acks": beacon.uuid}
@@ -723,7 +705,7 @@ class BeaconingSocketProtocol(DatagramProtocol):
         remote_ifinfo = rx.json.get("payload", {}).get("remote", None)
         if remote_ifinfo is not None and not own_beacon:
             self._add_remote_fabric_hints(hints, remote_ifinfo, rx)
-        if len(hints) > 0:
+        if hints:
             self.topology_hints[rx.uuid] = hints
             if self.debug:
                 all_hints = self.getAllTopologyHints()
@@ -1154,6 +1136,18 @@ class NetworksMonitoringService(SingleInstanceService):
             self.beaconing_protocol.stopProtocol()
         return super().stopService()
 
+    def _get_topology_hints(self):
+        # serialize hints and remove empty values to make the payload
+        # smaller
+        return [
+            {
+                key: value
+                for key, value in hint._asdict().items()
+                if value is not None
+            }
+            for hint in self.beaconing_protocol.getAllTopologyHints()
+        ]
+
     @inlineCallbacks
     def _updateInterfaces(self, interfaces):
         """Record `interfaces` if they've changed."""
@@ -1169,7 +1163,8 @@ class NetworksMonitoringService(SingleInstanceService):
                     solicitation=True
                 )
                 yield pause(3.0)
-                hints = self.beaconing_protocol.getJSONTopologyHints()
+                hints = self._get_topology_hints()
+
             maas_url, system_id, credentials = yield self.getRefreshDetails()
             yield self._run_refresh(
                 maas_url,
diff --git a/src/provisioningserver/utils/tests/test_services.py b/src/provisioningserver/utils/tests/test_services.py
index 18b79fe..05cdea1 100644
--- a/src/provisioningserver/utils/tests/test_services.py
+++ b/src/provisioningserver/utils/tests/test_services.py
@@ -435,8 +435,7 @@ class TestNetworksMonitoringService(MAASTestCase):
             "hints": {"my-hint": "foo"},
         }
         self.all_interfaces_mock.return_value = network_extra["interfaces"]
-        beaconing_mock = self.patch(services.BeaconingSocketProtocol)
-        beaconing_mock.return_value.getJSONTopologyHints.return_value = (
+        self.patch(service, "_get_topology_hints").return_value = (
             network_extra["hints"]
         )
 
@@ -1416,52 +1415,6 @@ class TestBeaconingSocketProtocol(SharedSecretTestCase):
         yield protocol.stopProtocol()
 
     @inlineCallbacks
-    def test_getJSONTopologyHints_converts_hints_to_dictionary(self):
-        # Note: Always use a random port for testing. (port=0)
-        protocol = BeaconingSocketProtocol(
-            reactor,
-            port=0,
-            process_incoming=False,
-            loopback=True,
-            interface="::",
-            debug=True,
-        )
-        # Don't try to send out any replies.
-        self.patch(services, "create_beacon_payload")
-        self.patch(protocol, "send_beacon")
-        # Need to generate a real UUID with the current time, so it doesn't
-        # get aged out.
-        uuid = str(uuid1())
-        # Make the protocol think we sent a beacon with this UUID already.
-        tx_mac = factory.make_mac_address()
-        fake_tx_beacon = FakeBeaconPayload(
-            uuid, ifname="eth1", mac=tx_mac, vid=100
-        )
-        fake_rx_beacon = {
-            "source_ip": "127.0.0.1",
-            "source_port": 5240,
-            "destination_ip": "224.0.0.118",
-            "interface": "eth0",
-            "type": "solicitation",
-            "payload": fake_tx_beacon.payload,
-        }
-        protocol.beaconReceived(fake_rx_beacon)
-        all_hints = protocol.getJSONTopologyHints()
-        expected_hints = [
-            # Note: since vid=None on the received beacon, we expect that
-            # the hint won't have a 'vid' field.
-            dict(
-                ifname="eth0",
-                hint="on_remote_network",
-                related_ifname="eth1",
-                related_vid=100,
-                related_mac=tx_mac,
-            )
-        ]
-        self.assertEqual(all_hints, expected_hints)
-        yield protocol.stopProtocol()
-
-    @inlineCallbacks
     def test_queues_multicast_beacon_soliciations_upon_request(self):
         # Note: Always use a random port for testing. (port=0)
         clock = Clock()