From ffd100305f8e694f64c377a57c91c6fc6a9b6bd3 Mon Sep 17 00:00:00 2001 From: Adam King Date: Wed, 9 Nov 2022 13:24:25 -0500 Subject: [PATCH] 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 (cherry picked from commit 55661cc859a5160822b5293f7cc49d6fdb5b715d) --- src/pybind/mgr/cephadm/inventory.py | 10 ++-- src/pybind/mgr/cephadm/tests/test_cephadm.py | 1 + src/pybind/mgr/dashboard/tests/test_host.py | 10 ++-- .../ceph/deployment/inventory.py | 3 +- .../ceph/tests/test_inventory.py | 52 ++++++++++++++++++- 5 files changed, 65 insertions(+), 11 deletions(-) diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py index 04385a4fa81c..bd1eb7e5be1a 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 0d737380af85..a356aa3ab139 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 0f55ce52247c..5c816f08a6fd 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 570c1dbb3615..4345597b11d0 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 69c1c306cf80..2d916fad2f8c 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 -- 2.47.3