ceph-volume: refactor LvmBlueStore.setup_device()

This refactores redundant device setup calls in LvmBlueStore class:
Calling the same function twice with different arguments for WAL
and DB devices was inefficient and unnecessary.
The new implementation simplifies the logic by directly accessing
`self.args`, it removes the need for passing arguments manually.

Signed-off-by: Guillaume Abrioux <[email protected]>

Author: guits

src/ceph-volume/ceph_volume/objectstore/lvmbluestore.py

@@ -10,7 +10,7 @@
 from ceph_volume.devices.lvm.common import rollback_osd
 from ceph_volume.devices.lvm.listing import direct_report
 from .bluestore import BlueStore
-from typing import Dict, Any, Optional, List, Tuple, TYPE_CHECKING
+from typing import Dict, Any, Optional, List, TYPE_CHECKING
 
 if TYPE_CHECKING:
     import argparse
@@ -131,21 +131,10 @@ def prepare(self) -> None:
         self.pre_prepare()
 
         # 2/
-        self.wal_device_path, wal_uuid, tags = self.setup_device(
-            'wal',
-            self.args.block_wal,
-            self.tags,
-            self.args.block_wal_size,
-            self.args.block_wal_slots)
-        self.db_device_path, db_uuid, tags = self.setup_device(
-            'db',
-            self.args.block_db,
-            self.tags,
-            self.args.block_db_size,
-            self.args.block_db_slots)
-
+        self.setup_metadata_devices()
         self.tags['ceph.type'] = 'block'
-        self.block_lv.set_tags(self.tags)  # type: ignore
+        if self.block_lv is not None:
+            self.block_lv.set_tags(self.tags)
 
         # 3/ encryption-only operations
         if self.encrypted:
@@ -205,12 +194,7 @@ def luks_format_and_open(self,
 
         return '/dev/mapper/%s' % uuid
 
-    def setup_device(self,
-                     device_type: str,
-                     device_name: str,
-                     tags: Dict[str, Any],
-                     size: int,
-                     slots: int) -> Tuple[str, str, Dict[str, Any]]:
+    def setup_metadata_devices(self) -> None:
         """
         Check if ``device`` is an lv, if so, set the tags, making sure to
         update the tags with the lv_uuid and lv_path which the incoming tags
@@ -219,57 +203,73 @@ def setup_device(self,
         If the device is not a logical volume, then retrieve the partition UUID
         by querying ``blkid``
         """
-        if device_name is None:
-            return '', '', tags
-        tags['ceph.type'] = device_type
-        tags['ceph.vdo'] = api.is_vdo(device_name)
-
-        try:
-            vg_name, lv_name = device_name.split('/')
-            lv = api.get_single_lv(filters={'lv_name': lv_name,
-                                            'vg_name': vg_name})
-        except ValueError:
-            lv = None
-
-        if lv:
-            lv_uuid = lv.lv_uuid
-            path = lv.lv_path
-            tags['ceph.%s_uuid' % device_type] = lv_uuid
-            tags['ceph.%s_device' % device_type] = path
-            lv.set_tags(tags)
-        elif disk.is_partition(device_name) or disk.is_device(device_name):
-            # We got a disk or a partition, create an lv
-            lv_type = "osd-{}".format(device_type)
-            name_uuid = system.generate_uuid()
-            kwargs = {
-                'name_prefix': lv_type,
-                'uuid': name_uuid,
-                'vg': None,
-                'device': device_name,
-                'slots': slots,
-                'extents': None,
-                'size': None,
-                'tags': tags,
+        s: Dict[str, Any] = {
+            'db': {
+                'attr_map': 'db_device_path',
+                'device_name': self.args.block_db,
+                'device_size': self.args.block_db_size,
+                'device_slots': self.args.block_db_slots,
+                },
+            'wal': {
+                'attr_map': 'wal_device_path',
+                'device_name': self.args.block_wal,
+                'device_size': self.args.block_wal_size,
+                'device_slots': self.args.block_wal_slots,
+                }
             }
-            # TODO use get_block_db_size and co here to get configured size in
-            # conf file
-            if size != 0:
-                kwargs['size'] = size
-            lv = api.create_lv(**kwargs)
-            if lv is not None:
-                path = lv.lv_path
-                lv_uuid = lv.lv_uuid
-                tags['ceph.{}_device'.format(device_type)] = path
-                tags['ceph.{}_uuid'.format(device_type)] = lv_uuid
-                lv.set_tags(tags)
-        else:
-            # otherwise assume this is a regular disk partition
-            name_uuid = self.get_ptuuid(device_name)
-            path = device_name
-            tags['ceph.%s_uuid' % device_type] = name_uuid
-            tags['ceph.%s_device' % device_type] = path
-            lv_uuid = name_uuid
-        return path, lv_uuid, tags
+        for device_type, device_args in s.items():
+            device_name: str = device_args.get('device_name', None)
+            size: int = device_args.get('device_size')
+            slots: int = device_args.get('device_slots')
+            if device_name is None:
+                continue
+            _tags: Dict[str, Any] = self.tags.copy()
+            _tags['ceph.type'] = device_type
+            _tags['ceph.vdo'] = api.is_vdo(device_name)
+
+            try:
+                vg_name, lv_name = device_name.split('/')
+                lv = api.get_single_lv(filters={'lv_name': lv_name,
+                                                'vg_name': vg_name})
+            except ValueError:
+                lv = None
+
+            if lv:
+                _tags['ceph.%s_uuid' % device_type] = lv.lv_uuid
+                _tags['ceph.%s_device' % device_type] = lv.lv_path
+                lv.set_tags(_tags)
+            elif disk.is_partition(device_name) or disk.is_device(device_name):
+                # We got a disk or a partition, create an lv
+                path = device_name
+                lv_type = "osd-{}".format(device_type)
+                name_uuid = system.generate_uuid()
+                kwargs = {
+                    'name_prefix': lv_type,
+                    'uuid': name_uuid,
+                    'vg': None,
+                    'device': device_name,
+                    'slots': slots,
+                    'extents': None,
+                    'size': None,
+                    'tags': _tags,
+                }
+                # TODO use get_block_db_size and co here to get configured size in
+                # conf file
+                if size != 0:
+                    kwargs['size'] = size
+                # We do not create LV if this is a partition
+                if not disk.is_partition(device_name):
+                    lv = api.create_lv(**kwargs)
+                if lv is not None:
+                    path, lv_uuid = lv.lv_path, lv.lv_uuid
+                    for key, value in {
+                        f"ceph.{device_type}_uuid": lv_uuid,
+                        f"ceph.{device_type}_device": path,
+                    }.items():
+                        _tags[key] = value
+                        self.tags[key] = value
+                    lv.set_tags(_tags)
+                setattr(self, f'{device_type}_device_path', path)
 
     def get_osd_device_path(self,
                             osd_lvs: List["Volume"],

src/ceph-volume/ceph_volume/tests/devices/lvm/test_prepare.py

@@ -1,9 +1,6 @@
 import pytest
 from ceph_volume.devices import lvm
-from ceph_volume.api import lvm as api
 from unittest.mock import patch
-from ceph_volume import objectstore
-
 
 class TestLVM(object):
 
@@ -25,35 +22,6 @@ def test_main_shows_prepare_subcommands(self, capsys):
         assert 'Format an LVM device' in stdout
 
 
-@patch('ceph_volume.util.prepare.create_key', return_value='fake-secret')
-class TestPrepareDevice(object):
-
-    def test_cannot_use_device(self, m_create_key, factory):
-        args = factory(data='/dev/var/foo')
-        with pytest.raises(RuntimeError) as error:
-            p = lvm.prepare.Prepare([])
-            p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args=args)
-            p.objectstore.prepare_data_device( 'data', '0')
-        assert 'Cannot use device (/dev/var/foo)' in str(error.value)
-        assert 'A vg/lv path or an existing device is needed' in str(error.value)
-
-@patch('ceph_volume.util.prepare.create_key', return_value='fake-secret')
-class TestGetClusterFsid(object):
-    def setup_method(self):
-        self.p = lvm.prepare.Prepare([])
-
-    def test_fsid_is_passed_in(self, m_create_key, factory):
-        args = factory(cluster_fsid='aaaa-1111')
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args)
-        assert self.p.objectstore.get_cluster_fsid() == 'aaaa-1111'
-
-    def test_fsid_is_read_from_ceph_conf(self, m_create_key, factory, conf_ceph_stub):
-        conf_ceph_stub('[global]\nfsid = bbbb-2222')
-        args = factory(cluster_fsid='')
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args)
-        assert self.p.objectstore.get_cluster_fsid() == 'bbbb-2222'
-
-
 @patch('ceph_volume.util.prepare.create_key', return_value='fake-secret')
 class TestPrepare(object):
 
@@ -72,56 +40,6 @@ def test_main_shows_full_help(self, m_create_key, capsys):
         assert 'Use the bluestore objectstore' in stdout
         assert 'A physical device or logical' in stdout
 
-    def test_setup_device_device_name_is_none(self, m_create_key):
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args=[])
-        result = self.p.objectstore.setup_device(device_type='data',
-                                            device_name=None,
-                                            tags={'ceph.type': 'data'},
-                                            size=0,
-                                            slots=None)
-        assert result == ('', '', {'ceph.type': 'data'})
-
-    @patch('ceph_volume.api.lvm.Volume.set_tags')
-    @patch('ceph_volume.api.lvm.get_single_lv')
-    def test_setup_device_lv_passed(self, m_get_single_lv, m_set_tags, m_create_key):
-        fake_volume = api.Volume(lv_name='lv_foo', lv_path='/fake-path', vg_name='vg_foo', lv_tags='', lv_uuid='fake-uuid')
-        m_get_single_lv.return_value = fake_volume
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args=[])
-        result = self.p.objectstore.setup_device(device_type='data', device_name='vg_foo/lv_foo', tags={'ceph.type': 'data'}, size=0, slots=None)
-
-        assert result == ('/fake-path', 'fake-uuid', {'ceph.type': 'data',
-                                                    'ceph.vdo': '0',
-                                                    'ceph.data_uuid': 'fake-uuid',
-                                                    'ceph.data_device': '/fake-path'})
-
-    @patch('ceph_volume.api.lvm.create_lv')
-    @patch('ceph_volume.api.lvm.Volume.set_tags')
-    @patch('ceph_volume.util.disk.is_device')
-    def test_setup_device_device_passed(self, m_is_device, m_set_tags, m_create_lv, m_create_key):
-        fake_volume = api.Volume(lv_name='lv_foo', lv_path='/fake-path', vg_name='vg_foo', lv_tags='', lv_uuid='fake-uuid')
-        m_is_device.return_value = True
-        m_create_lv.return_value = fake_volume
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args=[])
-        result = self.p.objectstore.setup_device(device_type='data', device_name='/dev/sdx', tags={'ceph.type': 'data'}, size=0, slots=None)
-
-        assert result == ('/fake-path', 'fake-uuid', {'ceph.type': 'data',
-                                                    'ceph.vdo': '0',
-                                                    'ceph.data_uuid': 'fake-uuid',
-                                                    'ceph.data_device': '/fake-path'})
-
-    @patch('ceph_volume.objectstore.baseobjectstore.BaseObjectStore.get_ptuuid')
-    @patch('ceph_volume.api.lvm.get_single_lv')
-    def test_setup_device_partition_passed(self, m_get_single_lv, m_get_ptuuid, m_create_key):
-        m_get_single_lv.side_effect = ValueError()
-        m_get_ptuuid.return_value = 'fake-uuid'
-        self.p.objectstore = objectstore.lvmbluestore.LvmBlueStore(args=[])
-        result = self.p.objectstore.setup_device(device_type='data', device_name='/dev/sdx', tags={'ceph.type': 'data'}, size=0, slots=None)
-
-        assert result == ('/dev/sdx', 'fake-uuid', {'ceph.type': 'data',
-                                                    'ceph.vdo': '0',
-                                                    'ceph.data_uuid': 'fake-uuid',
-                                                    'ceph.data_device': '/dev/sdx'})
-
     def test_invalid_osd_id_passed(self, m_create_key):
         with pytest.raises(SystemExit):
             lvm.prepare.Prepare(argv=['--osd-id', 'foo']).main()