Merge ~cgrabowski/maas:fix_lp2051988_hardware_sync_updates_without_change into maas:master

[Christian Grabowski]

fix: only create HW sync events when hardware has changed

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix_lp2051988_hardware_sync_updates_without_change lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/5429/console
COMMIT: adacf7ed450000ebf4c25c309ccd9cb686ca167f

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix_lp2051988_hardware_sync_updates_without_change lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: a221000b4208f3c00a6c96cb0f614a82a9826c68

review: Approve

[Jack Lloyd-Walters (lloydwaltersj)]

Looks good! +1

[Jack Lloyd-Walters (lloydwaltersj)]

review: Approve

diff --git a/src/metadataserver/builtin_scripts/hooks.py b/src/metadataserver/builtin_scripts/hooks.py
index 80b7f8e..431b2c0 100644
--- a/src/metadataserver/builtin_scripts/hooks.py
+++ b/src/metadataserver/builtin_scripts/hooks.py
@@ -14,6 +14,7 @@ import logging
 import operator
 from operator import itemgetter
 import re
+from typing import Any
 
 from django.core.exceptions import ValidationError
 from django.db import IntegrityError, transaction
@@ -368,6 +369,34 @@ def _process_system_information(node, system_data):
         node.tags.add(tag)
 
 
+def _device_diff(
+    old: NodeDevice,
+    new: dict[str, Any],
+    new_hardware_type: HARDWARE_TYPE,
+    numa_node: int,
+    physical_blockdevice: PhysicalBlockDevice,
+    physical_interface: PhysicalInterface,
+    commissioning_driver: str,
+    device_vpd: dict[str, Any],
+) -> bool:
+    if not old:
+        return True
+
+    diff = not (
+        old.hardware_type == new_hardware_type
+        and old.numa_node == numa_node
+        and old.physical_blockdevice == physical_blockdevice
+        and old.physical_interface == physical_interface
+        and old.vendor_name == new.get("vendor")
+        and old.product_name == new.get("product")
+        and old.commissioning_driver == commissioning_driver
+    )
+
+    for vpd in NodeDeviceVPD.objects.filter(node_device=old):
+        diff = diff or (device_vpd.get(vpd.key) != vpd.value)
+    return diff
+
+
 def _add_or_update_node_device(
     node,
     numa_nodes,
@@ -404,18 +433,29 @@ def _add_or_update_node_device(
 
     if key in old_devices:
         node_device = old_devices.pop(key)
-        node_device.hardware_type = hardware_type
-        node_device.numa_node = numa_node
-        node_device.physical_block_device = storage_device
-        node_device.physical_interface = network_device
-        node_device.vendor_name = device.get("vendor")
-        node_device.product_name = device.get("product")
-        node_device.commissioning_driver = commissioning_driver
-        node_device.save()
-        _add_node_device_vpd(node_device, device_vpd)
-        _hardware_sync_node_device_notify(
-            node, node_device, HARDWARE_SYNC_ACTIONS.UPDATED
-        )
+
+        if _device_diff(
+            node_device,
+            device,
+            hardware_type,
+            numa_node,
+            storage_device,
+            network_device,
+            commissioning_driver,
+            device_vpd,
+        ):
+            node_device.hardware_type = hardware_type
+            node_device.numa_node = numa_node
+            node_device.physical_block_device = storage_device
+            node_device.physical_interface = network_device
+            node_device.vendor_name = device.get("vendor")
+            node_device.product_name = device.get("product")
+            node_device.commissioning_driver = commissioning_driver
+            node_device.save()
+            _add_node_device_vpd(node_device, device_vpd)
+            _hardware_sync_node_device_notify(
+                node, node_device, HARDWARE_SYNC_ACTIONS.UPDATED
+            )
     else:
         pci_address = device.get("pci_address")
         create_args = {
@@ -609,6 +649,7 @@ def _process_lxd_resources(node, data):
     resources = data["resources"]
     update_deployment_resources = node.status == NODE_STATUS.DEPLOYED
     # Update CPU details.
+    old_cpu_count = node.cpu_count
     node.cpu_count, node.cpu_speed, cpu_model, numa_nodes = parse_lxd_cpuinfo(
         resources
     )
@@ -619,7 +660,8 @@ def _process_lxd_resources(node, data):
         resources.get("memory", {}), numa_nodes
     )
 
-    _hardware_sync_memory_notify(node, old_memory)
+    if old_memory != node.memory:
+        _hardware_sync_memory_notify(node, old_memory)
 
     # Create or update NUMA nodes. This must be kept as a dictionary as not all
     # systems maintain linear continuity. e.g the PPC64 machine in our CI uses
@@ -657,7 +699,7 @@ def _process_lxd_resources(node, data):
         )
 
         # in the case of hardware sync for a cpu, a new one is added if this value has been updated
-        if not created:
+        if created and old_cpu_count > 0:
             _hardware_sync_cpu_notify(
                 node, cpu_model, HARDWARE_SYNC_ACTIONS.ADDED
             )
diff --git a/src/metadataserver/builtin_scripts/tests/test_hooks.py b/src/metadataserver/builtin_scripts/tests/test_hooks.py
index 406c179..6393a15 100644
--- a/src/metadataserver/builtin_scripts/tests/test_hooks.py
+++ b/src/metadataserver/builtin_scripts/tests/test_hooks.py
@@ -44,6 +44,7 @@ from maasserver.utils.orm import reload_object
 from maastesting.testcase import MAASTestCase
 import metadataserver.builtin_scripts.hooks as hooks_module
 from metadataserver.builtin_scripts.hooks import (
+    _device_diff,
     _hardware_sync_block_device_notify,
     _hardware_sync_cpu_notify,
     _hardware_sync_memory_notify,
@@ -60,6 +61,7 @@ from metadataserver.builtin_scripts.hooks import (
     parse_interfaces,
     process_lxd_results,
     retag_node_for_hardware_by_modalias,
+    update_node_devices,
     update_node_fruid_metadata,
     update_node_network_information,
 )
@@ -4086,6 +4088,42 @@ class TestUpdateNodePhysicalBlockDevices(MAASServerTestCase):
         )
 
 
+class TestDeviceDiff(MAASServerTestCase):
+    def test_device_has_not_changed(self):
+        old = factory.make_NodeDevice()
+        new = {"vendor": old.vendor_name, "product": old.product_name}
+        result = _device_diff(
+            old,
+            new,
+            old.hardware_type,
+            old.numa_node,
+            old.physical_blockdevice,
+            old.physical_interface,
+            old.commissioning_driver,
+            {},
+        )
+        self.assertFalse(result)
+
+    def test_device_has_changed(self):
+        old = factory.make_NodeDevice()
+        factory.make_NodeDeviceVPD(node_device=old)
+        new = {"vendor": factory.make_name(), "product": factory.make_name()}
+        new_numa_node = factory.make_NUMANode()
+        new_block_device = factory.make_BlockDevice()
+        new_interface = factory.make_Interface()
+        result = _device_diff(
+            old,
+            new,
+            HARDWARE_TYPE.NODE,
+            new_numa_node,
+            new_block_device,
+            new_interface,
+            old.commissioning_driver,
+            {factory.make_name(): factory.make_name()},
+        )
+        self.assertTrue(result)
+
+
 class TestUpdateNodeNetworkInformation(MAASServerTestCase):
     """Tests the update_node_network_information function using data from LXD.
 
@@ -4968,6 +5006,191 @@ class TestUpdateBootInterface(MAASServerTestCase):
         self.assertIn("kernel-cmdline failed with status: 1", logger.output)
 
 
+class TestUpdateNodeDevices(MAASServerTestCase):
+    def test_update_node_devices_does_not_create_hwsync_event_if_nothing_changes(
+        self,
+    ):
+        node = factory.make_Node()
+        numa = factory.make_NUMANode(node=node)
+        block_device = factory.make_PhysicalBlockDevice(node=node)
+        node_device = factory.make_NodeDevice(
+            node=node,
+            physical_blockdevice=block_device,
+            hardware_type=HARDWARE_TYPE.STORAGE,
+            numa_node=numa,
+            bus=NODE_DEVICE_BUS.PCIE,
+        )
+
+        mock_hardware_sync_notify = self.patch(
+            hooks_module, "_hardware_sync_notify"
+        )
+
+        data = {
+            "storage": {
+                "disks": [
+                    {
+                        "id": block_device.name,
+                        "pci_address": node_device.pci_address,
+                    }
+                ]
+            },
+            "pci": {
+                "devices": [
+                    {
+                        "vendor_id": node_device.vendor_id,
+                        "product_id": node_device.product_id,
+                        "vendor": node_device.vendor_name,
+                        "product": node_device.product_name,
+                        "pci_address": node_device.pci_address,
+                        "driver": node_device.commissioning_driver,
+                    }
+                ]
+            },
+        }
+
+        update_node_devices(
+            node,
+            data,
+            [numa],
+            storage_devices={node_device.pci_address: block_device},
+        )
+
+        mock_hardware_sync_notify.assert_not_called()
+
+    def test_update_node_devices_creates_hwsync_event_when_device_added(self):
+        node = factory.make_Node()
+        numa = factory.make_NUMANode(node=node)
+        block_device = factory.make_PhysicalBlockDevice(node=node)
+
+        mock_hardware_sync_notify = self.patch(
+            hooks_module, "_hardware_sync_notify"
+        )
+
+        pci_address = "0000:23:00.0"
+        data = {
+            "storage": {
+                "disks": [
+                    {
+                        "id": block_device.name,
+                        "pci_address": pci_address,
+                    }
+                ]
+            },
+            "pci": {
+                "devices": [
+                    {
+                        "vendor_id": 8086,
+                        "product_id": 8086,
+                        "vendor": factory.make_name(),
+                        "product": factory.make_name(),
+                        "pci_address": pci_address,
+                        "driver": factory.make_name(),
+                    }
+                ]
+            },
+        }
+
+        update_node_devices(
+            node, data, [numa], storage_devices={pci_address: block_device}
+        )
+        mock_hardware_sync_notify.assert_called_once_with(
+            "NODE_HARDWARE_SYNC_PCI_DEVICE",
+            node,
+            0,
+            "added",
+            device_type="pci device",
+        )
+
+    def test_update_node_devices_creates_hwsync_event_when_device_updated(
+        self,
+    ):
+        node = factory.make_Node()
+        numa = factory.make_NUMANode(node=node)
+        block_device = factory.make_PhysicalBlockDevice(node=node)
+        node_device = factory.make_NodeDevice(
+            node=node,
+            physical_blockdevice=block_device,
+            hardware_type=HARDWARE_TYPE.STORAGE,
+            numa_node=numa,
+            bus=NODE_DEVICE_BUS.PCIE,
+        )
+        data = {
+            "storage": {
+                "disks": [
+                    {
+                        "id": block_device.name,
+                        "pci_address": node_device.pci_address,
+                    }
+                ]
+            },
+            "pci": {
+                "devices": [
+                    {
+                        "vendor_id": node_device.vendor_id,
+                        "product_id": node_device.product_id,
+                        "vendor": factory.make_name(),
+                        "product": node_device.product_name,
+                        "pci_address": node_device.pci_address,
+                        "driver": node_device.commissioning_driver,
+                    }
+                ]
+            },
+        }
+
+        mock_hardware_sync_notify = self.patch(
+            hooks_module, "_hardware_sync_notify"
+        )
+
+        update_node_devices(
+            node,
+            data,
+            [numa],
+            storage_devices={node_device.pci_address: block_device},
+        )
+        mock_hardware_sync_notify.assert_called_once_with(
+            "NODE_HARDWARE_SYNC_PCI_DEVICE",
+            node,
+            node_device.device_number,
+            "updated",
+            device_type="pci device",
+        )
+
+    def test_update_node_devices_creates_hwsync_event_when_device_removed(
+        self,
+    ):
+        node = factory.make_Node()
+        numa = factory.make_NUMANode(node=node)
+        block_device = factory.make_PhysicalBlockDevice(node=node)
+        node_device = factory.make_NodeDevice(
+            node=node,
+            physical_blockdevice=block_device,
+            hardware_type=HARDWARE_TYPE.STORAGE,
+            numa_node=numa,
+            bus=NODE_DEVICE_BUS.PCIE,
+        )
+
+        mock_hardware_sync_notify = self.patch(
+            hooks_module, "_hardware_sync_notify"
+        )
+
+        data = {}
+
+        update_node_devices(
+            node,
+            data,
+            [numa],
+            storage_devices={node_device.pci_address: block_device},
+        )
+
+        mock_hardware_sync_notify.assert_called_once_with(
+            "NODE_HARDWARE_SYNC_PCI_DEVICE",
+            node,
+            node_device.device_number,
+            "removed",
+            device_type="pci device",
+        )
+
+
 class TestHardwareSyncNotify(MAASServerTestCase):
     def setup(self):
         details = EVENT_DETAILS[EVENT_TYPES.NODE_HARDWARE_SYNC_BLOCK_DEVICE]
diff --git a/src/provisioningserver/events.py b/src/provisioningserver/events.py
index bffd6ae..04b8605 100644
--- a/src/provisioningserver/events.py
+++ b/src/provisioningserver/events.py
@@ -252,25 +252,25 @@ EVENT_DETAILS = {
         description="Node exiting rescue mode failure", level=ERROR
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_BMC: EventDetail(
-        description="Node BMC hardware sync state change", level=AUDIT
+        description="Node BMC hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_BLOCK_DEVICE: EventDetail(
-        description="Node Block Device hardware sync state change", level=AUDIT
+        description="Node Block Device hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_CPU: EventDetail(
-        description="Node CPU hardware sync state change", level=AUDIT
+        description="Node CPU hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_INTERFACE: EventDetail(
-        description="Node Interface hardware sync state change", level=AUDIT
+        description="Node Interface hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_MEMORY: EventDetail(
-        description="Node Memory hardware sync state change", level=AUDIT
+        description="Node Memory hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_PCI_DEVICE: EventDetail(
-        description="Node PCI Device hardware sync state change", level=AUDIT
+        description="Node PCI Device hardware sync state change", level=INFO
     ),
     EVENT_TYPES.NODE_HARDWARE_SYNC_USB_DEVICE: EventDetail(
-        description="Node USB Device hardware sync state chage", level=AUDIT
+        description="Node USB Device hardware sync state chage", level=INFO
     ),
     EVENT_TYPES.REQUEST_NODE_START_COMMISSIONING: EventDetail(
         description="User starting node commissioning", level=DEBUG