diff --git a/doc/source/contributor/webapi-version-history.rst b/doc/source/contributor/webapi-version-history.rst index f9b783e124..15781b9e04 100644 --- a/doc/source/contributor/webapi-version-history.rst +++ b/doc/source/contributor/webapi-version-history.rst @@ -2,6 +2,14 @@ REST API Version History ======================== +1.57 (master) +------------- + +Added the following new endpoint for allocation: + +* ``PATCH /v1/allocations/`` that allows updating ``name`` + and ``extra`` fields for an existing allocation. + 1.56 (Stein, 12.1.0) -------------------- diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index d461f6d4f2..db714de381 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -196,6 +196,11 @@ class AllocationCollection(collection.Collection): return sample +class AllocationPatchType(types.JsonPatchType): + + _api_base = Allocation + + class AllocationsController(pecan.rest.RestController): """REST controller for allocations.""" @@ -377,6 +382,61 @@ class AllocationsController(pecan.rest.RestController): new_allocation.uuid) return Allocation.convert_with_links(new_allocation) + def _validate_patch(self, patch): + allowed_fields = ['name', 'extra'] + for p in patch: + path = p['path'].split('/')[1] + if path not in allowed_fields: + msg = _("Cannot update %s in an allocation. Only 'name' and " + "'extra' are allowed to be updated.") + raise exception.Invalid(msg % p['path']) + + @METRICS.timer('AllocationsController.patch') + @wsme.validate(types.uuid, [AllocationPatchType]) + @expose.expose(Allocation, types.uuid_or_name, body=[AllocationPatchType]) + def patch(self, allocation_ident, patch): + """Update an existing allocation. + + :param allocation_ident: UUID or logical name of an allocation. + :param patch: a json PATCH document to apply to this allocation. + """ + if not api_utils.allow_allocation_update(): + raise webob_exc.HTTPMethodNotAllowed(_( + "The API version does not allow updating allocations")) + context = pecan.request.context + cdict = context.to_policy_values() + policy.authorize('baremetal:allocation:update', cdict, cdict) + self._validate_patch(patch) + names = api_utils.get_patch_values(patch, '/name') + for name in names: + if len(name) and not api_utils.is_valid_logical_name(name): + msg = _("Cannot update allocation with invalid name " + "'%(name)s'") % {'name': name} + raise exception.Invalid(msg) + rpc_allocation = api_utils.get_rpc_allocation_with_suffix( + allocation_ident) + allocation_dict = rpc_allocation.as_dict() + allocation = Allocation(**api_utils.apply_jsonpatch(allocation_dict, + patch)) + # Update only the fields that have changed + for field in objects.Allocation.fields: + try: + patch_val = getattr(allocation, field) + except AttributeError: + # Ignore fields that aren't exposed in the API + continue + if patch_val == wtypes.Unset: + patch_val = None + if rpc_allocation[field] != patch_val: + rpc_allocation[field] = patch_val + + notify.emit_start_notification(context, rpc_allocation, 'update') + with notify.handle_error_notification(context, + rpc_allocation, 'update'): + rpc_allocation.save() + notify.emit_end_notification(context, rpc_allocation, 'update') + return Allocation.convert_with_links(rpc_allocation) + @METRICS.timer('AllocationsController.delete') @expose.expose(None, types.uuid_or_name, status_code=http_client.NO_CONTENT) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 34690d9678..ebe633aa4b 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1161,3 +1161,11 @@ def allow_build_configdrive(): Version 1.56 of the API added support for building configdrive. """ return pecan.request.version.minor >= versions.MINOR_56_BUILD_CONFIGDRIVE + + +def allow_allocation_update(): + """Check if updating an existing allocation is allowed or not. + + Version 1.57 of the API added support for updating an allocation. + """ + return pecan.request.version.minor >= versions.MINOR_57_ALLOCATION_UPDATE diff --git a/ironic/api/controllers/v1/versions.py b/ironic/api/controllers/v1/versions.py index 2a52437cdd..1865c90290 100644 --- a/ironic/api/controllers/v1/versions.py +++ b/ironic/api/controllers/v1/versions.py @@ -94,6 +94,7 @@ BASE_VERSION = 1 # v1.54: Add events support. # v1.55: Add deploy templates API. # v1.56: Add support for building configdrives. +# v1.57: Add support for updating an exisiting allocation. MINOR_0_JUNO = 0 MINOR_1_INITIAL_VERSION = 1 @@ -152,6 +153,7 @@ MINOR_53_PORT_SMARTNIC = 53 MINOR_54_EVENTS = 54 MINOR_55_DEPLOY_TEMPLATES = 55 MINOR_56_BUILD_CONFIGDRIVE = 56 +MINOR_57_ALLOCATION_UPDATE = 57 # When adding another version, update: # - MINOR_MAX_VERSION @@ -159,7 +161,7 @@ MINOR_56_BUILD_CONFIGDRIVE = 56 # explanation of what changed in the new version # - common/release_mappings.py, RELEASE_MAPPING['master']['api'] -MINOR_MAX_VERSION = MINOR_56_BUILD_CONFIGDRIVE +MINOR_MAX_VERSION = MINOR_57_ALLOCATION_UPDATE # String representations of the minor and maximum versions _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION) diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 9715aba0f7..d7bc4dae8e 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -423,6 +423,11 @@ allocation_policies = [ 'Delete Allocation records', [{'path': '/allocations/{allocation_id}', 'method': 'DELETE'}, {'path': '/nodes/{node_ident}/allocation', 'method': 'DELETE'}]), + policy.DocumentedRuleDefault( + 'baremetal:allocation:update', + 'rule:is_admin', + 'Change name and extra fields of an allocation', + [{'path': '/allocations/{allocation_id}', 'method': 'PATCH'}]), ] event_policies = [ diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 61636574e1..2faa620ab7 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -163,7 +163,7 @@ RELEASE_MAPPING = { } }, 'master': { - 'api': '1.56', + 'api': '1.57', 'rpc': '1.48', 'objects': { 'Allocation': ['1.0'], diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index 05d78faa2f..20aa72f981 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -386,10 +386,180 @@ class TestPatch(test_api_base.BaseApiTest): 'value': 'bar', 'op': 'add'}], expect_errors=True, - headers=self.headers) + headers={api_base.Version.string: '1.56'}) self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.METHOD_NOT_ALLOWED, response.status_int) + def test_update_not_found(self): + uuid = uuidutils.generate_uuid() + response = self.patch_json('/allocations/%s' % uuid, + [{'path': '/name', 'value': 'b', + 'op': 'replace'}], + expect_errors=True, + headers=self.headers) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + + def test_add(self): + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/extra/foo', 'value': 'bar', + 'op': 'add'}], headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_int) + + def test_add_non_existent(self): + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/foo', 'value': 'bar', + 'op': 'add'}], + expect_errors=True, + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertTrue(response.json['error_message']) + + def test_add_multi(self): + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/extra/foo1', 'value': 'bar1', + 'op': 'add'}, + {'path': '/extra/foo2', 'value': 'bar2', + 'op': 'add'}], headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % self.allocation.uuid, + headers=self.headers) + expected = {"foo1": "bar1", "foo2": "bar2"} + self.assertEqual(expected, result['extra']) + + def test_replace_invalid_name(self): + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/name', 'value': '[test]', + 'op': 'replace'}], + expect_errors=True, + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertTrue(response.json['error_message']) + + @mock.patch.object(notification_utils, '_emit_api_notification') + @mock.patch.object(timeutils, 'utcnow') + def test_replace_singular(self, mock_utcnow, mock_notify): + test_time = datetime.datetime(2000, 1, 1, 0, 0) + + mock_utcnow.return_value = test_time + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/name', + 'value': 'test', 'op': 'replace'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % self.allocation.uuid, + headers=self.headers) + self.assertEqual('test', result['name']) + return_updated_at = timeutils.parse_isotime( + result['updated_at']).replace(tzinfo=None) + self.assertEqual(test_time, return_updated_at) + mock_notify.assert_has_calls([mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.INFO, + obj_fields.NotificationStatus.START), + mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.INFO, + obj_fields.NotificationStatus.END)]) + + @mock.patch.object(notification_utils, '_emit_api_notification') + @mock.patch.object(objects.Allocation, 'save') + def test_update_error(self, mock_save, mock_notify): + mock_save.side_effect = Exception() + allocation = obj_utils.create_test_allocation(self.context) + self.patch_json('/allocations/%s' % allocation.uuid, [{'path': '/name', + 'value': 'new', 'op': 'replace'}], + expect_errors=True, headers=self.headers) + mock_notify.assert_has_calls([mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.INFO, + obj_fields.NotificationStatus.START), + mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.ERROR, + obj_fields.NotificationStatus.ERROR)]) + + def test_replace_multi(self): + extra = {"foo1": "bar1", "foo2": "bar2", "foo3": "bar3"} + allocation = obj_utils.create_test_allocation( + self.context, extra=extra, uuid=uuidutils.generate_uuid()) + new_value = 'new value' + response = self.patch_json('/allocations/%s' % allocation.uuid, + [{'path': '/extra/foo2', + 'value': new_value, 'op': 'replace'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + + extra["foo2"] = new_value + self.assertEqual(extra, result['extra']) + + def test_remove_uuid(self): + response = self.patch_json('/allocations/%s' % self.allocation.uuid, + [{'path': '/uuid', 'op': 'remove'}], + expect_errors=True, + headers=self.headers) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + + def test_remove_singular(self): + allocation = obj_utils.create_test_allocation( + self.context, extra={'a': 'b'}, uuid=uuidutils.generate_uuid()) + response = self.patch_json('/allocations/%s' % allocation.uuid, + [{'path': '/extra/a', 'op': 'remove'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + self.assertEqual(result['extra'], {}) + + # Assert nothing else was changed + self.assertEqual(allocation.uuid, result['uuid']) + + def test_remove_multi(self): + extra = {"foo1": "bar1", "foo2": "bar2", "foo3": "bar3"} + allocation = obj_utils.create_test_allocation( + self.context, extra=extra, uuid=uuidutils.generate_uuid()) + + # Removing one item from the collection + response = self.patch_json('/allocations/%s' % allocation.uuid, + [{'path': '/extra/foo2', 'op': 'remove'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + extra.pop("foo2") + self.assertEqual(extra, result['extra']) + + # Removing the collection + response = self.patch_json('/allocations/%s' % allocation.uuid, + [{'path': '/extra', 'op': 'remove'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + result = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + self.assertEqual({}, result['extra']) + + # Assert nothing else was changed + self.assertEqual(allocation.uuid, result['uuid']) + + def test_remove_non_existent_property_fail(self): + response = self.patch_json( + '/allocations/%s' % self.allocation.uuid, + [{'path': '/extra/non-existent', 'op': 'remove'}], + expect_errors=True, headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_code) + self.assertTrue(response.json['error_message']) + def _create_locally(_api, _ctx, allocation, _topic): allocation.create() diff --git a/releasenotes/notes/allow-allocation-update-94d862c3da454be2.yaml b/releasenotes/notes/allow-allocation-update-94d862c3da454be2.yaml new file mode 100644 index 0000000000..0c8d8d2bb8 --- /dev/null +++ b/releasenotes/notes/allow-allocation-update-94d862c3da454be2.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + Adds REST API endpoint for updating an existing allocation. Only ``name`` + and ``extra`` fields are allowed to be updated.