From: Raimund Sacherer Date: Tue, 12 May 2026 09:07:30 +0000 (+0200) Subject: python-common/drive_selection: keep existing-OSD devices past limit X-Git-Tag: testing/wip-yuri10-testing-20260526.155424-main~4^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=a75d42acb06eb910484f7ad0e5679bf4ebf32fb0;p=ceph-ci.git python-common/drive_selection: keep existing-OSD devices past limit assign_devices() breaks out of disk iteration on the first hit from _limit_reached(). When that happens, any later existing-OSD device for the current spec is silently dropped from the selection, and ceph-volume's lvm batch loses sight of it. Only break when the candidate is not an existing OSD for this service_id. Existing-for-this-spec devices continue to be added past the limit; they are already accounted for through existing_daemons. Complements d3f1a0e1c0b ("fix limit with existing devices", 2023), which excluded ceph devices from the limit count. That fix prevents the break from firing in most cases; this one keeps the iteration useful when it does fire anyway. Needs review: interaction across spec shapes (with/without explicit limit:, with/without existing_daemons) should be looked at. Fixes: https://tracker.ceph.com/issues/76522 Signed-off-by: Raimund Sacherer --- diff --git a/src/python-common/ceph/deployment/drive_selection/selector.py b/src/python-common/ceph/deployment/drive_selection/selector.py index 864f2e86116..e30da382477 100644 --- a/src/python-common/ceph/deployment/drive_selection/selector.py +++ b/src/python-common/ceph/deployment/drive_selection/selector.py @@ -162,9 +162,24 @@ class DriveSelection(object): # break on this condition. if self._limit_reached(device_filter, devices, disk.path): - logger.debug("Ignoring disk {}. Limit reached".format( - disk.path)) - break + # Check if this device has an existing OSD for this spec. + # If so, we still want to include it (don't break) as it will + # be needed for the ceph-volume lvm batch command. + is_existing_osd_for_spec = False + if disk.ceph_device_lvm and disk.lvs: + for lv in disk.lvs: + if 'osdspec_affinity' in lv.keys(): + if lv['osdspec_affinity'] == str(self.spec.service_id): + is_existing_osd_for_spec = True + break + + if not is_existing_osd_for_spec: + logger.debug("Ignoring disk {}. Limit reached".format( + disk.path)) + continue + # else: fall through to include this device even though the + # limit is reached — existing-OSD-for-spec devices are + # already accounted for via existing_daemons. if disk in devices: continue diff --git a/src/python-common/ceph/tests/test_disk_selector.py b/src/python-common/ceph/tests/test_disk_selector.py index 7a6c09a4eef..a32925b69b1 100644 --- a/src/python-common/ceph/tests/test_disk_selector.py +++ b/src/python-common/ceph/tests/test_disk_selector.py @@ -597,3 +597,61 @@ class TestDeviceSelectionLimit: # DriveSelection does device assignment on initialization. Let's check # it picked up the expected devices assert ds._data == [dev_a, dev_b] + + def test_assign_devices_keeps_existing_osd_past_limit(self): + # When _limit_reached() fires part-way through the disk iteration, + # assign_devices() used to break unconditionally, which dropped any + # subsequent device that is already an OSD for the current spec. + # ceph-volume's `lvm batch` then doesn't see those devices and the + # drive group goes out of sync. + # + # Shape: limit=2, three candidate disks. /dev/sda and /dev/sdb are + # fresh (not yet OSDs); they fill the limit. /dev/sdc is already an + # OSD for this spec — it must still be included in the selection. + existing_lv = {'osd_id': '0', 'osdspec_affinity': 'my_osd_spec'} + dev_a = Device('/dev/sda', ceph_device_lvm=False, available=True) + dev_b = Device('/dev/sdb', ceph_device_lvm=False, available=True) + dev_c = Device('/dev/sdc', ceph_device_lvm=True, available=False, + lvs=[existing_lv]) + all_devices: List[Device] = [dev_a, dev_b, dev_c] + filter = DeviceSelection(all=True, limit=2) + dgs = DriveGroupSpec(service_id='my_osd_spec', data_devices=filter) + ds = drive_selection.DriveSelection(dgs, all_devices) + + # /dev/sdc must survive the limit because it is already an OSD for + # this spec; existing_daemons accounts for it. + assert dev_c in ds._data + + def test_assign_devices_continues_past_non_osd_at_limit(self): + # Iteration-order regression: when a non-this-spec device is the + # one that trips _limit_reached(), assign_devices() must not stop + # the disk loop entirely — later disks may be existing-OSD-for-this- + # spec and must still be included. + # + # Shape: limit=2, existing_daemons=1. Three candidate disks in + # this order: + # /dev/sda — fresh, fills the new-device budget (limit - existing + # = 1 new disk allowed). After adding it, the limit is + # reached for any subsequent non-this-spec disk. + # /dev/sdb — fresh, hits _limit_reached and is *not* an existing + # OSD for the spec. Must be skipped without terminating + # the loop. + # /dev/sdc — existing OSD for this spec. Must be included because + # existing-OSD-for-spec devices bypass the limit cap. + existing_lv = {'osd_id': '0', 'osdspec_affinity': 'my_osd_spec'} + dev_a = Device('/dev/sda', ceph_device_lvm=False, available=True) + dev_b = Device('/dev/sdb', ceph_device_lvm=False, available=True) + dev_c = Device('/dev/sdc', ceph_device_lvm=True, available=False, + lvs=[existing_lv]) + all_devices: List[Device] = [dev_a, dev_b, dev_c] + filter = DeviceSelection(all=True, limit=2) + dgs = DriveGroupSpec(service_id='my_osd_spec', data_devices=filter) + ds = drive_selection.DriveSelection(dgs, all_devices, + existing_daemons=1) + + # dev_a fits in the new-device budget; dev_b is over the limit and + # not for this spec; dev_c is for this spec and must not be lost + # to an over-aggressive break. + assert dev_a in ds._data + assert dev_b not in ds._data + assert dev_c in ds._data