From 02a1551f6455792ee050d6009c94514402b7df04 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Wed, 31 Jan 2018 18:06:17 -0600 Subject: [PATCH] Ensure resource classes correctly Previously, SchedulerReportClient.set_inventory_for_provider used this logic to pre-create custom resource classes found in the input inventory. list(map(self._ensure_resource_class, (rc_name for rc_name in inv_data if rc_name not in fields.ResourceClass.STANDARD))) ...where _ensure_resource_class would always attempt to create the resource class it was given, and raise an exception if that failed for any reason. Note in particular that attempting to create an already-extant "standard" resource class will fail just the same as attempting to create any nonexistent resource class that doesn't conform to the schema (i.e. beginning with "CUSTOM_"). The problem is that this relies on the local system's notion of the set of standard resource classes. If the placement service is running newer code, standard resource classes may have been added that the compute service doesn't know about yet. This change set solves the problem by only attempting to create resource classes with the 'CUSTOM_' prefix. self._ensure_resource_class (which worked on a single resource class) is replaced with self._ensure_resource_classes (plural) which accepts a list of *all* the desired resource classes (including the standard ones), filters down to the CUSTOM_* ones, and attempts to create the remainder. Note that if placement ever decides to start allowing creation of resource classes with prefixes other than CUSTOM_, it will have to do so under a new microversion, so we can't future-proof this by e.g. prefetching all the known resource classes from placement and attempting to create any not found in that list. In the process of doing this rework, obsolete code paths relying on pre-1.7 placement microversions are removed. Change-Id: I600ed262e1b5d11facbf461e28291193665280cf Closes-Bug: #1746615 --- nova/scheduler/client/report.py | 58 ++++++++++--------- .../openstack/placement/test_report_client.py | 16 +---- .../unit/scheduler/client/test_report.py | 53 ++++++++++++++--- 3 files changed, 79 insertions(+), 48 deletions(-) diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index 4e29fc6963ea..8543aa9e55af 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -1044,9 +1044,7 @@ class SchedulerReportClient(object): parent_provider_uuid=parent_provider_uuid) # Auto-create custom resource classes coming from a virt driver - for rc_name in inv_data: - if rc_name not in fields.ResourceClass.STANDARD: - self._ensure_resource_class(context, rc_name) + self._ensure_resource_classes(context, set(inv_data)) if inv_data: self._update_inventory(context, rp_uuid, inv_data) @@ -1201,34 +1199,38 @@ class SchedulerReportClient(object): raise exception.ResourceProviderUpdateFailed(url=url, error=resp.text) @safe_connect - def _ensure_resource_class(self, context, name): - """Make sure a custom resource class exists. - - PUT the resource class using microversion 1.7. - - Returns the name of the resource class if it was successfully - created or already exists. Otherwise None. + def _ensure_resource_classes(self, context, names): + """Make sure resource classes exist. :param context: The security context - :param name: String name of the resource class to check/create. - :raises: `exception.InvalidResourceClass` upon error. + :param names: Iterable of string names of the resource classes to + check/create. Must not be None. + :raises: exception.InvalidResourceClass if an attempt is made to create + an invalid resource class. """ - # no payload on the put request - response = self.put("/resource_classes/%s" % name, None, version="1.7", - global_request_id=context.global_id) - if 200 <= response.status_code < 300: - return name - else: - msg = ("Failed to ensure resource class record with placement API " - "for resource class %(rc_name)s. Got %(status_code)d: " - "%(err_text)s.") - args = { - 'rc_name': name, - 'status_code': response.status_code, - 'err_text': response.text, - } - LOG.error(msg, args) - raise exception.InvalidResourceClass(resource_class=name) + # Placement API version that supports PUT /resource_classes/CUSTOM_* + # to create (or validate the existence of) a consumer-specified + # resource class. + version = '1.7' + to_ensure = set(n for n in names + if n.startswith(fields.ResourceClass.CUSTOM_NAMESPACE)) + + for name in to_ensure: + # no payload on the put request + resp = self.put( + "/resource_classes/%s" % name, None, version=version, + global_request_id=context.global_id) + if not resp: + msg = ("Failed to ensure resource class record with placement " + "API for resource class %(rc_name)s. Got " + "%(status_code)d: %(err_text)s.") + args = { + 'rc_name': name, + 'status_code': resp.status_code, + 'err_text': resp.text, + } + LOG.error(msg, args) + raise exception.InvalidResourceClass(resource_class=name) def update_compute_node(self, context, compute_node): """Creates or updates stats for the supplied compute node. diff --git a/nova/tests/functional/api/openstack/placement/test_report_client.py b/nova/tests/functional/api/openstack/placement/test_report_client.py index 93b925c79a7a..056ab655195b 100644 --- a/nova/tests/functional/api/openstack/placement/test_report_client.py +++ b/nova/tests/functional/api/openstack/placement/test_report_client.py @@ -202,7 +202,7 @@ class SchedulerReportClientTests(test.TestCase): # Try setting some invalid inventory and make sure the report # client raises the expected error. inv_data = { - 'BAD_FOO': { + 'CUSTOM_BOGU$_CLA$$': { 'total': 100, 'reserved': 0, 'min_unit': 1, @@ -274,18 +274,8 @@ class SchedulerReportClientTests(test.TestCase): } with interceptor.RequestsInterceptor(app=self.app, url=self.url): self.client.update_compute_node(self.context, self.compute_node) - # Simulate that our locally-running code has an outdated notion of - # standard resource classes. - with mock.patch.object(fields.ResourceClass, 'STANDARD', - ('VCPU', 'MEMORY_MB', 'DISK_GB')): - # TODO(efried): Once bug #1746615 is fixed, this will no longer - # raise, and can be replaced with: - # self.client.set_inventory_for_provider( - # self.context, self.compute_uuid, self.compute_name, inv) - self.assertRaises( - exception.InvalidResourceClass, - self.client.set_inventory_for_provider, - self.context, self.compute_uuid, self.compute_name, inv) + self.client.set_inventory_for_provider( + self.context, self.compute_uuid, self.compute_name, inv) @mock.patch('keystoneauth1.session.Session.get_endpoint', return_value='http://localhost:80/placement') diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index 6e2e3c1b6cdc..8d031addeb32 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -3059,7 +3059,7 @@ There was a conflict when trying to complete your request. @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_delete_inventory') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_ensure_resource_class') + '_ensure_resource_classes') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_ensure_resource_provider') def test_set_inventory_for_provider_no_custom(self, mock_erp, mock_erc, @@ -3106,7 +3106,8 @@ There was a conflict when trying to complete your request. parent_provider_uuid=None, ) # No custom resource classes to ensure... - self.assertFalse(mock_erc.called) + mock_erc.assert_called_once_with(self.context, + set(['VCPU', 'MEMORY_MB', 'DISK_GB'])) mock_upd.assert_called_once_with( self.context, mock.sentinel.rp_uuid, @@ -3119,7 +3120,7 @@ There was a conflict when trying to complete your request. @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_delete_inventory') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_ensure_resource_class') + '_ensure_resource_classes') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_ensure_resource_provider') def test_set_inventory_for_provider_no_inv(self, mock_erp, mock_erc, @@ -3140,7 +3141,7 @@ There was a conflict when trying to complete your request. mock.sentinel.rp_name, parent_provider_uuid=None, ) - self.assertFalse(mock_erc.called) + mock_erc.assert_called_once_with(self.context, set()) self.assertFalse(mock_upd.called) mock_del.assert_called_once_with(self.context, mock.sentinel.rp_uuid) @@ -3149,7 +3150,7 @@ There was a conflict when trying to complete your request. @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_delete_inventory') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_ensure_resource_class') + '_ensure_resource_classes') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_ensure_resource_provider') def test_set_inventory_for_provider_with_custom(self, mock_erp, @@ -3205,7 +3206,9 @@ There was a conflict when trying to complete your request. mock.sentinel.rp_name, parent_provider_uuid=None, ) - mock_erc.assert_called_once_with(self.context, 'CUSTOM_IRON_SILVER') + mock_erc.assert_called_once_with( + self.context, + set(['VCPU', 'MEMORY_MB', 'DISK_GB', 'CUSTOM_IRON_SILVER'])) mock_upd.assert_called_once_with( self.context, mock.sentinel.rp_uuid, @@ -3216,7 +3219,7 @@ There was a conflict when trying to complete your request. @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_delete_inventory', new=mock.Mock()) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_ensure_resource_class', new=mock.Mock()) + '_ensure_resource_classes', new=mock.Mock()) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_ensure_resource_provider') def test_set_inventory_for_provider_with_parent(self, mock_erp): @@ -3526,3 +3529,39 @@ class TestAllocations(SchedulerReportClientTestCase): # With a 409, only the error should be called self.assertEqual(0, mock_log.info.call_count) self.assertEqual(1, mock_log.error.call_count) + + +class TestResourceClass(SchedulerReportClientTestCase): + def setUp(self): + super(TestResourceClass, self).setUp() + _put_patch = mock.patch( + "nova.scheduler.client.report.SchedulerReportClient.put") + self.addCleanup(_put_patch.stop) + self.mock_put = _put_patch.start() + + def test_ensure_resource_classes(self): + rcs = ['VCPU', 'CUSTOM_FOO', 'MEMORY_MB', 'CUSTOM_BAR'] + self.client._ensure_resource_classes(self.context, rcs) + self.mock_put.assert_has_calls([ + mock.call('/resource_classes/%s' % rc, None, version='1.7', + global_request_id=self.context.global_id) + for rc in ('CUSTOM_FOO', 'CUSTOM_BAR') + ], any_order=True) + + def test_ensure_resource_classes_none(self): + for empty in ([], (), set(), {}): + self.client._ensure_resource_classes(self.context, empty) + self.mock_put.assert_not_called() + + def test_ensure_resource_classes_put_fail(self): + resp = requests.Response() + resp.status_code = 503 + self.mock_put.return_value = resp + rcs = ['VCPU', 'MEMORY_MB', 'CUSTOM_BAD'] + self.assertRaises( + exception.InvalidResourceClass, + self.client._ensure_resource_classes, self.context, rcs) + # Only called with the "bad" one + self.mock_put.assert_called_once_with( + '/resource_classes/CUSTOM_BAD', None, version='1.7', + global_request_id=self.context.global_id)