From: Adam King Date: Wed, 9 Nov 2022 18:24:25 +0000 (-0500) Subject: mgr/cephadm: fix check for if devices have changed X-Git-Tag: v18.1.0~746^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=55661cc859a5160822b5293f7cc49d6fdb5b715d;p=ceph-ci.git 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 --- diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py index 149ee123840..bb7be85f67f 100644 --- 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 76a1073fc4a..5a814e0183e 100644 --- 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 0f55ce52247..5c816f08a6f 100644 --- 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 570c1dbb361..4345597b11d 100644 --- 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 69c1c306cf8..2d916fad2f8 100644 --- 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