From cc2732baf8aa46150d69299c133fe68b320e2cb9 Mon Sep 17 00:00:00 2001 From: Jan Fajerski Date: Thu, 6 Feb 2020 16:49:12 +0100 Subject: [PATCH] ceph-volume: fix various lvm list issues A single report on a non-lvm device now works. Format was cleaned up, report lvm journal,wal, db only once. Fixes: https://tracker.ceph.com/issues/44009 Signed-off-by: Jan Fajerski (cherry picked from commit 000bf2ffff57701952e2aa1a67a04e519c4d07a6) --- .../ceph_volume/devices/lvm/listing.py | 101 +++++++++--------- .../tests/devices/lvm/test_listing.py | 76 ++----------- 2 files changed, 60 insertions(+), 117 deletions(-) diff --git a/src/ceph-volume/ceph_volume/devices/lvm/listing.py b/src/ceph-volume/ceph_volume/devices/lvm/listing.py index f8a7dda78dd50..1ae8489f6fcb6 100644 --- a/src/ceph-volume/ceph_volume/devices/lvm/listing.py +++ b/src/ceph-volume/ceph_volume/devices/lvm/listing.py @@ -92,32 +92,16 @@ class List(object): print(json.dumps(report, indent=4, sort_keys=True)) else: if not report: - raise SystemExit('No valid Ceph devices found') + raise SystemExit('No valid Ceph lvm devices found') pretty_report(report) - def create_report(self, lvs, full_report=True, arg_is_vg=False): + def create_report(self, lvs): """ Create a report for LVM dev(s) passed. Returns '{}' to denote failure. """ - if not lvs: - return {} - - def create_report_for_nonlv_device(): - # bluestore will not have a journal, filestore will not have - # a block/wal/db, so we must skip if not present - if dev_type == 'data': - return - - device_uuid = lv.tags.get('ceph.%s_uuid' % dev_type) - pv = api.get_first_pv(filters={'vg_name': lv.vg_name}) - if device_uuid and pv: - report[osd_id].append({'tags': {'PARTUUID': device_uuid}, - 'type': dev_type, - 'path': pv.pv_name}) report = {} - lvs = [lvs] if isinstance(lvs, api.Volume) else lvs for lv in lvs: if not api.is_ceph_device(lv): continue @@ -126,18 +110,30 @@ class List(object): report.setdefault(osd_id, []) lv_report = lv.as_dict() - dev_type = lv.tags.get('ceph.type', None) - if dev_type != 'journal' or (dev_type == 'journal' and - full_report): - pvs = api.get_pvs(filters={'lv_uuid': lv.lv_uuid}) - lv_report['devices'] = [pv.name for pv in pvs] if pvs else [] - report[osd_id].append(lv_report) + pvs = api.get_pvs(filters={'lv_uuid': lv.lv_uuid}) + lv_report['devices'] = [pv.name for pv in pvs] if pvs else [] + report[osd_id].append(lv_report) - if arg_is_vg or dev_type in ['journal', 'wal']: - create_report_for_nonlv_device() + phys_devs = self.create_report_non_lv_device(lv) + if phys_devs: + report[osd_id].append(phys_devs) return report + def create_report_non_lv_device(self, lv): + report = {} + if lv.tags.get('ceph.type', '') in ['data', 'block']: + for dev_type in ['journal', 'wal', 'db']: + dev = lv.tags.get('ceph.{}_device'.format(dev_type), '') + # counting / in the device name seems brittle but should work, + # lvs will have 3 + if dev and dev.count('/') == 2: + device_uuid = lv.tags.get('ceph.{}_uuid'.format(dev_type)) + report = {'tags': {'PARTUUID': device_uuid}, + 'type': dev_type, + 'path': dev} + return report + def full_report(self): """ Create a report of all Ceph LVs. Returns '{}' to denote failure. @@ -150,33 +146,36 @@ class List(object): volume in the form of vg/lv or a device with an absolute path like /dev/sda1 or /dev/sda. Returns '{}' to denote failure. """ - lv = api.get_first_lv(filters={'lv_path': device}) - # whether the dev to reported is lv... - arg_is_vg = False - - # The `device` argument can be a logical volume name or a device path. - # If it's a path that exists, use the canonical path (in particular, - # dereference symlinks); otherwise, assume it's a logical volume name - # and use it as-is. - if os.path.exists(device): - device = os.path.realpath(device) - - lv = api.get_first_lv(filters={'lv_path': device}) - if not lv: - # if device at given path is not LV, it might be PV... - pv = api.get_first_pv(filters={'pv_name': device}) - if pv: - lv = api.get_first_lv(filters={'vg_name': pv.vg_name}) - # or VG. - else: - vg_name = os.path.dirname(device) - lv = api.get_first_lv(filters={'vg_name': vg_name}) - arg_is_vg = True + lvs = [] + if os.path.isabs(device): + # we have a block device + lvs = api.get_device_lvs(device) + if not lvs: + # maybe this was a LV path /dev/vg_name/lv_name or /dev/mapper/ + lvs = api.get_lvs(filters={'path': device}) + else: + # vg_name/lv_name was passed + vg_name, lv_name = device.split('/') + lvs = api.get_lvs(filters={'lv_name': lv_name, + 'vg_name': vg_name}) + + report = self.create_report(lvs) + + if not report: + # check if device is a non-lvm journals or wal/db + for dev_type in ['journal', 'wal', 'db']: + lvs = api.get_lvs(tags={ + 'ceph.{}_device'.format(dev_type): device}) + if lvs: + # just taking the first lv here should work + lv = lvs[0] + phys_dev = self.create_report_non_lv_device(lv) + osd_id = lv.tags.get('ceph.osd_id') + if osd_id: + report[osd_id] = [phys_dev] - if not lv: - return {} - return self.create_report(lv, full_report=False, arg_is_vg=arg_is_vg) + return report def main(self): sub_command_help = dedent(""" diff --git a/src/ceph-volume/ceph_volume/tests/devices/lvm/test_listing.py b/src/ceph-volume/ceph_volume/tests/devices/lvm/test_listing.py index 0ca6992203f28..e41cbba726251 100644 --- a/src/ceph-volume/ceph_volume/tests/devices/lvm/test_listing.py +++ b/src/ceph-volume/ceph_volume/tests/devices/lvm/test_listing.py @@ -153,41 +153,19 @@ class TestFullReport(object): assert result['0'][0]['name'] == 'volume1' assert result['0'][1]['name'] == 'wal' - def test_physical_journal_gets_reported(self, pvolumes, volumes, monkeypatch): - tags = 'ceph.osd_id=0,ceph.journal_uuid=x,ceph.type=journal' - pv = api.PVolume(vg_name="VolGroup", pv_name='/dev/sda1', pv_tags={}, - pv_uuid="0000", lv_uuid="aaaa") + @pytest.mark.parametrize('type_', ['journal', 'db', 'wal']) + def test_physical_2nd_device_gets_reported(self, type_, monkeypatch): + tags = ('ceph.osd_id=0,ceph.{t}_uuid=x,ceph.type=data,' + 'ceph.{t}_device=/dev/sda1').format(t=type_) osd = api.Volume(lv_name='volume1', lv_uuid='y', lv_tags=tags, vg_name='VolGroup', lv_path='/dev/VolGroup/lv') - pvolumes.append(pv) - volumes.append(osd) - monkeypatch.setattr(lvm.listing.api, 'get_pvs', lambda **kwargs: - pvolumes) monkeypatch.setattr(lvm.listing.api, 'get_lvs', lambda **kwargs: - volumes) + [osd]) result = lvm.listing.List([]).full_report() assert result['0'][1]['path'] == '/dev/sda1' assert result['0'][1]['tags'] == {'PARTUUID': 'x'} - assert result['0'][1]['type'] == 'journal' - - def test_physical_wal_gets_reported(self, pvolumes, volumes, monkeypatch): - tags = 'ceph.osd_id=0,ceph.wal_uuid=x,ceph.type=wal' - pv = api.PVolume(pv_name='/dev/sda1', pv_tags={}, pv_uuid="0000", - vg_name="VolGroup", lv_uuid="aaaa") - osd = api.Volume(lv_name='volume1', lv_uuid='y', lv_tags=tags, - lv_path='/dev/VolGroup/lv', vg_name="VolGroup") - pvolumes.append(pv) - volumes.append(osd) - monkeypatch.setattr(lvm.listing.api, 'get_pvs', lambda **kwargs: - pvolumes) - monkeypatch.setattr(lvm.listing.api, 'get_lvs', lambda **kwargs: - volumes) - - result = lvm.listing.List([]).full_report() - assert result['0'][1]['path'] == '/dev/sda1' - assert result['0'][1]['tags'] == {'PARTUUID': 'x'} - assert result['0'][1]['type'] == 'wal' + assert result['0'][1]['type'] == type_ class TestSingleReport(object): @@ -196,9 +174,8 @@ class TestSingleReport(object): # ceph lvs are detected by looking into its tags lv = api.Volume(lv_name='lv', lv_tags={}, lv_path='/dev/VolGroup/lv', vg_name='VolGroup') - volumes.append(lv) monkeypatch.setattr(lvm.listing.api, 'get_lvs', lambda **kwargs: - volumes) + [lv]) result = lvm.listing.List([]).single_report('VolGroup/lv') assert result == {} @@ -220,20 +197,14 @@ class TestSingleReport(object): assert result['0'][0]['path'] == '/dev/VolGroup/lv' assert result['0'][0]['devices'] == [] - def test_report_a_ceph_journal_device(self, volumes, pvolumes, monkeypatch): + def test_report_a_ceph_journal_device(self, monkeypatch): # ceph lvs are detected by looking into its tags - tags = 'ceph.osd_id=0,ceph.journal_uuid=x,ceph.type=journal,' + \ + tags = 'ceph.osd_id=0,ceph.journal_uuid=x,ceph.type=data,' + \ 'ceph.journal_device=/dev/sda1' - pv = api.PVolume(pv_name='/dev/sda1', pv_uuid="0000", pv_tags={}, - vg_name="VolGroup", lv_uuid="aaaa") lv = api.Volume(lv_name='lv', lv_uuid='aaa', lv_tags=tags, lv_path='/dev/VolGroup/lv', vg_name='VolGroup') - pvolumes.append(pv) - volumes.append(lv) - monkeypatch.setattr(lvm.listing.api, 'get_pvs', lambda **kwargs: - pvolumes) monkeypatch.setattr(lvm.listing.api, 'get_lvs', lambda **kwargs: - volumes) + [lv] if 'tags' in kwargs else []) result = lvm.listing.List([]).single_report('/dev/sda1') assert result['0'][0]['tags'] == {'PARTUUID': 'x'} @@ -268,33 +239,6 @@ class TestSingleReport(object): assert result['0'][0]['path'] == '/dev/VolGroup/lv' assert result['0'][0]['devices'] == ['/dev/sda1', '/dev/sdb1'] - def test_report_a_ceph_lv_with_multiple_pvs_of_same_name(self, pvolumes, - volumes, - monkeypatch): - tags = 'ceph.osd_id=0,ceph.type=data' - lv = api.Volume(lv_name='lv', vg_name='VolGroup', lv_uuid='aaaa', - lv_path='/dev/VolGroup/lv', lv_tags=tags) - volumes.append(lv) - monkeypatch.setattr(api, 'get_lv_from_argument', lambda device: None) - monkeypatch.setattr(api, 'get_lv', lambda vg_name: lv) - FooPVolume = api.PVolume(vg_name="vg", pv_name='/dev/sda', - pv_uuid="0000", pv_tags={}, lv_uuid="aaaa") - BarPVolume = api.PVolume(vg_name="vg", pv_name='/dev/sda', - pv_uuid="0000", pv_tags={}) - pvolumes.append(FooPVolume) - pvolumes.append(BarPVolume) - listing = lvm.listing.List([]) - monkeypatch.setattr(lvm.listing.api, 'get_lvs', lambda **kwargs: - volumes) - monkeypatch.setattr(lvm.listing.api, 'get_pvs', lambda **kwargs: - pvolumes) - - result = listing.single_report('/dev/sda') - assert result['0'][0]['name'] == 'lv' - assert result['0'][0]['lv_tags'] == tags - assert result['0'][0]['path'] == '/dev/VolGroup/lv' - assert len(result) == 1 - def test_report_a_ceph_lv_with_no_matching_devices(self, volumes, monkeypatch): tags = 'ceph.osd_id=0,ceph.type=data' -- 2.39.5