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 <rsachere@redhat.com>

diff --git a/src/python-common/ceph/deployment/drive_selection/selector.py b/src/python-common/ceph/deployment/drive_selection/selector.py
index 864f2e86116c3304eb65d4c7841b7e5d15284323..e30da382477c5c82072daee8e8b9f516c85715aa 100644 (file)
--- 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 7a6c09a4eef43fc143808cb3d6ca6650ae618118..a32925b69b1ddf8df2fbcc8f69a622a85d66a979 100644 (file)
--- 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