From 307999c58101a5920e23ef181643d3b46bbf987c Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 18 Mar 2019 16:49:57 +0100 Subject: [PATCH] Prepare _heal_allocations_for_instance for nested allocations When no allocations exist for an instance the current heal code uses a report client call that can only handle allocations from a single RP. This call is now replaced with a more generic one so in a later patch port allocations can be added to this code path too. Related-Bug: #1819923 Change-Id: Ide343c1c922dac576b1944827dc24caefab59b74 --- nova/cmd/manage.py | 25 +++--- nova/scheduler/client/report.py | 30 ++----- nova/tests/functional/test_report_client.py | 25 ++++-- .../unit/scheduler/client/test_report.py | 83 ++++++++++++------- nova/tests/unit/test_nova_manage.py | 76 +++++++---------- 5 files changed, 119 insertions(+), 120 deletions(-) diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 6fec93233273..d615b56d9b0e 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -1674,10 +1674,16 @@ class PlacementCommands(object): {'instance': instance.uuid, 'node_uuid': node_uuid, 'resources': resources}) else: - if placement.put_allocations( - ctxt, node_uuid, instance.uuid, resources, - instance.project_id, instance.user_id, - consumer_generation=None): + payload = { + 'allocations': { + node_uuid: {'resources': resources}, + }, + 'project_id': instance.project_id, + 'user_id': instance.user_id, + 'consumer_generation': None + } + resp = placement.put_allocations(ctxt, instance.uuid, payload) + if resp: output(_('Successfully created allocations for ' 'instance %(instance)s against resource ' 'provider %(provider)s.') % @@ -1688,21 +1694,16 @@ class PlacementCommands(object): instance=instance.uuid, provider=node_uuid) def _heal_missing_project_and_user_id( - self, allocations, instance, dry_run, output, placement): + self, ctxt, allocations, instance, dry_run, output, placement): allocations['project_id'] = instance.project_id allocations['user_id'] = instance.user_id - # We use CONSUMER_GENERATION_VERSION for PUT - # /allocations/{consumer_id} to mirror the body structure from - # get_allocs_for_consumer. if dry_run: output(_('[dry-run] Update allocations for instance ' '%(instance)s: %(allocations)s') % {'instance': instance.uuid, 'allocations': allocations}) else: - resp = placement.put( - '/allocations/%s' % instance.uuid, - allocations, version=report.CONSUMER_GENERATION_VERSION) + resp = placement.put_allocations(ctxt, instance.uuid, allocations) if resp: output(_('Successfully updated allocations for ' 'instance %s.') % instance.uuid) @@ -1772,7 +1773,7 @@ class PlacementCommands(object): # because we don't want to mess up shared or nested # provider allocations. return self._heal_missing_project_and_user_id( - allocations, instance, dry_run, output, placement) + ctxt, allocations, instance, dry_run, output, placement) output(_('Instance %s already has allocations with ' 'matching consumer project/user.') % instance.uuid) diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index d91557795c06..8dcfadf8d540 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -1930,27 +1930,17 @@ class SchedulerReportClient(object): 'text': r.text}) return r.status_code == 204 + # TODO(gibi): kill safe_connect @safe_connect @retries - def put_allocations(self, context, rp_uuid, consumer_uuid, alloc_data, - project_id, user_id, consumer_generation): - """Creates allocation records for the supplied instance UUID against - the supplied resource provider. - - :note Currently we only allocate against a single resource provider. - Once shared storage and things like NUMA allocations are a - reality, this will change to allocate against multiple providers. + def put_allocations(self, context, consumer_uuid, payload): + """Creates allocation records for the supplied consumer UUID based on + the provided allocation dict :param context: The security context - :param rp_uuid: The UUID of the resource provider to allocate against. :param consumer_uuid: The instance's UUID. - :param alloc_data: Dict, keyed by resource class, of amounts to - consume. - :param project_id: The project_id associated with the allocations. - :param user_id: The user_id associated with the allocations. - :param consumer_generation: The current generation of the consumer or - None if this the initial allocation of the - consumer + :param payload: Dict in the format expected by the placement + PUT /allocations/{consumer_uuid} API :returns: True if the allocations were created, False otherwise. :raises: Retry if the operation should be retried due to a concurrent resource provider update. @@ -1958,14 +1948,6 @@ class SchedulerReportClient(object): generation conflict """ - payload = { - 'allocations': { - rp_uuid: {'resources': alloc_data}, - }, - 'project_id': project_id, - 'user_id': user_id, - 'consumer_generation': consumer_generation - } r = self._put_allocations(context, consumer_uuid, payload) if r.status_code != 204: err = r.json()['errors'][0] diff --git a/nova/tests/functional/test_report_client.py b/nova/tests/functional/test_report_client.py index 03467242a828..1184765b3695 100644 --- a/nova/tests/functional/test_report_client.py +++ b/nova/tests/functional/test_report_client.py @@ -243,10 +243,16 @@ class SchedulerReportClientTests(SchedulerReportClientTestBase): # Update allocations with our instance alloc_dict = utils.resources_from_flavor(self.instance, self.instance.flavor) + payload = { + "allocations": { + self.compute_uuid: {"resources": alloc_dict} + }, + "project_id": self.instance.project_id, + "user_id": self.instance.user_id, + "consumer_generation": None + } self.client.put_allocations( - self.context, self.compute_uuid, self.instance_uuid, - alloc_dict, self.instance.project_id, self.instance.user_id, - None) + self.context, self.instance_uuid, payload) # Check that allocations were made resp = self.client.get('/allocations/%s' % self.instance_uuid) @@ -685,13 +691,18 @@ class SchedulerReportClientTests(SchedulerReportClientTestBase): inv, self.client._get_inventory( self.context, uuids.cn)['inventories']) - + payload = { + "allocations": { + uuids.cn: {"resources": {orc.SRIOV_NET_VF: 1}} + }, + "project_id": uuids.proj, + "user_id": uuids.user, + "consumer_generation": None + } # Now set up an InventoryInUse case by creating a VF allocation... self.assertTrue( self.client.put_allocations( - self.context, uuids.cn, uuids.consumer, - {orc.SRIOV_NET_VF: 1}, - uuids.proj, uuids.user, None)) + self.context, uuids.consumer, payload)) # ...and trying to delete the provider's VF inventory bad_inv = { 'CUSTOM_BANDWIDTH': { diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index 4cbdfecb0e19..bcd060e967ee 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -269,14 +269,19 @@ class TestPutAllocations(SchedulerReportClientTestCase): consumer_uuid = mock.sentinel.consumer data = {"MEMORY_MB": 1024} expected_url = "/allocations/%s" % consumer_uuid - resp = self.client.put_allocations(self.context, rp_uuid, - consumer_uuid, data, - mock.sentinel.project_id, - mock.sentinel.user_id, - mock.sentinel.consumer_generation) + payload = { + "allocations": { + rp_uuid: {"resources": data} + }, + "project_id": mock.sentinel.project_id, + "user_id": mock.sentinel.user_id, + "consumer_generation": mock.sentinel.consumer_generation + } + resp = self.client.put_allocations( + self.context, consumer_uuid, payload) self.assertTrue(resp) mock_put.assert_called_once_with( - expected_url, mock.ANY, version='1.28', + expected_url, payload, version='1.28', global_request_id=self.context.global_id) @mock.patch.object(report.LOG, 'warning') @@ -288,14 +293,20 @@ class TestPutAllocations(SchedulerReportClientTestCase): consumer_uuid = mock.sentinel.consumer data = {"MEMORY_MB": 1024} expected_url = "/allocations/%s" % consumer_uuid - resp = self.client.put_allocations(self.context, rp_uuid, - consumer_uuid, data, - mock.sentinel.project_id, - mock.sentinel.user_id, - mock.sentinel.consumer_generation) + payload = { + "allocations": { + rp_uuid: {"resources": data} + }, + "project_id": mock.sentinel.project_id, + "user_id": mock.sentinel.user_id, + "consumer_generation": mock.sentinel.consumer_generation + } + resp = self.client.put_allocations( + self.context, consumer_uuid, payload) + self.assertFalse(resp) mock_put.assert_called_once_with( - expected_url, mock.ANY, version='1.28', + expected_url, payload, version='1.28', global_request_id=self.context.global_id) log_msg = mock_warn.call_args[0][0] self.assertIn("Failed to save allocation for", log_msg) @@ -313,13 +324,17 @@ class TestPutAllocations(SchedulerReportClientTestCase): consumer_uuid = mock.sentinel.consumer data = {"MEMORY_MB": 1024} expected_url = "/allocations/%s" % consumer_uuid + payload = { + "allocations": { + rp_uuid: {"resources": data} + }, + "project_id": mock.sentinel.project_id, + "user_id": mock.sentinel.user_id, + "consumer_generation": mock.sentinel.consumer_generation + } self.assertRaises(exception.AllocationUpdateFailed, self.client.put_allocations, - self.context, rp_uuid, - consumer_uuid, data, - mock.sentinel.project_id, - mock.sentinel.user_id, - mock.sentinel.consumer_generation) + self.context, consumer_uuid, payload) mock_put.assert_called_once_with( expected_url, mock.ANY, version='1.28', @@ -343,14 +358,19 @@ class TestPutAllocations(SchedulerReportClientTestCase): consumer_uuid = mock.sentinel.consumer data = {"MEMORY_MB": 1024} expected_url = "/allocations/%s" % consumer_uuid - resp = self.client.put_allocations(self.context, rp_uuid, - consumer_uuid, data, - mock.sentinel.project_id, - mock.sentinel.user_id, - mock.sentinel.consumer_generation) + payload = { + "allocations": { + rp_uuid: {"resources": data} + }, + "project_id": mock.sentinel.project_id, + "user_id": mock.sentinel.user_id, + "consumer_generation": mock.sentinel.consumer_generation + } + resp = self.client.put_allocations( + self.context, consumer_uuid, payload) self.assertTrue(resp) mock_put.assert_has_calls([ - mock.call(expected_url, mock.ANY, version='1.28', + mock.call(expected_url, payload, version='1.28', global_request_id=self.context.global_id)] * 2) @mock.patch('time.sleep', new=mock.Mock()) @@ -369,14 +389,19 @@ class TestPutAllocations(SchedulerReportClientTestCase): consumer_uuid = mock.sentinel.consumer data = {"MEMORY_MB": 1024} expected_url = "/allocations/%s" % consumer_uuid - resp = self.client.put_allocations(self.context, rp_uuid, - consumer_uuid, data, - mock.sentinel.project_id, - mock.sentinel.user_id, - mock.sentinel.consumer_generation) + payload = { + "allocations": { + rp_uuid: {"resources": data} + }, + "project_id": mock.sentinel.project_id, + "user_id": mock.sentinel.user_id, + "consumer_generation": mock.sentinel.consumer_generation + } + resp = self.client.put_allocations( + self.context, consumer_uuid, payload) self.assertFalse(resp) mock_put.assert_has_calls([ - mock.call(expected_url, mock.ANY, version='1.28', + mock.call(expected_url, payload, version='1.28', global_request_id=self.context.global_id)] * 3) def test_claim_resources_success(self): diff --git a/nova/tests/unit/test_nova_manage.py b/nova/tests/unit/test_nova_manage.py index 3651d91c7f69..fe5524fcf155 100644 --- a/nova/tests/unit/test_nova_manage.py +++ b/nova/tests/unit/test_nova_manage.py @@ -21,6 +21,7 @@ import ddt import fixtures import mock from oslo_db import exception as db_exc +from oslo_serialization import jsonutils from oslo_utils.fixture import uuidsentinel from oslo_utils import uuidutils from six.moves import StringIO @@ -2469,8 +2470,9 @@ class TestNovaManagePlacement(test.NoDBTestCase): return_value=objects.ComputeNode(uuid=uuidsentinel.node)) @mock.patch('nova.scheduler.utils.resources_from_flavor', return_value=mock.sentinel.resources) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'put_allocations', return_value=False) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.put', + return_value=fake_requests.FakeResponse( + 500, content=jsonutils.dumps({"errors": [{"code": ""}]}))) def test_heal_allocations_put_allocations_fails( self, mock_put_allocations, mock_res_from_flavor, mock_get_compute_node, mock_get_allocs, mock_get_instances, @@ -2481,46 +2483,20 @@ class TestNovaManagePlacement(test.NoDBTestCase): instance = mock_get_instances.return_value[0] mock_res_from_flavor.assert_called_once_with( instance, instance.flavor) - mock_put_allocations.assert_called_once_with( - test.MatchType(context.RequestContext), uuidsentinel.node, - uuidsentinel.instance, mock.sentinel.resources, 'fake-project', - 'fake-user', consumer_generation=None) - @mock.patch('nova.objects.CellMappingList.get_all', - return_value=objects.CellMappingList(objects=[ - objects.CellMapping(name='cell1', - uuid=uuidsentinel.cell1)])) - @mock.patch('nova.objects.InstanceList.get_by_filters', - return_value=objects.InstanceList(objects=[ - objects.Instance( - uuid=uuidsentinel.instance, host='fake', node='fake', - task_state=None, flavor=objects.Flavor(), - project_id='fake-project', user_id='fake-user')])) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'get_allocs_for_consumer', return_value={}) - @mock.patch('nova.objects.ComputeNode.get_by_host_and_nodename', - return_value=objects.ComputeNode(uuid=uuidsentinel.node)) - @mock.patch('nova.scheduler.utils.resources_from_flavor', - return_value=mock.sentinel.resources) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'put_allocations', - side_effect=exception.AllocationUpdateFailed( - consumer_uuid=uuidsentinel.instance, - error="consumer generation conflict")) - def test_heal_allocations_put_allocations_fails_with_consumer_conflict( - self, mock_put_allocations, mock_res_from_flavor, - mock_get_compute_node, mock_get_allocs, mock_get_instances, - mock_get_all_cells): - self.assertEqual(3, self.cli.heal_allocations()) - self.assertIn('Failed to update allocations for consumer', - self.output.getvalue()) - instance = mock_get_instances.return_value[0] - mock_res_from_flavor.assert_called_once_with( - instance, instance.flavor) + expected_payload = { + 'allocations': { + uuidsentinel.node: { + 'resources': mock.sentinel.resources + } + }, + 'user_id': 'fake-user', + 'project_id': 'fake-project', + 'consumer_generation': None + } mock_put_allocations.assert_called_once_with( - test.MatchType(context.RequestContext), uuidsentinel.node, - uuidsentinel.instance, mock.sentinel.resources, 'fake-project', - 'fake-user', consumer_generation=None) + '/allocations/%s' % instance.uuid, expected_payload, + global_request_id=mock.ANY, version='1.28') @mock.patch('nova.objects.CellMappingList.get_all', new=mock.Mock(return_value=objects.CellMappingList(objects=[ @@ -2560,8 +2536,8 @@ class TestNovaManagePlacement(test.NoDBTestCase): return_value=objects.ComputeNode(uuid=uuidsentinel.node))) @mock.patch('nova.scheduler.utils.resources_from_flavor', new=mock.Mock(return_value=mock.sentinel.resources)) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'put_allocations', return_value=True) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.put', + return_value=fake_requests.FakeResponse(204)) def test_heal_allocations_get_allocs_retrieval_fails(self, mock_put, mock_getinst): # This "succeeds" @@ -2612,7 +2588,8 @@ class TestNovaManagePlacement(test.NoDBTestCase): } }, "project_id": uuidsentinel.project_id, - "user_id": uuidsentinel.user_id + "user_id": uuidsentinel.user_id, + "consumer_generation": 12, } self.assertEqual(0, self.cli.heal_allocations(verbose=True)) self.assertIn('Processed 1 instances.', self.output.getvalue()) @@ -2623,7 +2600,7 @@ class TestNovaManagePlacement(test.NoDBTestCase): expected_put_data['user_id'] = 'fake-user' mock_put.assert_called_once_with( '/allocations/%s' % uuidsentinel.instance, expected_put_data, - version='1.28') + global_request_id=mock.ANY, version='1.28') @mock.patch('nova.objects.CellMappingList.get_all', return_value=objects.CellMappingList(objects=[ @@ -2639,8 +2616,11 @@ class TestNovaManagePlacement(test.NoDBTestCase): 'get_allocs_for_consumer') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.put', return_value=fake_requests.FakeResponse( - 409, content='Inventory and/or allocations changed while ' - 'attempting to allocate')) + 409, + content=jsonutils.dumps( + {"errors": [ + {"code": "placement.concurrent_update", + "detail": "consumer generation conflict"}]}))) def test_heal_allocations_put_fails( self, mock_put, mock_get_allocs, mock_get_instances, mock_get_all_cells): @@ -2666,7 +2646,7 @@ class TestNovaManagePlacement(test.NoDBTestCase): } self.assertEqual(3, self.cli.heal_allocations(verbose=True)) self.assertIn( - 'Inventory and/or allocations changed', self.output.getvalue()) + 'consumer generation conflict', self.output.getvalue()) mock_get_allocs.assert_called_once_with( test.MatchType(context.RequestContext), uuidsentinel.instance) expected_put_data = mock_get_allocs.return_value @@ -2674,7 +2654,7 @@ class TestNovaManagePlacement(test.NoDBTestCase): expected_put_data['user_id'] = 'fake-user' mock_put.assert_called_once_with( '/allocations/%s' % uuidsentinel.instance, expected_put_data, - version='1.28') + global_request_id=mock.ANY, version='1.28') @mock.patch('nova.compute.api.AggregateAPI.get_aggregate_list', return_value=objects.AggregateList(objects=[