From d5bfd0f950b3de983a4f4ebab7a0ddb901a47c30 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) Conflicts: src/pybind/mgr/cephadm/tests/test_cephadm.py --- src/pybind/mgr/cephadm/inventory.py | 10 ++-- src/pybind/mgr/dashboard/tests/test_host.py | 10 ++-- .../ceph/deployment/inventory.py | 3 +- .../ceph/tests/test_inventory.py | 52 ++++++++++++++++++- 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py index 92e10ea394de1..9d439657fd1b2 100644 --- a/src/pybind/mgr/cephadm/inventory.py +++ b/src/pybind/mgr/cephadm/inventory.py @@ -527,12 +527,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/dashboard/tests/test_host.py b/src/pybind/mgr/dashboard/tests/test_host.py index 0f55ce52247c1..5c816f08a6fd9 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 570c1dbb3615e..4345597b11d09 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 69c1c306cf801..2d916fad2f8c2 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.39.5