From 1c648d2d0044cd5381d79aed5248dd363edbce55 Mon Sep 17 00:00:00 2001 From: Guillaume Abrioux Date: Tue, 18 Mar 2025 14:20:51 +0000 Subject: [PATCH] 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 (cherry picked from commit 7626c12e5fd4800187963f3b7f8691eb2847c119) --- .../ceph_volume/objectstore/lvmbluestore.py | 142 ++++++++--------- .../tests/devices/lvm/test_prepare.py | 82 ---------- .../tests/objectstore/test_lvmbluestore.py | 146 ++++++++++-------- 3 files changed, 155 insertions(+), 215 deletions(-) diff --git a/src/ceph-volume/ceph_volume/objectstore/lvmbluestore.py b/src/ceph-volume/ceph_volume/objectstore/lvmbluestore.py index 4a1557ec6a60f..a9fd578a2051c 100644 --- a/src/ceph-volume/ceph_volume/objectstore/lvmbluestore.py +++ b/src/ceph-volume/ceph_volume/objectstore/lvmbluestore.py @@ -10,7 +10,7 @@ from ceph_volume.systemd import systemctl 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 @@ class LvmBlueStore(BlueStore): 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 @@ class LvmBlueStore(BlueStore): 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 @@ class LvmBlueStore(BlueStore): 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"], diff --git a/src/ceph-volume/ceph_volume/tests/devices/lvm/test_prepare.py b/src/ceph-volume/ceph_volume/tests/devices/lvm/test_prepare.py index a908b371a7fa7..89843d3f66b60 100644 --- a/src/ceph-volume/ceph_volume/tests/devices/lvm/test_prepare.py +++ b/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 @@ class TestLVM(object): 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 @@ class TestPrepare(object): 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() diff --git a/src/ceph-volume/ceph_volume/tests/objectstore/test_lvmbluestore.py b/src/ceph-volume/ceph_volume/tests/objectstore/test_lvmbluestore.py index ba80e680b2d29..06e0f366d6199 100644 --- a/src/ceph-volume/ceph_volume/tests/objectstore/test_lvmbluestore.py +++ b/src/ceph-volume/ceph_volume/tests/objectstore/test_lvmbluestore.py @@ -1,8 +1,10 @@ import pytest +from argparse import Namespace from unittest.mock import patch, Mock, MagicMock, call from ceph_volume.objectstore.lvmbluestore import LvmBlueStore from ceph_volume.api.lvm import Volume -from ceph_volume.util import system +from ceph_volume.util import system, disk +from typing import Callable class TestLvmBlueStore: @@ -119,8 +121,10 @@ class TestLvmBlueStore: @patch('ceph_volume.util.disk.is_partition', Mock(return_value=True)) @patch('ceph_volume.api.lvm.create_lv') - def test_prepare_data_device(self, m_create_lv, factory): - args = factory(data='/dev/foo', + def test_prepare_data_device(self, + m_create_lv: MagicMock, + factory: Callable[..., Namespace]) -> None: + args = factory(data='/dev/foo1', data_slots=1, data_size=102400) self.lvm_bs.args = args @@ -175,10 +179,18 @@ class TestLvmBlueStore: self.lvm_bs.safe_prepare() assert m_rollback_osd.mock_calls == [call('111')] + @patch('ceph_volume.objectstore.lvmbluestore.LvmBlueStore.pre_prepare', Mock(return_value=None)) + @patch('ceph_volume.objectstore.lvmbluestore.LvmBlueStore.prepare_dmcrypt', MagicMock()) + @patch('ceph_volume.objectstore.baseobjectstore.BaseObjectStore.prepare_osd_req', MagicMock()) + @patch('ceph_volume.objectstore.bluestore.BlueStore.osd_mkfs', MagicMock()) + @patch('ceph_volume.util.disk.is_partition', Mock(return_value=True)) @patch('ceph_volume.objectstore.baseobjectstore.BaseObjectStore.get_ptuuid', Mock(return_value='c6798f59-01')) @patch('ceph_volume.api.lvm.Volume.set_tags', MagicMock()) @patch('ceph_volume.api.lvm.get_single_lv') - def test_prepare(self, m_get_single_lv, is_root, factory): + def test_prepare(self, + m_get_single_lv: MagicMock, + is_root: Callable[..., None], + factory: Callable[..., Namespace]) -> None: m_get_single_lv.return_value = Volume(lv_name='lv_foo', lv_path='/fake-path', vg_name='vg_foo', @@ -194,16 +206,14 @@ class TestLvmBlueStore: with_tpm=False ) self.lvm_bs.args = args - self.lvm_bs.pre_prepare = lambda: None self.lvm_bs.block_lv = MagicMock() - self.lvm_bs.prepare_osd_req = MagicMock() - self.lvm_bs.osd_mkfs = MagicMock() - self.lvm_bs.prepare_dmcrypt = MagicMock() self.lvm_bs.secrets['dmcrypt_key'] = 'fake-secret' self.lvm_bs.prepare() assert self.lvm_bs.wal_device_path == '/dev/foo1' assert self.lvm_bs.db_device_path == '/dev/foo2' - assert self.lvm_bs.block_lv.set_tags.mock_calls == [call({'ceph.type': 'block', 'ceph.vdo': '0', 'ceph.wal_uuid': 'c6798f59-01', 'ceph.wal_device': '/dev/foo1', 'ceph.db_uuid': 'c6798f59-01', 'ceph.db_device': '/dev/foo2'})] + assert self.lvm_bs.block_lv.set_tags.mock_calls == [call({ + 'ceph.type': 'block', + })] assert not self.lvm_bs.prepare_dmcrypt.called assert self.lvm_bs.osd_mkfs.called assert self.lvm_bs.prepare_osd_req.called @@ -248,84 +258,96 @@ class TestLvmBlueStore: {}) assert result == '' - def test_setup_device_is_none(self): - result = self.lvm_bs.setup_device('block', - None, - {}, - 1, - 1) - assert result == ('', '', {}) - @patch('ceph_volume.api.lvm.Volume.set_tags', return_value=MagicMock()) @patch('ceph_volume.util.system.generate_uuid', Mock(return_value='d83fa1ca-bd68-4c75-bdc2-464da58e8abd')) @patch('ceph_volume.api.lvm.create_lv') @patch('ceph_volume.util.disk.is_device', Mock(return_value=True)) - def test_setup_device_is_device(self, m_create_lv, m_set_tags): + def test_setup_metadata_devices_is_device(self, + m_create_lv: MagicMock, + m_set_tags: MagicMock, + factory: Callable[..., Namespace]) -> None: m_create_lv.return_value = Volume(lv_name='lv_foo', lv_path='/fake-path', vg_name='vg_foo', lv_tags='', lv_uuid='fake-uuid') - result = self.lvm_bs.setup_device('block', - '/dev/foo', - {}, - 1, - 1) - assert m_create_lv.mock_calls == [call(name_prefix='osd-block', + args = factory(cluster_fsid='abcd', + osd_fsid='abc123', + crush_device_class='ssd', + osd_id='111', + block_db='/dev/db', + block_db_size=disk.Size(gb=200), + block_db_slots=1, + block_wal=None, + block_wal_size='0', + block_wal_slots=None) + self.lvm_bs.args = args + self.lvm_bs.setup_metadata_devices() + assert m_create_lv.mock_calls == [call(name_prefix='osd-db', uuid='d83fa1ca-bd68-4c75-bdc2-464da58e8abd', vg=None, - device='/dev/foo', + device='/dev/db', slots=1, extents=None, - size=1, - tags={'ceph.type': 'block', + size=disk.Size(gb=200), + tags={'ceph.type': 'db', 'ceph.vdo': '0', - 'ceph.block_device': '/fake-path', - 'ceph.block_uuid': 'fake-uuid'})] - assert result == ('/fake-path', - 'fake-uuid', - {'ceph.type': 'block', - 'ceph.vdo': '0', - 'ceph.block_device': '/fake-path', - 'ceph.block_uuid': 'fake-uuid' - }) + 'ceph.db_device': '/fake-path', + 'ceph.db_uuid': 'fake-uuid'})] @patch('ceph_volume.api.lvm.get_single_lv') @patch('ceph_volume.api.lvm.Volume.set_tags', return_value=MagicMock()) - def test_setup_device_is_lv(self, m_set_tags, m_get_single_lv): + def test_setup_metadata_devices_is_lv(self, + m_set_tags: MagicMock, + m_get_single_lv: MagicMock, + factory: Callable[..., Namespace]) -> None: m_get_single_lv.return_value = Volume(lv_name='lv_foo', lv_path='/fake-path', vg_name='vg_foo', lv_tags='', lv_uuid='fake-uuid') - result = self.lvm_bs.setup_device('block', - 'vg_foo/lv_foo', - {}, - 1, - 1) - assert result == ('/fake-path', - 'fake-uuid', - {'ceph.type': 'block', - 'ceph.vdo': '0', - 'ceph.block_device': '/fake-path', - 'ceph.block_uuid': 'fake-uuid' - }) + args = factory(cluster_fsid='abcd', + osd_fsid='abc123', + crush_device_class='ssd', + osd_id='111', + block_db='vg1/lv1', + block_db_size=disk.Size(gb=200), + block_db_slots=1, + block_wal=None, + block_wal_size='0', + block_wal_slots=None) + self.lvm_bs.args = args + self.lvm_bs.setup_metadata_devices() + assert m_set_tags.mock_calls == [call({ + 'ceph.type': 'db', + 'ceph.vdo': '0', + 'ceph.db_uuid': 'fake-uuid', + 'ceph.db_device': '/fake-path' + })] + @patch('ceph_volume.util.disk.is_partition', Mock(return_value=True)) + @patch('ceph_volume.objectstore.baseobjectstore.BaseObjectStore.get_ptuuid', Mock(return_value='c6798f59-01')) @patch('ceph_volume.api.lvm.Volume.set_tags', return_value=MagicMock()) - def test_setup_device_partition(self, m_set_tags): - self.lvm_bs.get_ptuuid = lambda x: 'c6798f59-01' - result = self.lvm_bs.setup_device('block', - '/dev/foo1', - {}, - 1, - 1) - assert result == ('/dev/foo1', - 'c6798f59-01', - {'ceph.type': 'block', - 'ceph.vdo': '0', - 'ceph.block_uuid': 'c6798f59-01', - 'ceph.block_device': '/dev/foo1'}) + @patch('ceph_volume.api.lvm.create_lv') + def test_setup_metadata_devices_partition(self, + m_create_lv: MagicMock, + m_set_tags: MagicMock, + factory: Callable[..., Namespace]) -> None: + args = factory(cluster_fsid='abcd', + osd_fsid='abc123', + crush_device_class='ssd', + osd_id='111', + block_db='/dev/foo1', + block_db_size=disk.Size(gb=200), + block_db_slots=1, + block_wal=None, + block_wal_size='0', + block_wal_slots=None) + self.lvm_bs.args = args + self.lvm_bs.setup_metadata_devices() + m_create_lv.assert_not_called() + m_set_tags.assert_not_called() def test_get_osd_device_path_lv_block(self): lvs = [Volume(lv_name='lv_foo', -- 2.39.5