From 86749004bb0733f6b8b902209db6519ca580633a Mon Sep 17 00:00:00 2001 From: Rafael Folco Date: Mon, 24 Apr 2017 18:39:49 +0000 Subject: [PATCH] Delete all inventory has its own method DELETE Previously we deleted all inventories for a resource provider with a PUT method and empty payload. With bp delete-inventories-placement-api implemented, scheduler's client report now can use DELETE instead of PUT. To keep it consistent across microversions, it should fallback to put method if the delete attempt returns 406. Change-Id: I626ca4a821b5ab0ebcaa79937a31771fbae5b8d9 --- nova/scheduler/client/report.py | 93 ++++++--- .../unit/scheduler/client/test_report.py | 179 ++++++++++++++++-- 2 files changed, 220 insertions(+), 52 deletions(-) diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index 7b45d4dd0df..9c8f60c8dea 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -241,10 +241,18 @@ def put(self, url, data): url, json=data, endpoint_filter=self.ks_filter, raise_exc=False) - def delete(self, url): + def delete(self, url, version=None): + kwargs = {} + if version is not None: + # TODO(mriedem): Perform some version discovery at some point. + kwargs = { + 'headers': { + 'OpenStack-API-Version': 'placement %s' % version + }, + } return self._client.delete( url, - endpoint_filter=self.ks_filter, raise_exc=False) + endpoint_filter=self.ks_filter, raise_exc=False, **kwargs) # TODO(sbauza): Change that poor interface into passing a rich versioned # object that would provide the ResourceProvider requirements. @@ -602,6 +610,9 @@ def _update_inventory(self, rp_uuid, inv_data): def _delete_inventory(self, rp_uuid): """Deletes all inventory records for a resource provider with the supplied UUID. + + First attempt to DELETE the inventory using microversion 1.5. If + this results in a 406, fail over to a PUT. """ curr = self._get_inventory_and_update_provider_generation(rp_uuid) @@ -617,50 +628,70 @@ def _delete_inventory(self, rp_uuid): LOG.info(msg, rp_uuid) url = '/resource_providers/%s/inventories' % rp_uuid + r = self.delete(url, version="1.5") + placement_req_id = get_placement_request_id(r) cur_rp_gen = self._resource_providers[rp_uuid]['generation'] - payload = { - 'resource_provider_generation': cur_rp_gen, - 'inventories': {}, + msg_args = { + 'rp_uuid': rp_uuid, + 'placement_req_id': placement_req_id, } - r = self.put(url, payload) - placement_req_id = get_placement_request_id(r) - if r.status_code == 200: - # Update our view of the generation for next time - updated_inv = r.json() - new_gen = updated_inv['resource_provider_generation'] - - self._resource_providers[rp_uuid]['generation'] = new_gen - msg_args = { - 'rp_uuid': rp_uuid, - 'generation': new_gen, - 'placement_req_id': placement_req_id, + if r.status_code == 406: + # microversion 1.5 not available so try the earlier way + # TODO(cdent): When we're happy that all placement + # servers support microversion 1.5 we can remove this + # call and the associated code. + LOG.debug('Falling back to placement API microversion 1.0 ' + 'for deleting all inventory for a resource provider.') + payload = { + 'resource_provider_generation': cur_rp_gen, + 'inventories': {}, } - LOG.info(_LI('[%(placement_req_id)s] Deleted all inventory for ' - 'resource provider %(rp_uuid)s at generation ' - '%(generation)i'), + r = self.put(url, payload) + placement_req_id = get_placement_request_id(r) + msg_args['placement_req_id'] = placement_req_id + if r.status_code == 200: + # Update our view of the generation for next time + updated_inv = r.json() + new_gen = updated_inv['resource_provider_generation'] + + self._resource_providers[rp_uuid]['generation'] = new_gen + msg_args['generation'] = new_gen + LOG.info(_LI("[%(placement_req_id)s] Deleted all inventory " + "for resource provider %(rp_uuid)s at generation " + "%(generation)i."), + msg_args) + return + + if r.status_code == 204: + self._resource_providers[rp_uuid]['generation'] = cur_rp_gen + 1 + LOG.info(_LI("[%(placement_req_id)s] Deleted all inventory for " + "resource provider %(rp_uuid)s."), msg_args) return + elif r.status_code == 404: + # This can occur if another thread deleted the inventory and the + # resource provider already + LOG.debug("[%(placement_req_id)s] Resource provider %(rp_uuid)s " + "deleted by another thread when trying to delete " + "inventory. Ignoring.", + msg_args) + self._resource_providers.pop(rp_uuid, None) + self._provider_aggregate_map.pop(rp_uuid, None) + return elif r.status_code == 409: rc_str = _extract_inventory_in_use(r.text) if rc_str is not None: msg = _LW("[%(placement_req_id)s] We cannot delete inventory " "%(rc_str)s for resource provider %(rp_uuid)s " "because the inventory is in use.") - msg_args = { - 'rp_uuid': rp_uuid, - 'rc_str': rc_str, - 'placement_req_id': placement_req_id, - } + msg_args['rc_str'] = rc_str LOG.warning(msg, msg_args) return msg = _LE("[%(placement_req_id)s] Failed to delete inventory for " - "resource provider %(rp_uuid)s. Got error response: %(err)s") - msg_args = { - 'rp_uuid': rp_uuid, - 'err': r.text, - 'placement_req_id': placement_req_id, - } + "resource provider %(rp_uuid)s. Got error response: " + "%(err)s.") + msg_args['err'] = r.text LOG.error(msg, msg_args) def set_inventory_for_provider(self, rp_uuid, rp_name, inv_data): diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index ab9e0a38803..f64c1f94fe3 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -701,13 +701,16 @@ def test_update_compute_node_no_inv(self, mock_ui, mock_delete, mock_delete.assert_called_once_with(cn.uuid) self.assertFalse(mock_ui.called) - @mock.patch('nova.scheduler.client.report._extract_inventory_in_use') + @mock.patch.object(report.LOG, 'info') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'put') + @mock.patch.object(report.LOG, 'info') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'get') - def test_delete_inventory_already_no_inventory(self, mock_get, mock_put, - mock_extract): + def test_delete_inventory(self, mock_get, mock_delete, mock_debug, + mock_put, mock_info): cn = self.compute_node rp = dict(uuid=cn.uuid, generation=42) # Make sure the resource provider exists for preventing to call the API @@ -716,23 +719,52 @@ def test_delete_inventory_already_no_inventory(self, mock_get, mock_put, mock_get.return_value.json.return_value = { 'resource_provider_generation': 1, 'inventories': { + 'VCPU': {'total': 16}, } } + mock_delete.return_value.status_code = 204 + mock_delete.return_value.headers = {'openstack-request-id': + uuids.request_id} result = self.client._delete_inventory(cn.uuid) self.assertIsNone(result) self.assertFalse(mock_put.called) + self.assertEqual(uuids.request_id, + mock_info.call_args[0][1]['placement_req_id']) + + @mock.patch('nova.scheduler.client.report._extract_inventory_in_use') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'get') + def test_delete_inventory_already_no_inventory(self, mock_get, mock_delete, + mock_extract): + cn = self.compute_node + rp = dict(uuid=cn.uuid, generation=42) + # Make sure the resource provider exists for preventing to call the API + self.client._resource_providers[cn.uuid] = rp + + mock_get.return_value.json.return_value = { + 'resource_provider_generation': 1, + 'inventories': { + } + } + result = self.client._delete_inventory(cn.uuid) + self.assertIsNone(result) + self.assertFalse(mock_delete.called) self.assertFalse(mock_extract.called) new_gen = self.client._resource_providers[cn.uuid]['generation'] self.assertEqual(1, new_gen) @mock.patch.object(report.LOG, 'info') - @mock.patch('nova.scheduler.client.report._extract_inventory_in_use') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'put') + @mock.patch.object(report.LOG, 'debug') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'get') - def test_delete_inventory(self, mock_get, mock_put, mock_extract, - mock_info): + def test_delete_inventory_put(self, mock_get, mock_delete, mock_debug, + mock_put, mock_info): cn = self.compute_node rp = dict(uuid=cn.uuid, generation=42) # Make sure the resource provider exists for preventing to call the API @@ -741,11 +773,10 @@ def test_delete_inventory(self, mock_get, mock_put, mock_extract, mock_get.return_value.json.return_value = { 'resource_provider_generation': 1, 'inventories': { - 'VCPU': {'total': 16}, - 'MEMORY_MB': {'total': 1024}, 'DISK_GB': {'total': 10}, } } + mock_delete.return_value.status_code = 406 mock_put.return_value.status_code = 200 mock_put.return_value.json.return_value = { 'resource_provider_generation': 44, @@ -756,19 +787,88 @@ def test_delete_inventory(self, mock_get, mock_put, mock_extract, uuids.request_id} result = self.client._delete_inventory(cn.uuid) self.assertIsNone(result) - self.assertFalse(mock_extract.called) + self.assertTrue(mock_debug.called) + self.assertTrue(mock_put.called) new_gen = self.client._resource_providers[cn.uuid]['generation'] self.assertEqual(44, new_gen) self.assertTrue(mock_info.called) self.assertEqual(uuids.request_id, mock_info.call_args[0][1]['placement_req_id']) - @mock.patch.object(report.LOG, 'warning') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'put') + @mock.patch.object(report.LOG, 'debug') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'get') - def test_delete_inventory_inventory_in_use(self, mock_get, mock_put, + def test_delete_inventory_put_failover(self, mock_get, mock_delete, + mock_debug, mock_put): + cn = self.compute_node + rp = dict(uuid=cn.uuid, generation=42) + # Make sure the resource provider exists for preventing to call the API + self.client._resource_providers[cn.uuid] = rp + + mock_get.return_value.json.return_value = { + 'resource_provider_generation': 42, + 'inventories': { + 'DISK_GB': {'total': 10}, + } + } + mock_delete.return_value.status_code = 406 + mock_put.return_value.status_code = 200 + self.client._delete_inventory(cn.uuid) + self.assertTrue(mock_debug.called) + exp_url = '/resource_providers/%s/inventories' % rp['uuid'] + payload = { + 'resource_provider_generation': 42, + 'inventories': {}, + } + mock_put.assert_called_once_with(exp_url, payload) + + @mock.patch.object(report.LOG, 'error') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'put') + @mock.patch.object(report.LOG, 'debug') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') + def test_delete_inventory_put_failover_in_use(self, mock_delete, + mock_debug, mock_put, + mock_error): + cn = self.compute_node + rp = dict(uuid=cn.uuid, generation=42) + # Make sure the resource provider exists for preventing to call the API + self.client._resource_providers[cn.uuid] = rp + mock_delete.return_value.status_code = 406 + mock_put.return_value.status_code = 409 + mock_put.return_value.text = ( + 'There was a *fake* failure: inventory in use' + ) + mock_put.return_value.json.return_value = { + 'resource_provider_generation': 44, + 'inventories': { + } + } + mock_put.return_value.headers = {'openstack-request-id': + uuids.request_id} + self.client._delete_inventory(cn.uuid) + self.assertTrue(mock_debug.called) + exp_url = '/resource_providers/%s/inventories' % cn.uuid + payload = { + 'resource_provider_generation': rp['generation'], + 'inventories': {}, + } + mock_put.assert_called_once_with(exp_url, payload) + self.assertTrue(mock_error.called) + self.assertEqual(uuids.request_id, + mock_error.call_args[0][1]['placement_req_id']) + + @mock.patch.object(report.LOG, 'warning') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'get') + def test_delete_inventory_inventory_in_use(self, mock_get, mock_delete, mock_warn): cn = self.compute_node rp = dict(uuid=cn.uuid, generation=42) @@ -783,8 +883,8 @@ def test_delete_inventory_inventory_in_use(self, mock_get, mock_put, 'DISK_GB': {'total': 10}, } } - mock_put.return_value.status_code = 409 - mock_put.return_value.headers = {'openstack-request-id': + mock_delete.return_value.status_code = 409 + mock_delete.return_value.headers = {'openstack-request-id': uuids.request_id} rc_str = "VCPU, MEMORY_MB" in_use_exc = exception.InventoryInUse( @@ -798,8 +898,8 @@ def test_delete_inventory_inventory_in_use(self, mock_get, mock_put, update conflict: %s """ % six.text_type(in_use_exc) - mock_put.return_value.text = fault_text - mock_put.return_value.json.return_value = { + mock_delete.return_value.text = fault_text + mock_delete.return_value.json.return_value = { 'resource_provider_generation': 44, 'inventories': { } @@ -810,13 +910,50 @@ def test_delete_inventory_inventory_in_use(self, mock_get, mock_put, self.assertEqual(uuids.request_id, mock_warn.call_args[0][1]['placement_req_id']) + @mock.patch.object(report.LOG, 'debug') + @mock.patch.object(report.LOG, 'info') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete') + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'get') + def test_delete_inventory_inventory_404(self, mock_get, mock_delete, + mock_info, mock_debug): + """Test that when we attempt to delete all the inventory for a resource + provider but another thread has already deleted that resource provider, + that we simply remove the resource provider from our local cache and + return. + """ + cn = self.compute_node + rp = dict(uuid=cn.uuid, generation=42) + # Make sure the resource provider exists for preventing to call the API + self.client._resource_providers[cn.uuid] = rp + + mock_get.return_value.json.return_value = { + 'resource_provider_generation': 1, + 'inventories': { + 'VCPU': {'total': 16}, + 'MEMORY_MB': {'total': 1024}, + 'DISK_GB': {'total': 10}, + } + } + mock_delete.return_value.status_code = 404 + mock_delete.return_value.headers = {'openstack-request-id': + uuids.request_id} + result = self.client._delete_inventory(cn.uuid) + self.assertIsNone(result) + self.assertNotIn(cn.uuid, self.client._resource_providers) + self.assertTrue(mock_debug.called) + self.assertIn('deleted by another thread', mock_debug.call_args[0][0]) + self.assertEqual(uuids.request_id, + mock_debug.call_args[0][1]['placement_req_id']) + @mock.patch.object(report.LOG, 'error') @mock.patch.object(report.LOG, 'warning') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'put') + 'delete') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'get') - def test_delete_inventory_inventory_error(self, mock_get, mock_put, + def test_delete_inventory_inventory_error(self, mock_get, mock_delete, mock_warn, mock_error): cn = self.compute_node rp = dict(uuid=cn.uuid, generation=42) @@ -831,16 +968,16 @@ def test_delete_inventory_inventory_error(self, mock_get, mock_put, 'DISK_GB': {'total': 10}, } } - mock_put.return_value.status_code = 409 - mock_put.return_value.text = ( + mock_delete.return_value.status_code = 409 + mock_delete.return_value.text = ( 'There was a failure' ) - mock_put.return_value.json.return_value = { + mock_delete.return_value.json.return_value = { 'resource_provider_generation': 44, 'inventories': { } } - mock_put.return_value.headers = {'openstack-request-id': + mock_delete.return_value.headers = {'openstack-request-id': uuids.request_id} result = self.client._delete_inventory(cn.uuid) self.assertIsNone(result)