From 7220be3968ee1dd257c9add88228cc5bb9857795 Mon Sep 17 00:00:00 2001 From: Johannes Kulik Date: Thu, 17 Jun 2021 09:29:14 +0200 Subject: [PATCH] api: Add an update method for server-groups This new API endpoint allows to add and/or remove members from a server-group. We found this to be necessary, because instances might get created without a server-group, but later need an HA-partner and re-installing would mean downtime or too much effort. The endpoint checks the validity of the policy is still given after all changes are done and strives for idempotency in that it allows removal/addition of already removed/added instance uuids to accommodate for requests built in parallel. TODO: * api-request docs Change-Id: I30d5d1dc3a41553b4336aad3877018989159495c --- nova/api/openstack/compute/routes.py | 1 + .../compute/schemas/server_groups.py | 15 ++ nova/api/openstack/compute/server_groups.py | 105 +++++++++ nova/policies/server_groups.py | 11 + .../openstack/compute/test_server_groups.py | 211 +++++++++++++++++- nova/tests/unit/test_policy.py | 1 + 6 files changed, 340 insertions(+), 4 deletions(-) diff --git a/nova/api/openstack/compute/routes.py b/nova/api/openstack/compute/routes.py index c26f9a8ff1a..17edb91233b 100644 --- a/nova/api/openstack/compute/routes.py +++ b/nova/api/openstack/compute/routes.py @@ -705,6 +705,7 @@ def _create_controller(main_controller, controller_list, }), ('/os-server-groups/{id}', { 'GET': [server_groups_controller, 'show'], + 'PUT': [server_groups_controller, 'update'], 'DELETE': [server_groups_controller, 'delete'] }), ('/os-services', { diff --git a/nova/api/openstack/compute/schemas/server_groups.py b/nova/api/openstack/compute/schemas/server_groups.py index d8401e15834..6ea4214e0ed 100644 --- a/nova/api/openstack/compute/schemas/server_groups.py +++ b/nova/api/openstack/compute/schemas/server_groups.py @@ -83,3 +83,18 @@ # For backward compatible changes 'additionalProperties': True } + +update = { + 'type': 'object', + 'properties': { + 'add_members': { + 'type': 'array', + 'items': parameter_types.server_id, + }, + 'remove_members': { + 'type': 'array', + 'items': parameter_types.server_id, + } + }, + 'additionalProperties': False +} diff --git a/nova/api/openstack/compute/server_groups.py b/nova/api/openstack/compute/server_groups.py index caa629f9573..c2093b0e101 100644 --- a/nova/api/openstack/compute/server_groups.py +++ b/nova/api/openstack/compute/server_groups.py @@ -243,3 +243,108 @@ def create(self, req, body): raise exc.HTTPForbidden(explanation=msg) return {'server_group': self._format_server_group(context, sg, req)} + + @wsgi.Controller.api_version("2.64") + @validation.schema(schema.update) + @wsgi.expected_errors((400, 404)) + def update(self, req, id, body): + """Update a server-group's members + + Striving for idempotency, we accept already removed or already + contained members. + + We always remove first and then check if we can add the requested + members. That way, removing an instance for a host and adding another + one works in one request. + + We do all requested changes or no change. + """ + context = _authorize_context(req, 'update') + try: + sg = objects.InstanceGroup.get_by_uuid(context, id) + except nova.exception.InstanceGroupNotFound as e: + raise webob.exc.HTTPNotFound(explanation=e.format_message()) + + members_to_remove = set(body.get('remove_members', [])) + members_to_add = set(body.get('add_members', [])) + LOG.info('Called update for server-group %s with add_members: %s and ' + 'remove_members %s', + id, ', '.join(members_to_add), ', '.join(members_to_remove)) + + overlap = members_to_remove & members_to_add + if overlap: + msg = ('Parameters "add_members" and "remove_members" are ' + 'overlapping in {}'.format(', '.join(overlap))) + raise exc.HTTPBadRequest(explanation=msg) + + if not members_to_remove and not members_to_add: + LOG.info("No update requested.") + formatted_sg = self._format_server_group(context, sg, req) + return {'server_group': formatted_sg} + + # don't do work if it's not necessary. we might be able to get a fast + # way out if this request is already fulfilled + members_to_remove = members_to_remove & set(sg.members) + members_to_add = members_to_add - set(sg.members) + + if not members_to_remove and not members_to_add: + LOG.info("State already satisfied.") + formatted_sg = self._format_server_group(context, sg, req) + return {'server_group': formatted_sg} + + # retrieve all the instances to add, failing if one doesn't exist, + # because we need to check the hosts against the policy and adding + # non-existent instances doesn't make sense + found_instances_hosts = _get_not_deleted(context, members_to_add) + missing_uuids = members_to_add - set(found_instances_hosts) + if missing_uuids: + msg = ("One or more members in add_members cannot be found: {}" + .format(', '.join(missing_uuids))) + raise exc.HTTPBadRequest(explanation=msg) + + # check if the policy is still valid with these changes + if sg.policy in ('affinity', 'anti-affinity'): + current_members_hosts = _get_not_deleted(context, sg.members) + current_hosts = set(h for u, h in current_members_hosts.items() + if u not in members_to_remove) + if sg.policy == 'affinity': + outliers = [u for u, h in found_instances_hosts.items() + if h and h not in current_hosts] + elif sg.policy == 'anti-affinity': + outliers = [u for u, h in found_instances_hosts.items() + if h and h in current_hosts] + else: + outliers = None + LOG.warning('server-group update check not implemented for ' + 'policy %s', sg.policy) + if outliers: + LOG.info('Update of server-group %s with policy %s aborted: ' + 'policy violation by %s', + id, sg.policy, ', '.join(outliers)) + msg = ("Adding instance(s) {} would violate policy '{}'." + .format(', '.join(outliers), sg.policy)) + raise exc.HTTPBadRequest(explanation=msg) + + # update the server group and save it + if members_to_remove: + objects.InstanceGroup.remove_members(context, sg.id, + members_to_remove, sg.uuid) + if members_to_add: + try: + objects.InstanceGroup.add_members(context, id, members_to_add) + except Exception: + LOG.exception('Failed to add members.') + if members_to_remove: + LOG.info('Trying to add removed members again after ' + 'error.') + objects.InstanceGroup.add_members(context, id, + members_to_remove) + raise + + LOG.info("Changed server-group %s in DB.", id) + + # refresh InstanceGroup object, because we changed it directly in the + # DB. + sg.refresh() + + return {'server_group': self._format_server_group(context, sg, req)} diff --git a/nova/policies/server_groups.py b/nova/policies/server_groups.py index ea6ef2c13cf..dd77e14d94c 100644 --- a/nova/policies/server_groups.py +++ b/nova/policies/server_groups.py @@ -73,6 +73,17 @@ } ] ), + policy.DocumentedRuleDefault( + POLICY_ROOT % 'update', + BASE_POLICY_RULE, + "Update members of a server group", + [ + { + 'path': '/os-server-groups/{server_group_id}', + 'method': 'PUT' + } + ] + ), ] diff --git a/nova/tests/unit/api/openstack/compute/test_server_groups.py b/nova/tests/unit/api/openstack/compute/test_server_groups.py index 0a05875e3aa..e3fa9a46ae5 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_groups.py +++ b/nova/tests/unit/api/openstack/compute/test_server_groups.py @@ -185,12 +185,12 @@ def test_create_server_group_rbac_admin_only(self): "Policy doesn't allow %s to be performed." % rule_name, exc.format_message()) - def _create_instance(self, ctx, cell): + def _create_instance(self, ctx, cell, host='host1'): with context.target_cell(ctx, cell) as cctx: instance = objects.Instance(context=cctx, image_ref=uuidsentinel.fake_image_ref, node='node1', reservation_id='a', - host='host1', project_id='fake', + host=host, project_id='fake', vm_state='fake', system_metadata={'key': 'value'}) instance.create() @@ -202,10 +202,10 @@ def _create_instance(self, ctx, cell): im.create() return instance - def _create_instance_group(self, context, members): + def _create_instance_group(self, context, members, policy=None): ig = objects.InstanceGroup(context=context, name='fake_name', user_id='fake_user', project_id='fake', - members=members) + members=members, policy=policy) ig.create() return ig.uuid @@ -848,3 +848,206 @@ def test_additional_params(self): sgroup = server_group_template(unknown='unknown') self.assertRaises(self.validation_error, self.controller.create, req, body={'server_group': sgroup}) + + def test_update_server_group_not_found(self): + """We raise a 404 if the server group does not exist.""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + self.assertRaises(webob.exc.HTTPNotFound, + self.controller.update, req, uuidsentinel.group1, body={}) + + def test_update_server_group_empty(self): + """We do not fail if the user doesn't request any changes""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + res_dict = self.controller.update(req, ig_uuid, body={}) + result_members = res_dict['server_group']['members'] + self.assertEqual(3, len(result_members)) + for member in members: + self.assertIn(member, result_members) + + def test_update_server_group_add_remove_overlap(self): + """We do not accept changes, if there's a server to be both added and + removed, because the result would depend on implementation details if + we remove first or add first. + """ + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + body = { + 'add_members': [uuidsentinel.uuid1, uuidsentinel.uuid2], + 'remove_members': [uuidsentinel.uuid2, uuidsentinel.uuid3], + } + result = self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.update, req, ig_uuid, body=body) + self.assertIn('Parameters "add_members" and "remove_members" are ' + 'overlapping in {}'.format(uuidsentinel.uuid2), + six.text_type(result)) + + def test_update_server_group_remove_nonexisting(self): + """Don't fail if the user tries to remove a server not being member of + the server group. + """ + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + body = { + 'remove_members': [uuidsentinel.uuid4], + } + res_dict = self.controller.update(req, ig_uuid, body=body) + result_members = res_dict['server_group']['members'] + self.assertEqual(3, len(result_members)) + for member in members: + self.assertIn(member, result_members) + + def test_update_server_group_add_already_added(self): + """Don't fail if the user adds a server that's already a member of the + server group. + """ + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + body = { + 'add_members': [members[0]], + } + res_dict = self.controller.update(req, ig_uuid, body=body) + result_members = res_dict['server_group']['members'] + self.assertEqual(3, len(result_members)) + for member in members: + self.assertIn(member, result_members) + + def test_update_server_group_add_against_policy_affinity(self): + """Fail if adding the server would break the policy.""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + + cell1 = self.cells[uuidsentinel.cell1] + instances = [self._create_instance(ctx, cell1, host='host1')] + + ig_uuid = self._create_instance_group(ctx, [i.uuid for i in instances], + policy='affinity') + + cell1 = self.cells[uuidsentinel.cell1] + new_instance = self._create_instance(ctx, cell1, host='host2') + + body = { + 'add_members': [new_instance.uuid], + } + result = self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.update, req, ig_uuid, body=body) + self.assertIn("Adding instance(s) {} would violate policy 'affinity'." + .format(new_instance.uuid), + six.text_type(result)) + + def test_update_server_group_add_against_policy_anti_affinity(self): + """Fail if adding the server would break the policy.""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + + cell1 = self.cells[uuidsentinel.cell1] + instances = [self._create_instance(ctx, cell1, host='host1')] + + ig_uuid = self._create_instance_group(ctx, [i.uuid for i in instances], + policy='anti-affinity') + + cell1 = self.cells[uuidsentinel.cell1] + new_instance = self._create_instance(ctx, cell1, host='host1') + + body = { + 'add_members': [new_instance.uuid], + } + result = self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.update, req, ig_uuid, body=body) + self.assertIn("Adding instance(s) {} would violate policy " + "'anti-affinity'.".format(new_instance.uuid), + six.text_type(result)) + + def test_update_server_group_add_with_remove_fixes_policy(self): + """Don't fail if adding a server would break the policy, but the remove + in the same request fixes that. + """ + """Fail if adding the server would break the policy.""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + + cell1 = self.cells[uuidsentinel.cell1] + instances = [self._create_instance(ctx, cell1, host='host1'), + self._create_instance(ctx, cell1, host='host2')] + + ig_uuid = self._create_instance_group(ctx, [i.uuid for i in instances], + policy='anti-affinity') + + cell1 = self.cells[uuidsentinel.cell1] + new_instance = self._create_instance(ctx, cell1, host='host2') + + body = { + 'add_members': [new_instance.uuid], + 'remove_members': [instances[1].uuid], + } + res_dict = self.controller.update(req, ig_uuid, body=body) + result_members = res_dict['server_group']['members'] + self.assertEqual(2, len(result_members)) + for member in [instances[0].uuid, new_instance.uuid]: + self.assertIn(member, result_members) + + def test_update_server_group_add_nonexisting_instance(self): + """Fail if the instances the user tries to add does not exist.""" + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + body = { + 'add_members': [uuidsentinel.uuid1], + } + result = self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.update, req, ig_uuid, body=body) + self.assertIn('One or more members in add_members cannot be found: ' + '{}'.format(uuidsentinel.uuid1), + six.text_type(result)) + + def test_update_server_group_add_instance_multiple_cells(self): + """Don't fail if the instance the user tries to add is in another cell. + """ + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + (ig_uuid, instances, members) = self._create_groups_and_instances(ctx) + cell1 = self.cells[uuidsentinel.cell1] + cell2 = self.cells[uuidsentinel.cell2] + new_instances = [self._create_instance(ctx, cell1), + self._create_instance(ctx, cell2)] + body = { + 'add_members': [i.uuid for i in new_instances], + } + res_dict = self.controller.update(req, ig_uuid, body=body) + result_members = res_dict['server_group']['members'] + self.assertEqual(5, len(result_members)) + for member in members: + self.assertIn(member, result_members) + for instance in new_instances: + self.assertIn(instance.uuid, result_members) + + def test_update_server_group_add_against_soft_policy(self): + """Don't fail if the policy would fail, but it's a soft-* policy - they + are best-effort by design. + """ + req = fakes.HTTPRequest.blank('', version=self.wsgi_api_version) + ctx = context.RequestContext('fake_user', 'fake') + + cell1 = self.cells[uuidsentinel.cell1] + instances = [self._create_instance(ctx, cell1, host='host1')] + + ig_uuid = self._create_instance_group(ctx, [i.uuid for i in instances], + policy='soft-anti-affinity') + + cell1 = self.cells[uuidsentinel.cell1] + new_instance = self._create_instance(ctx, cell1, host='host1') + + body = { + 'add_members': [new_instance.uuid], + } + res_dict = self.controller.update(req, ig_uuid, body=body) + result_members = res_dict['server_group']['members'] + self.assertEqual(2, len(result_members)) + for member in [instances[0].uuid, new_instance.uuid]: + self.assertIn(member, result_members) diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index a5afc02bbae..998bbfc39c5 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -429,6 +429,7 @@ def setUp(self): "os_compute_api:os-server-groups:show", "os_compute_api:os-server-groups:create", "os_compute_api:os-server-groups:delete", +"os_compute_api:os-server-groups:update", "os_compute_api:os-shelve:shelve", "os_compute_api:os-shelve:unshelve", "os_compute_api:os-volumes",