From: Sebastian Wagner Date: Fri, 28 Feb 2020 16:50:38 +0000 (+0100) Subject: python-common: Make Drive Group filter by AND, instead of OR X-Git-Tag: v15.1.1~75^2~2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=09d5b5dd421df7b37ca7208b3bb3fa61299ce26b;p=ceph.git python-common: Make Drive Group filter by AND, instead of OR Right now, a drive group that specifies `size=100GB` + `rotational=True` matches on all drives that are either `size=100GB` OR `rotational=True`. Now we change this to AND. Fixes: https://tracker.ceph.com/issues/43713 Signed-off-by: Sebastian Wagner --- diff --git a/src/python-common/ceph/deployment/drive_selection/__init__.py b/src/python-common/ceph/deployment/drive_selection/__init__.py index 560e8bfa8588..994e2f2da572 100644 --- a/src/python-common/ceph/deployment/drive_selection/__init__.py +++ b/src/python-common/ceph/deployment/drive_selection/__init__.py @@ -1,3 +1,2 @@ from .selector import DriveSelection # NOQA from .matchers import Matcher, SubstringMatcher, EqualityMatcher, AllMatcher, SizeMatcher # NOQA -from .filter import Filter # NOQA diff --git a/src/python-common/ceph/deployment/drive_selection/filter.py b/src/python-common/ceph/deployment/drive_selection/filter.py index 14d8887f7433..106d9a1e7101 100644 --- a/src/python-common/ceph/deployment/drive_selection/filter.py +++ b/src/python-common/ceph/deployment/drive_selection/filter.py @@ -14,57 +14,21 @@ from .matchers import Matcher, SubstringMatcher, AllMatcher, SizeMatcher, Equali logger = logging.getLogger(__name__) -class Filter(object): - """ Filter class to assign properties to bare filters. - - This is a utility class that tries to simplify working - with information comming from a textfile (drive_group.yaml) - - """ - - def __init__(self, - name, # type: str - matcher, # type: Optional[Matcher] - ): - self.name = str(name) - self.matcher = matcher - logger.debug("Initializing {} filter <{}>".format( - self.matcher.__class__.__name__, self.name)) - - @property - def is_matchable(self): - # type: () -> bool - """ A property to indicate if a Filter has a matcher - - Some filter i.e. 'limit' or 'osd_per_device' are valid filter - attributes but cannot be applied to a disk set. In this case - we return 'None' - :return: If a matcher is present True/Flase - :rtype: bool - """ - return self.matcher is not None - - def __repr__(self): - """ Visual representation of the filter - """ - return 'Filter<{}>'.format(self.name) - - class FilterGenerator(object): def __init__(self, device_filter): # type: (DeviceSelection) -> None self.device_filter = device_filter def __iter__(self): - # type: () -> Generator[Filter, None, None] + # type: () -> Generator[Matcher, None, None] if self.device_filter.size: - yield Filter('size', SizeMatcher('size', self.device_filter.size)) + yield SizeMatcher('size', self.device_filter.size) if self.device_filter.model: - yield Filter('model', SubstringMatcher('model', self.device_filter.model)) + yield SubstringMatcher('model', self.device_filter.model) if self.device_filter.vendor: - yield Filter('vendor', SubstringMatcher('vendor', self.device_filter.vendor)) + yield SubstringMatcher('vendor', self.device_filter.vendor) if self.device_filter.rotational is not None: val = '1' if self.device_filter.rotational else '0' - yield Filter('rotational', EqualityMatcher('rotational', val)) + yield EqualityMatcher('rotational', val) if self.device_filter.all: - yield Filter('all', AllMatcher('all', str(self.device_filter.all))) + yield AllMatcher('all', str(self.device_filter.all)) diff --git a/src/python-common/ceph/deployment/drive_selection/selector.py b/src/python-common/ceph/deployment/drive_selection/selector.py index 5a69ab09d2d1..92bf81e6b84d 100644 --- a/src/python-common/ceph/deployment/drive_selection/selector.py +++ b/src/python-common/ceph/deployment/drive_selection/selector.py @@ -113,45 +113,37 @@ class DriveSelection(object): return [] devices = list() # type: List[Device] - for _filter in FilterGenerator(device_filter): - if not _filter.is_matchable: + for disk in self.disks.devices: + logger.debug("Processing disk {}".format(disk.path)) + + if not disk.available: logger.debug( - "Ignoring disk {}. Filter is not matchable".format( - device_filter)) + "Ignoring disk {}. Disk is not available".format(disk.path)) continue - for disk in self.disks.devices: - logger.debug("Processing disk {}".format(disk.path)) - - # continue criteria - assert _filter.matcher is not None - - if not disk.available: - logger.debug( - "Ignoring disk {}. Disk is not available".format(disk.path)) - continue + if not self._has_mandatory_idents(disk): + logger.debug( + "Ignoring disk {}. Missing mandatory idents".format( + disk.path)) + continue - if not _filter.matcher.compare(disk): - logger.debug( - "Ignoring disk {}. Filter did not match".format( - disk.path)) - continue + # break on this condition. + if self._limit_reached(device_filter, len(devices), disk.path): + logger.debug("Ignoring disk {}. Limit reached".format( + disk.path)) + break - if not self._has_mandatory_idents(disk): - logger.debug( - "Ignoring disk {}. Missing mandatory idents".format( - disk.path)) - continue + if disk in devices: + continue - # break on this condition. - if self._limit_reached(device_filter, len(devices), disk.path): - logger.debug("Ignoring disk {}. Limit reached".format( + if not all(m.compare(disk) for m in FilterGenerator(device_filter)): + logger.debug( + "Ignoring disk {}. Filter did not match".format( disk.path)) - break + continue - if disk not in devices: - logger.debug('Adding disk {}'.format(disk.path)) - devices.append(disk) + logger.debug('Adding disk {}'.format(disk.path)) + devices.append(disk) # This disk is already taken and must not be re-assigned. for taken_device in devices: diff --git a/src/python-common/ceph/deployment/translate.py b/src/python-common/ceph/deployment/translate.py index 3c51db6bdb97..a09e71269718 100644 --- a/src/python-common/ceph/deployment/translate.py +++ b/src/python-common/ceph/deployment/translate.py @@ -1,4 +1,10 @@ import logging + +try: + from typing import Optional +except ImportError: + pass + from ceph.deployment.drive_group import DriveGroupSpec from ceph.deployment.drive_selection.selector import DriveSelection @@ -16,11 +22,9 @@ class to_ceph_volume(object): self.selection = selection def run(self): + # type: () -> Optional[str] """ Generate ceph-volume commands based on the DriveGroup filters """ - try: - data_devices = [x.path for x in self.selection.data_devices()] - except AttributeError: - data_devices = [x for x in self.selection.data_devices()] + data_devices = [x.path for x in self.selection.data_devices()] db_devices = [x.path for x in self.selection.db_devices()] wal_devices = [x.path for x in self.selection.wal_devices()] journal_devices = [x.path for x in self.selection.journal_devices()] diff --git a/src/python-common/ceph/tests/test_disk_selector.py b/src/python-common/ceph/tests/test_disk_selector.py index 0e2ad2de37a6..8bc0825a3bce 100644 --- a/src/python-common/ceph/tests/test_disk_selector.py +++ b/src/python-common/ceph/tests/test_disk_selector.py @@ -479,199 +479,6 @@ class TestDriveGroup(object): return make_sample_data - if False: - def test_filter_devices_10_size_min_max(self, test_fix, inventory): - """ Test_fix's data_device_attrs is configured to take any disk from - 30G - 50G or with vendor samsung or with model 42-RGB - The default inventory setup is configured to have 10 data devices(50G) - and 2 wal devices(20G). - The expected match is 12 - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(test_fix.data_device_attrs) - assert len(ret) == 12 - - def test_filter_devices_size_exact(self, test_fix, inventory): - """ - Configure to only take disks with 20G (exact) - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size='20G')) - assert len(ret) == 2 - - def test_filter_devices_2_max(self, test_fix, inventory): - """ - Configure to only take disks with a max of 30G - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size=':30G')) - assert len(ret) == 2 - - def test_filter_devices_0_max(self, test_fix, inventory): - """ - Configure to only take disks with a max of 10G - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size=':10G')) - assert len(ret) == 0 - - def test_filter_devices_12_min(self, test_fix, inventory): - """ - Configure to only take disks with a min of 10G - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size='10G:')) - assert len(ret) == 12 - - def test_filter_devices_12_min_20G(self, test_fix, inventory): - """ - Configure to only take disks with a min of 20G - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size='20G:')) - assert len(ret) == 12 - - def test_filter_devices_0_model(self, test_fix, inventory): - """ - Configure to only take disks with a model of modelA - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(model='unknown')) - assert len(ret) == 0 - - def test_filter_devices_2_model(self, test_fix, inventory): - """ - Configure to only take disks with a model of model*(wildcard) - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(model='ssd_type_model')) - assert len(ret) == 2 - - def test_filter_devices_12_vendor(self, test_fix, inventory): - """ - Configure to only take disks with a vendor of samsung - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(vendor='samsung')) - assert len(ret) == 12 - - def test_filter_devices_2_rotational(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 0 - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(rotational='0')) - assert len(ret) == 2 - - def test_filter_devices_10_rotational(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(rotational='1')) - assert len(ret) == 10 - - def test_filter_devices_limit(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - This should take two disks, but limit=1 is in place - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(rotational='1', limit=1)) - assert len(ret) == 1 - - def test_filter_devices_all_limit_2(self, test_fix, inventory): - """ - Configure to take all disks - limiting to two - """ - inventory() - test_fix = test_fix() - ret = test_fix._filter_devices(dict(all=True, limit=2)) - assert len(ret) == 2 - - def test_filter_devices_empty_list_eq_matcher(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - This should take 10 disks, but limit=1 is in place - Available is set to False. No disks are assigned - """ - inventory(available=False) - test_fix = test_fix() - ret = test_fix._filter_devices(dict(rotational='1', limit=1)) - assert len(ret) == 0 - - def test_filter_devices_empty_string_matcher(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - This should take two disks, but limit=1 is in place - Available is set to False. No disks are assigned - """ - inventory(available=False) - test_fix = test_fix() - ret = test_fix._filter_devices(dict(vendor='samsung', limit=1)) - assert len(ret) == 0 - - def test_filter_devices_empty_size_matcher(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - This should take two disks, but limit=1 is in place - Available is set to False. No disks are assigned - """ - inventory(available=False) - test_fix = test_fix() - ret = test_fix._filter_devices(dict(size='10G:100G', limit=1)) - assert len(ret) == 0 - - def test_filter_devices_empty_all_matcher(self, test_fix, inventory): - """ - Configure to only take disks with a rotational flag of 1 - This should take two disks, but limit=1 is in place - Available is set to False. No disks are assigned - """ - inventory(available=False) - test_fix = test_fix() - ret = test_fix._filter_devices(dict(all=True, limit=1)) - assert len(ret) == 0 - - @patch('ceph.deployment.drive_selection.DriveGroup._check_filter') - def test_check_filter_support(self, check_filter_mock, test_fix): - test_fix = test_fix() - test_fix._check_filter_support() - check_filter_mock.assert_called - - def test_check_filter(self, test_fix): - test_fix = test_fix() - ret = test_fix._check_filter(dict(model='foo')) - assert ret is None - - def test_check_filter_raise(self, test_fix): - test_fix = test_fix() - with pytest.raises(DriveGroupValidationError): - test_fix._check_filter(dict(unknown='foo')) - pytest.fail("Filter unknown is not supported") - - def test_list_devices(self): - pass - - -class TestFilter(object): - def test_is_matchable(self): - ret = drive_selection.Filter(name='name', matcher=None) - assert ret.is_matchable is False - class TestDriveSelection(object): diff --git a/src/python-common/ceph/tests/test_drive_group.py b/src/python-common/ceph/tests/test_drive_group.py index 11fbfbd9a5df..9b0257414162 100644 --- a/src/python-common/ceph/tests/test_drive_group.py +++ b/src/python-common/ceph/tests/test_drive_group.py @@ -70,8 +70,8 @@ def test_ceph_volume_command_2(): db_devices=DeviceSelection(size='200GB:350GB', rotational=False), wal_devices=DeviceSelection(size='10G') ) - inventory = _mk_inventory(_mk_device(rotational=True)*2 + - _mk_device(rotational=False)*2 + + inventory = _mk_inventory(_mk_device(rotational=True, size="300.00 GB")*2 + + _mk_device(rotational=False, size="300.00 GB")*2 + _mk_device(size="10.0 GB", rotational=False)*2 ) sel = drive_selection.DriveSelection(spec, inventory) @@ -88,8 +88,8 @@ def test_ceph_volume_command_3(): wal_devices=DeviceSelection(size='10G'), encrypted=True ) - inventory = _mk_inventory(_mk_device(rotational=True)*2 + - _mk_device(rotational=False)*2 + + inventory = _mk_inventory(_mk_device(rotational=True, size="300.00 GB")*2 + + _mk_device(rotational=False, size="300.00 GB")*2 + _mk_device(size="10.0 GB", rotational=False)*2 ) sel = drive_selection.DriveSelection(spec, inventory) @@ -110,8 +110,8 @@ def test_ceph_volume_command_4(): osds_per_device=3, encrypted=True ) - inventory = _mk_inventory(_mk_device(rotational=True)*2 + - _mk_device(rotational=False)*2 + + inventory = _mk_inventory(_mk_device(rotational=True, size="300.00 GB")*2 + + _mk_device(rotational=False, size="300.00 GB")*2 + _mk_device(size="10.0 GB", rotational=False)*2 ) sel = drive_selection.DriveSelection(spec, inventory)