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

@@ -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', {

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
+}

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)}

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'
+            }
+        ]
+    ),
 ]
 
 

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)

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",