mgr/cephadm: fix check for if devices have changed

Directly comparing the dicts doesn't work since the
"created" field was added in. We should instead make use
of the existing Devices equality function.

Fixes: https://tracker.ceph.com/issues/57999
Signed-off-by: Adam King <adking@redhat.com>
(cherry picked from commit 55661cc859a5160822b5293f7cc49d6fdb5b715d)

diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py
index 04385a4fa81cc8e31b420133fcc487bb7b27ecbe..bd1eb7e5be1a7f96a4ce49a77d7a3b7773fe4b0f 100644 (file)
--- a/src/pybind/mgr/cephadm/inventory.py
+++ b/src/pybind/mgr/cephadm/inventory.py
@@ -642,12 +642,10 @@ class HostCache():
             del self.last_autotune[host]
 
     def devices_changed(self, host: str, b: List[inventory.Device]) -> bool:
-        a = self.devices[host]
-        if len(a) != len(b):
-            return True
-        aj = {d.path: d.to_json() for d in a}
-        bj = {d.path: d.to_json() for d in b}
-        if aj != bj:
+        old_devs = inventory.Devices(self.devices[host])
+        new_devs = inventory.Devices(b)
+        # relying on Devices class __eq__ function here
+        if old_devs != new_devs:
             self.mgr.log.info("Detected new or changed devices on %s" % host)
             return True
         return False

diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py
index 0d737380af85aa5a988fc5576de4dee09567605f..a356aa3ab1397159deff9a57b77303d8fc549ed9 100644 (file)
--- a/src/pybind/mgr/cephadm/tests/test_cephadm.py
+++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py
@@ -1214,6 +1214,7 @@ class TestCephadm(object):
             def __init__(self, c: str = 'a'):
                 # using 1015 here makes the serialized string exactly 1024 bytes if c is one char
                 self.content = {c: c * 1015}
+                self.path = 'dev/vdc'
 
             def to_json(self):
                 return self.content

diff --git a/src/pybind/mgr/dashboard/tests/test_host.py b/src/pybind/mgr/dashboard/tests/test_host.py
index 0f55ce52247c117fea36972700a8daace03f60be..5c816f08a6fd9d90dca4b4b1bc08a2c438ed95bc 100644 (file)
--- a/src/pybind/mgr/dashboard/tests/test_host.py
+++ b/src/pybind/mgr/dashboard/tests/test_host.py
@@ -499,11 +499,15 @@ class TestHosts(unittest.TestCase):
             host0 = inventories[0]
             self.assertEqual(host0['name'], 'host-0')
             self.assertEqual(host0['addr'], '1.2.3.4')
-            self.assertEqual(host0['devices'][0]['osd_ids'], [1, 2])
-            self.assertEqual(host0['devices'][1]['osd_ids'], [1])
-            self.assertEqual(host0['devices'][2]['osd_ids'], [2])
+            # devices should be sorted by path name, so
+            # /dev/sdb, /dev/sdc, nvme0n1
+            self.assertEqual(host0['devices'][0]['osd_ids'], [1])
+            self.assertEqual(host0['devices'][1]['osd_ids'], [2])
+            self.assertEqual(host0['devices'][2]['osd_ids'], [1, 2])
             host1 = inventories[1]
             self.assertEqual(host1['name'], 'host-1')
             self.assertEqual(host1['addr'], '1.2.3.5')
+            # devices should be sorted by path name, so
+            # /dev/sda, sdb
             self.assertEqual(host1['devices'][0]['osd_ids'], [])
             self.assertEqual(host1['devices'][1]['osd_ids'], [3])

diff --git a/src/python-common/ceph/deployment/inventory.py b/src/python-common/ceph/deployment/inventory.py
index 570c1dbb3615e0cd41ac24e7a08ac12ee35bea1f..4345597b11d0965158ca47c2b96938c881664816 100644 (file)
--- a/src/python-common/ceph/deployment/inventory.py
+++ b/src/python-common/ceph/deployment/inventory.py
@@ -15,7 +15,8 @@ class Devices(object):
 
     def __init__(self, devices):
         # type: (List[Device]) -> None
-        self.devices = devices  # type: List[Device]
+        # sort devices by path name so ordering is consistent
+        self.devices: List[Device] = sorted(devices, key=lambda d: d.path if d.path else '')
 
     def __eq__(self, other: Any) -> bool:
         if not isinstance(other, Devices):

diff --git a/src/python-common/ceph/tests/test_inventory.py b/src/python-common/ceph/tests/test_inventory.py
index 69c1c306cf8015a6ad595a35a9495ae27d814aef..2d916fad2f8c263df7cc51b3c54f840c7dfe5724 100644 (file)
--- a/src/python-common/ceph/tests/test_inventory.py
+++ b/src/python-common/ceph/tests/test_inventory.py
@@ -1,8 +1,10 @@
+import datetime
 import json
 import os
 import pytest
 
-from ceph.deployment.inventory import Devices
+from ceph.deployment.inventory import Devices, Device
+from ceph.utils import datetime_now
 
 
 @pytest.mark.parametrize("filename",
@@ -19,3 +21,51 @@ def test_from_json(filename):
     ds = Devices.from_json(data)
     assert len(ds.devices) == len(data)
     assert Devices.from_json(ds.to_json()) == ds
+
+
+class TestDevicesEquality():
+    created_time1 = datetime_now()
+    created_time2 = created_time1 + datetime.timedelta(seconds=30)
+
+    @pytest.mark.parametrize(
+        "old_devices, new_devices, expected_equal",
+        [
+            (  # identical list should be equal
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                True,
+            ),
+            (  # differing only in created time should still be equal
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                Devices([Device('/dev/sdb', available=True, created=created_time2),
+                         Device('/dev/sdc', available=True, created=created_time2)]),
+                True,
+            ),
+            (  # differing in some other field should make them not equal
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                Devices([Device('/dev/sdb', available=False, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                False,
+            ),
+            (  # different amount of devices should not pass equality
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1),
+                         Device('/dev/sdd', available=True, created=created_time1)]),
+                False,
+            ),
+            (  # differing order should not affect equality
+                Devices([Device('/dev/sdb', available=True, created=created_time1),
+                         Device('/dev/sdc', available=True, created=created_time1)]),
+                Devices([Device('/dev/sdc', available=True, created=created_time1),
+                         Device('/dev/sdb', available=True, created=created_time1)]),
+                True,
+            ),
+        ])
+    def test_equality(self, old_devices, new_devices, expected_equal):
+        assert (old_devices == new_devices) == expected_equal