From 46dbc89d57dd8e0f9e1072ac90d95084f3b099c9 Mon Sep 17 00:00:00 2001
From: Samuel Allan <samuel.allan@canonical.com>
Date: Tue, 30 Jul 2024 10:07:40 +0930
Subject: [PATCH] Fix finding local osds

Previously, this function could crash upon encountering
some non-osd files such as `ceph.client.crash.keyring`.

Copy the logic from `find_filestore_osds()` to be more correct
for finding the ceph osd directories.

Closes-Bug: #2072920
Change-Id: I632008b10cfb7f667163c9381ae48432cbd65547
---
 lib/charms_ceph/utils.py              | 25 ++++++++++++-------------
 unit_tests/test_actions_osd_out_in.py | 20 +++++++++++++++-----
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/lib/charms_ceph/utils.py b/lib/charms_ceph/utils.py
index 85e6249b..1b8c8851 100644
--- a/lib/charms_ceph/utils.py
+++ b/lib/charms_ceph/utils.py
@@ -695,7 +695,7 @@ def get_crimson_osd_ids():
 
 
 def get_local_osd_ids():
-    """This will list the /var/lib/ceph/osd/* directories and try
+    """This will list the /var/lib/ceph/osd/ceph-* directories and try
     to split the ID off of the directory name and return it in
     a list. Excludes crimson OSD's from the returned list.
 
@@ -704,19 +704,18 @@ def get_local_osd_ids():
     """
     osd_ids = []
     crimson_osds = get_crimson_osd_ids()
-    osd_path = os.path.join(os.sep, 'var', 'lib', 'ceph', 'osd')
+    osd_path = '/var/lib/ceph/osd'
     if os.path.exists(osd_path):
-        try:
-            dirs = os.listdir(osd_path)
-            for osd_dir in dirs:
-                osd_id = osd_dir.split('-')[1] if '-' in osd_dir else ''
-                if (_is_int(osd_id) and
-                        filesystem_mounted(os.path.join(
-                            os.sep, osd_path, osd_dir)) and
-                        osd_id not in crimson_osds):
-                    osd_ids.append(osd_id)
-        except OSError:
-            raise
+        dirs = [d for d in os.listdir(osd_path)
+                if d.startswith('ceph-')
+                and os.path.isdir(os.path.join(osd_path, d))]
+        for osd_dir in dirs:
+            osd_id = osd_dir.split('-', maxsplit=1)[1]
+            if (_is_int(osd_id) and
+                    filesystem_mounted(os.path.join(
+                        osd_path, osd_dir)) and
+                    osd_id not in crimson_osds):
+                osd_ids.append(osd_id)
     return osd_ids
 
 
diff --git a/unit_tests/test_actions_osd_out_in.py b/unit_tests/test_actions_osd_out_in.py
index 5808adfe..ae8fe383 100644
--- a/unit_tests/test_actions_osd_out_in.py
+++ b/unit_tests/test_actions_osd_out_in.py
@@ -120,17 +120,27 @@ class OSDMountTestCase(CharmTestCase):
     def setUp(self):
         super(OSDMountTestCase, self).setUp(actions, [])
 
+    @mock.patch('os.path.isdir')
     @mock.patch('os.path.exists')
     @mock.patch('os.listdir')
     @mock.patch('charms_ceph.utils.filesystem_mounted')
-    def test_mounted_osds(self, fs_mounted, listdir, exists):
+    def test_mounted_osds(self, fs_mounted, listdir, exists, isdir):
+        # Simulate two osds, only one of which is mounted.
+        # Also include some unrelated files which should be cleanly ignored.
         exists.return_value = True
+        isdir.return_value = True
         listdir.return_value = [
-            '/var/lib/ceph/osd/ceph-1', '/var/lib/ceph/osd/ceph-2']
-        fs_mounted.side_effect = lambda x: x == listdir.return_value[0]
+            'ceph-1',
+            'ceph-2',
+            'ceph.client.crash.keyring',
+            'ceph.client.osd-removal.keyring',
+            'ceph.client.osd-upgrade.keyring',
+        ]
+        fs_mounted.side_effect = lambda x: x == '/var/lib/ceph/osd/ceph-1'
+
         osds = actions.get_local_osd_ids()
-        self.assertIn(listdir.return_value[0][-1], osds)
-        self.assertNotIn(listdir.return_value[1][-1], osds)
+
+        self.assertEqual(osds, ['1'])
 
 
 class MainTestCase(CharmTestCase):