From f08cb3981315d60ddc611f0229fbdca495641660 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Sat, 2 Mar 2019 03:09:34 +0000 Subject: [PATCH] Add --aggregate option to 'resource provider inventory set' This adds a new '--aggregate' option which can set resource provider inventory for all resource providers that are members of the specified aggregate. The main motivation for this option is to give operators a tool to make setting overcommit allocation ratios on a per-aggregate basis easier, after the functionality from the old Aggregate[Core|Ram|Disk]Filter scheduler filters was removed back in Ocata [1]. [1] http://lists.openstack.org/pipermail/openstack-dev/2018-January/126283.html Story: 2006318 Task: 36189 Change-Id: Ib0cbb58d0adbbcfe83ee48d2ff6c9af1a516a7ae --- osc_placement/resources/inventory.py | 101 +++++++++--- osc_placement/tests/functional/base.py | 13 +- .../tests/functional/test_inventory.py | 154 ++++++++++++++++++ ...entory-set-aggregate-5f2239dd2685b636.yaml | 10 ++ requirements.txt | 1 + 5 files changed, 256 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/resource-provider-inventory-set-aggregate-5f2239dd2685b636.yaml diff --git a/osc_placement/resources/inventory.py b/osc_placement/resources/inventory.py index f0ae946..765b7bf 100644 --- a/osc_placement/resources/inventory.py +++ b/osc_placement/resources/inventory.py @@ -11,10 +11,15 @@ # under the License. from collections import defaultdict +import itertools from osc_lib.command import command +from osc_lib import exceptions +from osc_lib.i18n import _ from osc_lib import utils +from oslo_utils import excutils +from osc_placement.resources import common from osc_placement import version @@ -92,7 +97,7 @@ def parse_resource_argument(resource): return name, field, value -class SetInventory(command.Lister): +class SetInventory(command.Lister, version.CheckerMixin): """Replaces the set of inventory records for the resource provider. @@ -117,7 +122,8 @@ class SetInventory(command.Lister): parser.add_argument( 'uuid', metavar='', - help='UUID of the resource provider' + help='UUID of the resource provider or UUID of the aggregate, ' + 'if --aggregate is specified' ) fields_help = '\n'.join( '{} - {}'.format(f, INVENTORY_FIELDS[f]['help'].lower()) @@ -130,34 +136,91 @@ class SetInventory(command.Lister): default=[], action='append' ) + parser.add_argument( + '--aggregate', + action='store_true', + help='If this option is specified, the inventories for all ' + 'resource providers that are members of the aggregate will ' + 'be set. This option requires at least ' + '``--os-placement-api-version 1.3``' + ) return parser def take_action(self, parsed_args): - inventories = defaultdict(dict) - for r in parsed_args.resource: - name, field, value = parse_resource_argument(r) - inventories[name][field] = value - http = self.app.client_manager.placement - url = RP_BASE_URL + '/' + parsed_args.uuid - rp = http.request('GET', url).json() + if parsed_args.aggregate: + self.check_version(version.ge('1.3')) + filters = {'member_of': parsed_args.uuid} + url = common.url_with_filters(RP_BASE_URL, filters) + rps = http.request('GET', url).json()['resource_providers'] + if not rps: + raise exceptions.CommandError( + 'No resource providers found in aggregate with uuid %s.' % + parsed_args.uuid) + else: + url = RP_BASE_URL + '/' + parsed_args.uuid + rps = [http.request('GET', url).json()] - payload = {'inventories': inventories, - 'resource_provider_generation': rp['generation']} - url = BASE_URL.format(uuid=parsed_args.uuid) - resources = http.request('PUT', url, json=payload).json() + resources_list = [] + ret = 0 + for rp in rps: + inventories = defaultdict(dict) + url = BASE_URL.format(uuid=rp['uuid']) + payload = {'inventories': inventories, + 'resource_provider_generation': rp['generation']} - inventories = [ - dict(resource_class=k, **v) - for k, v in resources['inventories'].items() - ] + # Apply resource values to inventories + for r in parsed_args.resource: + name, field, value = parse_resource_argument(r) + inventories[name][field] = value + + try: + resources = http.request('PUT', url, json=payload).json() + except Exception as exp: + with excutils.save_and_reraise_exception() as err_ctx: + if parsed_args.aggregate: + self.log.error(_('Failed to set inventory for ' + 'resource provider %(rp)s: %(exp)s.'), + {'rp': rp['uuid'], 'exp': exp}) + err_ctx.reraise = False + ret += 1 + continue + resources_list.append((rp['uuid'], resources)) + + if ret > 0: + msg = _('Failed to set inventory for %(ret)s of %(total)s ' + 'resource providers.') % {'ret': ret, 'total': len(rps)} + raise exceptions.CommandError(msg) + + def get_rows(fields, resources, rp_uuid=None): + inventories = [ + dict(resource_class=k, **v) + for k, v in resources['inventories'].items() + ] + prepend = (rp_uuid, ) if rp_uuid else () + # This is a generator expression + rows = (prepend + utils.get_dict_properties(i, fields) + for i in inventories) + return rows fields = ('resource_class', ) + FIELDS - rows = (utils.get_dict_properties(i, fields) for i in inventories) - return fields, rows + if parsed_args.aggregate: + # If this is an aggregate batch, create output that will include + # resource provider as the first field to differentiate the values + rows = () + for rp_uuid, resources in resources_list: + subrows = get_rows(fields, resources, rp_uuid=rp_uuid) + rows = itertools.chain(rows, subrows) + fields = ('resource_provider', ) + fields + return fields, rows + else: + # If this was not an aggregate batch, show output for the one + # resource provider (show payload of the first item in the list), + # keeping the behavior prior to the addition of --aggregate option + return fields, get_rows(fields, resources_list[0][1]) class SetClassInventory(command.ShowOne): diff --git a/osc_placement/tests/functional/base.py b/osc_placement/tests/functional/base.py index ecc312d..8748cd4 100644 --- a/osc_placement/tests/functional/base.py +++ b/osc_placement/tests/functional/base.py @@ -262,10 +262,15 @@ class BaseTestCase(base.BaseTestCase): cmd += ' --resource-class ' + resource_class self.openstack(cmd) - def resource_inventory_set(self, uuid, *resources): - cmd = 'resource provider inventory set {uuid} {resources}'.format( - uuid=uuid, resources=' '.join( - ['--resource %s' % r for r in resources])) + def resource_inventory_set(self, uuid, *resources, **kwargs): + opts = [] + if kwargs.get('aggregate'): + opts.append('--aggregate') + fmt = 'resource provider inventory set {uuid} {resources} {opts}' + cmd = fmt.format( + uuid=uuid, + resources=' '.join(['--resource %s' % r for r in resources]), + opts=' '.join(opts)) return self.openstack(cmd, use_json=True) def resource_inventory_class_set(self, uuid, resource_class, **kwargs): diff --git a/osc_placement/tests/functional/test_inventory.py b/osc_placement/tests/functional/test_inventory.py index dc5902a..444dd54 100644 --- a/osc_placement/tests/functional/test_inventory.py +++ b/osc_placement/tests/functional/test_inventory.py @@ -10,6 +10,10 @@ # License for the specific language governing permissions and limitations # under the License. +from collections import defaultdict +import copy +import uuid + import six from osc_placement.tests.functional import base @@ -203,6 +207,14 @@ class TestSetInventory(base.BaseTestCase): self.assertEqual(16, inv['MEMORY_MB']['step_size']) self.assertEqual(32, inv['VCPU']['total']) + def test_fail_aggregate_arg_version_handling(self): + agg = str(uuid.uuid4()) + self.assertCommandFailed( + 'Operation or argument is not supported with version 1.0; ' + 'requires at least version 1.3', + self.resource_inventory_set, + agg, 'MEMORY_MB=16', aggregate=True) + class TestInventory15(TestInventory): VERSION = '1.5' @@ -212,3 +224,145 @@ class TestInventory15(TestInventory): self.resource_inventory_set(rp['uuid'], 'MEMORY_MB=16', 'VCPU=32') self.resource_inventory_delete(rp['uuid']) self.assertEqual([], self.resource_inventory_list(rp['uuid'])) + + +class TestAggregateInventory(base.BaseTestCase): + VERSION = '1.3' + + def _get_expected_inventories(self, old_inventories, resources): + new_inventories = [] + for old_inventory in old_inventories: + new_inventory = defaultdict(dict) + new_inventory.update(copy.deepcopy(old_inventory)) + for resource in resources: + rc, keyval = resource.split(':') + key, val = keyval.split('=') + # Handle allocation ratio which is a float + val = float(val) if '.' in val else int(val) + new_inventory[rc][key] = val + # The resource_class field is added by the osc_placement CLI, + # so add it to our expected inventories + if 'resource_class' not in new_inventory[rc]: + new_inventory[rc]['resource_class'] = rc + new_inventories.append(new_inventory) + return new_inventories + + def _setup_two_resource_providers_in_aggregate(self): + rps = [] + inventory2 = ['VCPU=8', + 'VCPU:max_unit=4', + 'VCPU:allocation_ratio=16.0', + 'MEMORY_MB=1024', + 'MEMORY_MB:reserved=256', + 'MEMORY_MB:allocation_ratio=2.5', + 'DISK_GB=16', + 'DISK_GB:allocation_ratio=1.5', + 'DISK_GB:min_unit=2', + 'DISK_GB:step_size=2'] + inventory1 = inventory2 + ['VGPU=8', + 'VGPU:allocation_ratio=1.0', + 'VGPU:min_unit=2', + 'VGPU:step_size=2'] + for i, inventory in enumerate([inventory1, inventory2]): + rps.append(self.resource_provider_create()) + resp = self.resource_inventory_set(rps[i]['uuid'], *inventory) + # Verify the resource_provider column is not present without + # --aggregate + self.assertNotIn('resource_provider', resp) + # Put both resource providers in the same aggregate + agg = str(uuid.uuid4()) + for rp in rps: + self.resource_provider_aggregate_set(rp['uuid'], agg) + return rps, agg + + def test_fail_if_no_rps_in_aggregate(self): + nonexistent_agg = str(uuid.uuid4()) + exc = self.assertRaises(base.CommandException, + self.resource_inventory_set, + nonexistent_agg, + 'VCPU=8', + aggregate=True) + self.assertIn('No resource providers found in aggregate with uuid {}' + .format(nonexistent_agg), six.text_type(exc)) + + def test_with_aggregate_one_fails(self): + # Set up some existing inventories with two resource providers + rps, agg = self._setup_two_resource_providers_in_aggregate() + # Set a custom resource class inventory on the first resource provider + self.resource_class_create('CUSTOM_FOO') + rp1_uuid = rps[0]['uuid'] + rp1_inv = self.resource_inventory_set(rp1_uuid, 'CUSTOM_FOO=1') + # Create an allocation for custom resource class on first provider + consumer = str(uuid.uuid4()) + alloc = 'rp=%s,CUSTOM_FOO=1' % rp1_uuid + self.resource_allocation_set(consumer, [alloc]) + # Try to set allocation ratio for an aggregate. The first set should + # fail because we're not going to set the custom resource class (which + # is equivalent to trying to remove it) and removal isn't allowed if + # there is an allocation of it present. The second set should succeed + new_resources = ['VCPU:allocation_ratio=5.0', 'VCPU:total=8'] + exc = self.assertRaises(base.CommandException, + self.resource_inventory_set, + agg, *new_resources, aggregate=True) + self.assertIn('Failed to set inventory for 1 of 2 resource providers.', + six.text_type(exc)) + output = self.output.getvalue() + self.error.getvalue() + err_txt = ("update conflict: Inventory for 'CUSTOM_FOO' on resource " + "provider '%s' in use. (HTTP 409)." % rp1_uuid) + self.assertIn('Failed to set inventory for resource provider %s: %s' % + (rp1_uuid, err_txt), output) + # Placement will default the following internally + placement_defaults = ['VCPU:max_unit=2147483647', + 'VCPU:min_unit=1', + 'VCPU:reserved=0', + 'VCPU:step_size=1'] + # Get expected inventory for the second resource provider (succeeded) + new_inventories = self._get_expected_inventories( + # Since inventories are expected to be fully replaced, + # use empty dict for old inventory + [{}], + new_resources + placement_defaults) + resp = self.resource_inventory_list(rps[1]['uuid']) + self.assertDictEqual(new_inventories[0], + {r['resource_class']: r for r in resp}) + # First resource provider should have remained the same (failed) + resp = self.resource_inventory_list(rp1_uuid) + self.assertDictEqual({r['resource_class']: r for r in rp1_inv}, + {r['resource_class']: r for r in resp}) + + def test_with_aggregate(self): + # Set up some existing inventories with two resource providers + rps, agg = self._setup_two_resource_providers_in_aggregate() + # Now, go ahead and update the allocation ratios and verify + new_resources = ['VCPU:allocation_ratio=5.0', + 'VCPU:total=8', + 'MEMORY_MB:allocation_ratio=6.0', + 'MEMORY_MB:total=1024', + 'DISK_GB:allocation_ratio=7.0', + 'DISK_GB:total=16'] + resp = self.resource_inventory_set(agg, *new_resources, aggregate=True) + # Verify the resource_provider column is present with --aggregate + for rp in resp: + self.assertIn('resource_provider', rp) + # Placement will default the following internally + placement_defaults = ['VCPU:max_unit=2147483647', + 'VCPU:min_unit=1', + 'VCPU:reserved=0', + 'VCPU:step_size=1', + 'MEMORY_MB:max_unit=2147483647', + 'MEMORY_MB:min_unit=1', + 'MEMORY_MB:reserved=0', + 'MEMORY_MB:step_size=1', + 'DISK_GB:max_unit=2147483647', + 'DISK_GB:min_unit=1', + 'DISK_GB:reserved=0', + 'DISK_GB:step_size=1'] + new_inventories = self._get_expected_inventories( + # Since inventories are expected to be fully replaced, + # use empty dicts for old inventories + [{}, {}], + new_resources + placement_defaults) + for i in range(2): + resp = self.resource_inventory_list(rps[i]['uuid']) + self.assertDictEqual(new_inventories[i], + {r['resource_class']: r for r in resp}) diff --git a/releasenotes/notes/resource-provider-inventory-set-aggregate-5f2239dd2685b636.yaml b/releasenotes/notes/resource-provider-inventory-set-aggregate-5f2239dd2685b636.yaml new file mode 100644 index 0000000..d0420c9 --- /dev/null +++ b/releasenotes/notes/resource-provider-inventory-set-aggregate-5f2239dd2685b636.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + A new ``--aggregate`` option has been added to the ``resource provider + inventory set`` command which can set resource provider inventory for + all resource providers that are members of the specified aggregate. + For example, VCPU, MEMORY_MB, and/or DISK_GB allocation ratios can be + managed in aggregate to resolve `bug 1804125`_. + + .. _bug 1804125: https://docs.openstack.org/nova/latest/admin/configuration/schedulers.html#bug-1804125 diff --git a/requirements.txt b/requirements.txt index c38135c..8f28cd4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,3 +7,4 @@ six>=1.10.0 # MIT keystoneauth1>=3.3.0 # Apache-2.0 simplejson>=3.16.0 # MIT osc-lib>=1.2.0 # Apache-2.0 +oslo.utils>=3.37.0 # Apache-2.0