From 9545623054fda8313cde1253d9af3fffcdaa3949 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Wed, 9 Feb 2022 13:39:01 +0100 Subject: [PATCH] Support microversion 1.39 The CLI now support the following syntax both for openstack resource providers list and allocation candidate list commands: --required T1,T2 --required T3 and it means (T1 or T2) and T3. In the allocation candidate list command the above can be used both outside and in a --group context. Story: 2005345 Story: 2005346 Depends-On: https://review.opendev.org/c/openstack/placement/+/826719 Change-Id: I38ff55bdd072f3a9c1ed03e28192d045cb4096cf --- .../resources/allocation_candidate.py | 33 ++++- osc_placement/resources/common.py | 20 +++ osc_placement/resources/resource_provider.py | 31 +++- osc_placement/tests/functional/base.py | 3 +- .../functional/test_allocation_candidate.py | 138 ++++++++++++++++++ .../functional/test_resource_provider.py | 66 ++++++++- osc_placement/version.py | 1 + ...sion-1.39-any-traits-da32e4ef7c9ac6b7.yaml | 10 ++ 8 files changed, 284 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/microversion-1.39-any-traits-da32e4ef7c9ac6b7.yaml diff --git a/osc_placement/resources/allocation_candidate.py b/osc_placement/resources/allocation_candidate.py index ef6c169..1e293f5 100644 --- a/osc_placement/resources/allocation_candidate.py +++ b/osc_placement/resources/allocation_candidate.py @@ -16,6 +16,7 @@ import collections from osc_lib.command import command from osc_lib import exceptions +from osc_placement.resources import common from osc_placement import version @@ -102,7 +103,10 @@ class ListAllocationCandidate(command.Lister, version.CheckerMixin): help='A required trait. May be repeated. Allocation candidates ' 'must collectively contain all of the required traits. ' 'This option requires at least ' - '``--os-placement-api-version 1.17``.' + '``--os-placement-api-version 1.17``. ' + 'Since ``--os-placement-api-version 1.39`` the value of ' + 'this parameter can be a comma separated list of trait names ' + 'to express OR relationship between those traits.' ) parser.add_argument( '--forbidden', @@ -204,18 +208,31 @@ class ListAllocationCandidate(command.Lister, version.CheckerMixin): params[_get_key('resources')] = ','.join( resource.replace('=', ':') for resource in group['resources']) + + # We need to handle required and forbidden together as they all + # end up in the same query param on the API. + # First just check that the requested feature is aligned with the + # request microversion + required_traits = [] if 'required' in group and group['required']: # Fail if --required but not high enough microversion. self.check_version(version.ge('1.17')) - params[_get_key('required')] = ','.join(group['required']) + if any(',' in required for required in group['required']): + self.check_version(version.ge('1.39')) + required_traits = group['required'] + + forbidden_traits = [] if 'forbidden' in group and group['forbidden']: self.check_version(version.ge('1.22')) - forbidden_traits = ','.join( - ['!' + f for f in group['forbidden']]) - if 'required' in params: - params[_get_key('required')] += ',' + forbidden_traits - else: - params[_get_key('required')] = forbidden_traits + forbidden_traits = ['!' + f for f in group['forbidden']] + + # Then collect the required query params containing both required + # and forbidden traits + params[_get_key('required')] = ( + common.get_required_query_param_from_args( + required_traits, forbidden_traits) + ) + if 'aggregate_uuid' in group and group['aggregate_uuid']: # Fail if --aggregate_uuid but not high enough microversion. self.check_version(version.ge('1.21')) diff --git a/osc_placement/resources/common.py b/osc_placement/resources/common.py index fcc7256..192396d 100644 --- a/osc_placement/resources/common.py +++ b/osc_placement/resources/common.py @@ -37,3 +37,23 @@ def url_with_filters(url, filters=None): url = urlparse.urljoin(url, '?' + urlencoded_filters) return url + + +def get_required_query_param_from_args(required_traits, forbidden_traits): + # Iterate the required params and collect OR groups and simple + # AND traits separately. Each OR group needs a separate query param + # while the AND traits and forbidden traits can be collated to a single + # query param + required_query_params = [] + and_traits = [] + for required in required_traits: + if ',' in required: + required_query_params.append('in:' + required) + else: + and_traits.append(required) + # We need an extra required query param for the and_traits and the + # forbidden traits + and_query = ','.join(and_traits + forbidden_traits) + if and_query: + required_query_params.append(and_query) + return required_query_params diff --git a/osc_placement/resources/resource_provider.py b/osc_placement/resources/resource_provider.py index f890ef1..51b3508 100644 --- a/osc_placement/resources/resource_provider.py +++ b/osc_placement/resources/resource_provider.py @@ -15,6 +15,7 @@ import argparse from osc_lib.command import command from osc_lib import utils +from osc_placement.resources import common from osc_placement import version @@ -114,7 +115,10 @@ class ListResourceProvider(command.Lister, version.CheckerMixin): help='A required trait. May be repeated. Resource providers ' 'must collectively contain all of the required traits. ' 'This option requires at least ' - '``--os-placement-api-version 1.18``.' + '``--os-placement-api-version 1.18``. ' + 'Since ``--os-placement-api-version 1.39`` the value of ' + 'this parameter can be a comma separated list of trait names ' + 'to express OR relationship between those traits.' ) parser.add_argument( '--forbidden', @@ -175,17 +179,28 @@ class ListResourceProvider(command.Lister, version.CheckerMixin): if 'in_tree' in parsed_args and parsed_args.in_tree: self.check_version(version.ge('1.14')) filters['in_tree'] = parsed_args.in_tree + + # We need to handle required and forbidden together as they all end up + # in the same query param on the API. + # First just check that the requested feature is aligned with the + # request microversion + required_traits = [] if 'required' in parsed_args and parsed_args.required: self.check_version(version.ge('1.18')) - filters['required'] = ','.join(parsed_args.required) + if any(',' in required for required in parsed_args.required): + self.check_version(version.ge('1.39')) + required_traits = parsed_args.required + + forbidden_traits = [] if 'forbidden' in parsed_args and parsed_args.forbidden: self.check_version(version.ge('1.22')) - forbidden_traits = ','.join( - ['!' + f for f in parsed_args.forbidden]) - if 'required' in filters: - filters['required'] += ',' + forbidden_traits - else: - filters['required'] = forbidden_traits + forbidden_traits = ['!' + f for f in parsed_args.forbidden] + + # Then collect the required query params containing both required and + # forbidden traits + filters['required'] = common.get_required_query_param_from_args( + required_traits, forbidden_traits) + if 'member_of' in parsed_args and parsed_args.member_of: # Fail if --member-of but not high enough microversion. self.check_version(version.ge('1.3')) diff --git a/osc_placement/tests/functional/base.py b/osc_placement/tests/functional/base.py index af4aed9..7c411f9 100644 --- a/osc_placement/tests/functional/base.py +++ b/osc_placement/tests/functional/base.py @@ -444,7 +444,8 @@ class BaseTestCase(base.BaseTestCase): limit=None): cmd = 'allocation candidate list ' for suffix, req_group in groups.items(): - cmd += ' --group %s' % suffix + if suffix: + cmd += ' --group %s' % suffix cmd += self._allocation_candidates_option(**req_group) if limit is not None: cmd += ' --limit %d' % limit diff --git a/osc_placement/tests/functional/test_allocation_candidate.py b/osc_placement/tests/functional/test_allocation_candidate.py index a3cf335..b60564f 100644 --- a/osc_placement/tests/functional/test_allocation_candidate.py +++ b/osc_placement/tests/functional/test_allocation_candidate.py @@ -420,3 +420,141 @@ class TestAllocationCandidate129(base.BaseTestCase): self.assertEqual(2, len(rps)) self.assertIn(self.rp1_1['uuid'], rps) self.assertIn(self.rp1_2['uuid'], rps) + + def test_list_with_any_traits_old_microversion(self): + groups = { + '': { + 'resources': ('DISK_GB=1',), + 'required': ('STORAGE_DISK_HDD,STORAGE_DISK_SSD',), + }, + '1': { + 'resources': ('VCPU=1',), + 'required': ('HW_CPU_X86_AVX',), + } + } + self.assertCommandFailed( + 'Operation or argument is not supported with version 1.29', + self.allocation_candidate_granular, groups=groups + ) + + +class TestAllocationCandidate139(base.BaseTestCase): + VERSION = '1.39' + + def setUp(self): + super(TestAllocationCandidate139, self).setUp() + + self.rp1 = self.resource_provider_create() + self.rp1_1 = self.resource_provider_create( + parent_provider_uuid=self.rp1['uuid']) + self.rp1_2 = self.resource_provider_create( + parent_provider_uuid=self.rp1['uuid']) + + self.resource_inventory_set(self.rp1['uuid'], 'DISK_GB=512') + self.resource_inventory_set( + self.rp1_1['uuid'], 'VCPU=8', 'MEMORY_MB=8192') + self.resource_inventory_set( + self.rp1_2['uuid'], 'VCPU=16', 'MEMORY_MB=8192') + + self.resource_provider_trait_set(self.rp1['uuid'], 'STORAGE_DISK_HDD') + self.resource_provider_trait_set(self.rp1_1['uuid'], 'HW_CPU_X86_AVX') + self.resource_provider_trait_set(self.rp1_2['uuid'], 'HW_CPU_X86_SSE') + + self.rp2 = self.resource_provider_create() + self.rp2_1 = self.resource_provider_create( + parent_provider_uuid=self.rp2['uuid']) + self.rp2_2 = self.resource_provider_create( + parent_provider_uuid=self.rp2['uuid']) + + self.resource_inventory_set(self.rp2['uuid'], 'DISK_GB=512') + self.resource_inventory_set( + self.rp2_1['uuid'], 'VCPU=8', 'MEMORY_MB=8192') + self.resource_inventory_set( + self.rp2_2['uuid'], 'VCPU=16', 'MEMORY_MB=8192') + + self.resource_provider_trait_set(self.rp2['uuid'], 'STORAGE_DISK_SSD') + self.resource_provider_trait_set(self.rp2_1['uuid'], 'HW_CPU_X86_AVX') + self.resource_provider_trait_set(self.rp2_2['uuid'], 'HW_CPU_X86_SSE') + + def test_list_with_any_traits(self): + # asking for HDD and AVX that is only on the first tree as the second + # has SSD instead + groups = { + '': { + 'resources': ('DISK_GB=1',), + 'required': ('STORAGE_DISK_HDD',), + }, + '1': { + 'resources': ('VCPU=1',), + 'required': ('HW_CPU_X86_AVX',), + } + } + rows = self.allocation_candidate_granular(groups=groups) + + # we expect one candidate + numbers = {row['#'] for row in rows} + self.assertEqual(1, len(numbers)) + # with two groups satisfied + self.assertEqual(2, len(rows)) + + rps = {row['resource provider'] for row in rows} + self.assertEqual({self.rp1['uuid'], self.rp1_1['uuid']}, rps) + + # extend this by asking for SSD or HDD + groups = { + '': { + 'resources': ('DISK_GB=1',), + 'required': ('STORAGE_DISK_HDD,STORAGE_DISK_SSD',), + }, + '1': { + 'resources': ('VCPU=1',), + 'required': ('HW_CPU_X86_AVX',), + } + } + rows = self.allocation_candidate_granular(groups=groups) + + # we expect two candidates now as both tree matches + numbers = {row['#'] for row in rows} + self.assertEqual(2, len(numbers)) + # with two groups satisfied each + self.assertEqual(4, len(rows)) + + rps = {row['resource provider'] for row in rows} + self.assertEqual( + { + self.rp1['uuid'], self.rp1_1['uuid'], + self.rp2['uuid'], self.rp2_1['uuid'], + }, + rps + ) + + # make it crazy by asking for (HDD or SSD) and SSD and not HDD + # this basically means SSD but tests all the branches of the client + # code + # similarly for the granular group ask for (AVX or SSE) and not SSE + groups = { + '': { + 'resources': ('DISK_GB=1',), + 'required': ( + 'STORAGE_DISK_HDD,STORAGE_DISK_SSD', + 'STORAGE_DISK_SSD' + ), + 'forbidden': ('STORAGE_DISK_HDD',), + }, + '1': { + 'resources': ('VCPU=1',), + 'required': ('HW_CPU_X86_AVX,HW_CPU_X86_SSE',), + 'forbidden': ('HW_CPU_X86_SSE',), + } + } + rows = self.allocation_candidate_granular(groups=groups) + + # SSD and AVX means we only the second tree matches with a single + # candidate + numbers = {row['#'] for row in rows} + self.assertEqual(1, len(numbers)) + # with two groups satisfied + self.assertEqual(2, len(rows)) + + rps = {row['resource provider'] for row in rows} + self.assertEqual({self.rp2['uuid'], self.rp2_1['uuid']}, rps) diff --git a/osc_placement/tests/functional/test_resource_provider.py b/osc_placement/tests/functional/test_resource_provider.py index 6a43fdd..bd99ce5 100644 --- a/osc_placement/tests/functional/test_resource_provider.py +++ b/osc_placement/tests/functional/test_resource_provider.py @@ -358,7 +358,7 @@ class TestResourceProvider122(base.BaseTestCase): rps = self.resource_provider_list( resources=('MEMORY_MB=1024', 'DISK_GB=80'), required=('HW_CPU_X86_VMX',), - forbidden=('!STORAGE_DISK_SSD',)) + forbidden=('STORAGE_DISK_SSD',)) uuids = [rp['uuid'] for rp in rps] @@ -366,3 +366,67 @@ class TestResourceProvider122(base.BaseTestCase): self.assertNotIn(rp1['uuid'], uuids) self.assertIn(rp2['uuid'], uuids) self.assertIn(rp3['uuid'], uuids) + + def test_list_required_trait_any_trait_old_microversion(self): + self.assertCommandFailed( + 'Operation or argument is not supported with version 1.22', + self.resource_provider_list, + resources=('MEMORY_MB=1024', 'DISK_GB=80'), + required=( + 'STORAGE_DISK_HDD,STORAGE_DISK_SSD', + 'HW_NIC_SRIOV_MULTIQUEUE'), + ) + + +class TestResourceProvider139(base.BaseTestCase): + VERSION = '1.39' + + def test_list_required_trait_any_trait(self): + rp1 = self.resource_provider_create() + rp2 = self.resource_provider_create() + self.resource_inventory_set( + rp1['uuid'], 'MEMORY_MB=8192', 'DISK_GB=512') + self.resource_inventory_set( + rp2['uuid'], 'MEMORY_MB=8192', 'DISK_GB=512') + self.resource_provider_trait_set( + rp1['uuid'], 'STORAGE_DISK_SSD', 'HW_NIC_SRIOV_MULTIQUEUE') + self.resource_provider_trait_set( + rp2['uuid'], 'STORAGE_DISK_HDD', 'HW_NIC_SRIOV_MULTIQUEUE') + + rps = self.resource_provider_list( + resources=('MEMORY_MB=1024', 'DISK_GB=80'), + required=('HW_NIC_SRIOV_MULTIQUEUE',)) + + self.assertEqual( + {rp1['uuid'], rp2['uuid']}, {rp['uuid'] for rp in rps}) + + # Narrow the results and check multiple args. + rps = self.resource_provider_list( + resources=('MEMORY_MB=1024', 'DISK_GB=80'), + required=('STORAGE_DISK_HDD', 'HW_NIC_SRIOV_MULTIQUEUE',)) + + self.assertEqual({rp2['uuid']}, {rp['uuid'] for rp in rps}) + + # Query for (HDD or SSD) and MULTIQUEUE and see that both RP returned + # again + rps = self.resource_provider_list( + resources=('MEMORY_MB=1024', 'DISK_GB=80'), + required=( + 'STORAGE_DISK_HDD,STORAGE_DISK_SSD', + 'HW_NIC_SRIOV_MULTIQUEUE') + ) + + self.assertEqual( + {rp1['uuid'], rp2['uuid']}, {rp['uuid'] for rp in rps}) + + # Query for (HDD or SSD) and MULTIQUEUE and !SSD and see that one of + # the RPs are filtered + rps = self.resource_provider_list( + resources=('MEMORY_MB=1024', 'DISK_GB=80'), + required=( + 'STORAGE_DISK_HDD,STORAGE_DISK_SSD', + 'HW_NIC_SRIOV_MULTIQUEUE'), + forbidden=('STORAGE_DISK_SSD',), + ) + + self.assertEqual({rp2['uuid']}, {rp['uuid'] for rp in rps}) diff --git a/osc_placement/version.py b/osc_placement/version.py index 89ab2e3..bcb452b 100644 --- a/osc_placement/version.py +++ b/osc_placement/version.py @@ -50,6 +50,7 @@ SUPPORTED_MICROVERSIONS = [ '1.29', '1.37', # unused '1.38', # Added for consumer types (Xena) + '1.39', # Added any-traits support (Yoga) ] SUPPORTED_VERSIONS = SUPPORTED_MICROVERSIONS + NEGOTIATE_VERSIONS # The max microversion lower than which are all supported by this client. diff --git a/releasenotes/notes/microversion-1.39-any-traits-da32e4ef7c9ac6b7.yaml b/releasenotes/notes/microversion-1.39-any-traits-da32e4ef7c9ac6b7.yaml new file mode 100644 index 0000000..30e1e0f --- /dev/null +++ b/releasenotes/notes/microversion-1.39-any-traits-da32e4ef7c9ac6b7.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + Both the ``openstack resource provider list`` and + ``openstack allocation candidate list`` command now supports + ``--os-placement-api-version 1.39`` where the ``--required`` argument now + can contain a comma separated list of trait names to express OR + relationship. So ``--required T1,T2 --required T3`` means + (T1 or T2) and T3. +