Add policy to InstanceGroup object

The change items in this patch as below:
1. Add policy and rules to InstanceGroup, deprecate
the policies field.
2. Also, make _from_db_object and _create_in_db method to
support new "policy" field.

The internal usage of InstanceGroup.policies is converted to
use the new model in the REST API and RequestSpec compat code.

Related to blueprint complex-anti-affinity-policies

Change-Id: Ib33719a4b9599d86848c618a6e142c71ece79ca5

nova/api/openstack/compute/server_groups.py

@@ -166,7 +166,8 @@ class ServerGroupController(wsgi.Controller):
         sg.user_id = context.user_id
         try:
             sg.name = vals.get('name')
-            sg.policies = vals.get('policies')
+            policies = vals.get('policies')
+            sg.policy = policies[0]
             sg.create()
         except ValueError as e:
             raise exc.HTTPBadRequest(explanation=e)

nova/db/sqlalchemy/api_models.py

@@ -455,10 +455,6 @@ class InstanceGroup(API_BASE):
                               "group_name": self.name})
         return self._policies[0] if self._policies else None
 
-    @property
-    def policies(self):
-        return [p.policy for p in self._policies]
-
     @property
     def members(self):
         return [m.instance_uuid for m in self._members]

nova/objects/instance_group.py

@@ -15,6 +15,7 @@
 import copy
 
 from oslo_db import exception as db_exc
+from oslo_serialization import jsonutils
 from oslo_utils import uuidutils
 from oslo_utils import versionutils
 from sqlalchemy.orm import contains_eager
@@ -110,7 +111,8 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
     # Version 1.8: Add count_members_by_user()
     # Version 1.9: Add get_by_instance_uuid()
     # Version 1.10: Add hosts field
-    VERSION = '1.10'
+    # Version 1.11: Add policy and deprecate policies, add _rules
+    VERSION = '1.11'
 
     fields = {
         'id': fields.IntegerField(),
@@ -124,10 +126,39 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
         'policies': fields.ListOfStringsField(nullable=True, read_only=True),
         'members': fields.ListOfStringsField(nullable=True),
         'hosts': fields.ListOfStringsField(nullable=True),
+        'policy': fields.StringField(nullable=True),
+        # NOTE(danms): Use rules not _rules for general access
+        '_rules': fields.DictOfStringsField(),
         }
 
+    def __init__(self, *args, **kwargs):
+        if 'rules' in kwargs:
+            kwargs['_rules'] = kwargs.pop('rules')
+        super(InstanceGroup, self).__init__(*args, **kwargs)
+
+    @property
+    def rules(self):
+        if '_rules' not in self:
+            return {}
+        # NOTE(danms): Coerce our rules into a typed dict for convenience
+        rules = {}
+        if 'max_server_per_host' in self._rules:
+            rules['max_server_per_host'] = \
+                    int(self._rules['max_server_per_host'])
+        return rules
+
     def obj_make_compatible(self, primitive, target_version):
         target_version = versionutils.convert_version_to_tuple(target_version)
+        if target_version < (1, 11):
+            # NOTE(yikun): Before 1.11, we had a policies property which is
+            # the list of policy name, even though it was a list, there was
+            # ever only one entry in the list.
+            policy = primitive.pop('policy', None)
+            if policy:
+                primitive['policies'] = [policy]
+            else:
+                primitive['policies'] = []
+            primitive.pop('rules', None)
         if target_version < (1, 7):
             # NOTE(danms): Before 1.7, we had an always-empty
             # metadetails property
@@ -151,8 +182,28 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
             # TODO(mriedem): Remove this when NovaPersistentObject is removed.
             ignore = {'deleted': False,
                       'deleted_at': None}
-            if field in ignore and not hasattr(db_inst, field):
+            if '_rules' == field:
+                db_policy = db_inst['policy']
+                instance_group._rules = (
+                    jsonutils.loads(db_policy['rules'])
+                    if db_policy and db_policy['rules']
+                    else {})
+            elif field in ignore and not hasattr(db_inst, field):
                 instance_group[field] = ignore[field]
+            elif 'policies' == field:
+                continue
+            # NOTE(yikun): The obj.policies is deprecated and marked as
+            # read_only in version 1.11, and there is no "policies" property
+            # in InstanceGroup model anymore, so we just skip to set
+            # "policies" and then load the "policies" when "policy" is set.
+            elif 'policy' == field:
+                db_policy = db_inst['policy']
+                if db_policy:
+                    instance_group.policy = db_policy['policy']
+                    instance_group.policies = [instance_group.policy]
+                else:
+                    instance_group.policy = None
+                    instance_group.policies = []
             else:
                 instance_group[field] = db_inst[field]
 
@@ -214,7 +265,8 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
 
     @staticmethod
     @db_api.api_context_manager.writer
-    def _create_in_db(context, values, policies=None, members=None):
+    def _create_in_db(context, values, policies=None, members=None,
+                      policy=None, rules=None):
         try:
             group = api_models.InstanceGroup()
             group.update(values)
@@ -223,13 +275,22 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
             raise exception.InstanceGroupIdExists(group_uuid=values['uuid'])
 
         if policies:
-            policy = api_models.InstanceGroupPolicy(
+            db_policy = api_models.InstanceGroupPolicy(
                 group_id=group['id'], policy=policies[0], rules=None)
-            group._policies = [policy]
+            group._policies = [db_policy]
-            group.save(context.session)
+            group.rules = None
+        elif policy:
+            db_rules = jsonutils.dumps(rules or {})
+            db_policy = api_models.InstanceGroupPolicy(
+                group_id=group['id'], policy=policy,
+                rules=db_rules)
+            group._policies = [db_policy]
         else:
             group._policies = []
 
+        if group._policies:
+            group.save(context.session)
+
         if members:
             group._members = _instance_group_members_add(context, group,
                                                          members)
@@ -358,6 +419,8 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
         payload = dict(updates)
         updates.pop('id', None)
         policies = updates.pop('policies', None)
+        policy = updates.pop('policy', None)
+        rules = updates.pop('_rules', None)
         members = updates.pop('members', None)
 
         if 'uuid' not in updates:
@@ -366,7 +429,9 @@ class InstanceGroup(base.NovaPersistentObject, base.NovaObject,
 
         db_group = self._create_in_db(self._context, updates,
                                       policies=policies,
-                                      members=members)
+                                      members=members,
+                                      policy=policy,
+                                      rules=rules)
         self._from_db_object(self._context, self, db_group)
         payload['server_group_id'] = self.uuid
         compute_utils.notify_about_server_group_update(self._context,

nova/objects/request_spec.py

@@ -210,7 +210,7 @@ class RequestSpec(base.NovaObject):
             policies = list(filter_properties.get('group_policies'))
             hosts = list(filter_properties.get('group_hosts'))
             members = list(filter_properties.get('group_members'))
-            self.instance_group = objects.InstanceGroup(policies=policies,
+            self.instance_group = objects.InstanceGroup(policy=policies[0],
                                                         hosts=hosts,
                                                         members=members)
             # hosts has to be not part of the updates for saving the object

nova/tests/functional/db/test_instance_group.py

@@ -32,7 +32,8 @@ class InstanceGroupObjectTestCase(test.TestCase):
                                       user_id=self.context.user_id,
                                       project_id=self.context.project_id,
                                       name='foogroup',
-                                      policies=['foo1'],
+                                      policy='anti-affinity',
+                                      rules={'max_server_per_host': 1},
                                       members=['memberfoo'])
         group.update(values)
         group.create()
@@ -45,9 +46,11 @@ class InstanceGroupObjectTestCase(test.TestCase):
         self.assertIsInstance(db_group.policy, api_models.InstanceGroupPolicy)
         self.assertEqual(create_group.policies[0], db_group.policy.policy)
         self.assertEqual(create_group.id, db_group.policy.group_id)
-        self.assertIsNone(db_group.policy.rules)
+        ovo_fixture.compare_obj(
-        ovo_fixture.compare_obj(self, create_group, db_group,
+            self, create_group, db_group,
-                                allow_missing=('deleted', 'deleted_at'))
+            comparators={'policy': lambda a, b: b == a.policy},
+            allow_missing=('deleted', 'deleted_at', 'policies', '_rules'))
+        self.assertEqual({'max_server_per_host': 1}, create_group.rules)
 
     def test_destroy(self):
         create_group = self._api_group()
@@ -64,8 +67,11 @@ class InstanceGroupObjectTestCase(test.TestCase):
         create_group.save()
         db_group = create_group._get_from_db_by_uuid(self.context,
                                                      create_group.uuid)
-        ovo_fixture.compare_obj(self, create_group, db_group,
+        ovo_fixture.compare_obj(
-                                allow_missing=('deleted', 'deleted_at'))
+            self, create_group, db_group,
+            comparators={'policy': lambda a, b: b == a.policy},
+            allow_missing=('deleted', 'deleted_at', 'policies', '_rules'))
+        self.assertEqual({'max_server_per_host': 1}, create_group.rules)
 
     def test_add_members(self):
         create_group = self._api_group()

nova/tests/unit/objects/test_instance_group.py

@@ -15,6 +15,7 @@
 import copy
 
 import mock
+from oslo_serialization import jsonutils
 from oslo_utils import timeutils
 from oslo_versionedobjects import exception as ovo_exc
 
@@ -30,6 +31,10 @@ _TS_NOW = timeutils.utcnow(with_timezone=True)
 # in process we lose microsecond resolution.
 _TS_NOW = _TS_NOW.replace(microsecond=0)
 _DB_UUID = uuids.fake
+_INST_GROUP_POLICY_DB = {
+    'policy': 'policy1',
+    'rules': jsonutils.dumps({'max_server_per_host': '2'}),
+}
 _INST_GROUP_DB = {
     'id': 1,
     'uuid': _DB_UUID,
@@ -37,11 +42,18 @@ _INST_GROUP_DB = {
     'project_id': 'fake_project',
     'name': 'fake_name',
     # a group can only have 1 policy associated with it
-    'policies': ['policy1'],
+    'policy': _INST_GROUP_POLICY_DB,
+    '_policies': [_INST_GROUP_POLICY_DB],
     'members': ['instance_id1', 'instance_id2'],
     'created_at': _TS_NOW,
-    'updated_at': _TS_NOW
+    'updated_at': _TS_NOW,
 }
+_INST_GROUP_OBJ_VALS = dict(
+    {k: v for k, v in
+     _INST_GROUP_DB.items()
+     if k not in ('policy', '_policies')},
+    policy=_INST_GROUP_POLICY_DB['policy'],
+    rules=jsonutils.loads(_INST_GROUP_POLICY_DB['rules']))
 
 
 class _TestInstanceGroupObject(object):
@@ -53,11 +65,21 @@ class _TestInstanceGroupObject(object):
                                                        _DB_UUID)
         mock_api_get.assert_called_once_with(self.context, _DB_UUID)
         self.assertEqual(_INST_GROUP_DB['members'], obj.members)
-        self.assertEqual(_INST_GROUP_DB['policies'], obj.policies)
+        self.assertEqual([_INST_GROUP_POLICY_DB['policy']], obj.policies)
         self.assertEqual(_DB_UUID, obj.uuid)
         self.assertEqual(_INST_GROUP_DB['project_id'], obj.project_id)
         self.assertEqual(_INST_GROUP_DB['user_id'], obj.user_id)
         self.assertEqual(_INST_GROUP_DB['name'], obj.name)
+        self.assertEqual(_INST_GROUP_POLICY_DB['policy'], obj.policy)
+        self.assertEqual({'max_server_per_host': 2}, obj.rules)
+
+    def test_rules_helper(self):
+        obj = objects.InstanceGroup()
+        self.assertEqual({}, obj.rules)
+        self.assertNotIn('_rules', obj)
+        obj._rules = {}
+        self.assertEqual({}, obj.rules)
+        self.assertIn('_rules', obj)
 
     @mock.patch('nova.objects.InstanceGroup._get_from_db_by_instance',
                 return_value=_INST_GROUP_DB)
@@ -87,9 +109,10 @@ class _TestInstanceGroupObject(object):
         changed_group['name'] = 'new_name'
         db_group = copy.deepcopy(_INST_GROUP_DB)
         mock_db_get.return_value = db_group
-        obj = objects.InstanceGroup(self.context, **_INST_GROUP_DB)
+        obj = objects.InstanceGroup(self.context, **_INST_GROUP_OBJ_VALS)
         self.assertEqual(obj.name, 'fake_name')
         obj.obj_reset_changes()
+        self.assertEqual(set([]), obj.obj_what_changed())
         obj.name = 'new_name'
         obj.members = ['instance_id1']  # Remove member 2
         obj.save()
@@ -105,7 +128,7 @@ class _TestInstanceGroupObject(object):
     @mock.patch('nova.objects.InstanceGroup._get_from_db_by_uuid')
     def test_save_without_hosts(self, mock_db_get, mock_notify):
         mock_db_get.return_value = _INST_GROUP_DB
-        obj = objects.InstanceGroup(self.context, **_INST_GROUP_DB)
+        obj = objects.InstanceGroup(self.context, **_INST_GROUP_OBJ_VALS)
         obj.obj_reset_changes()
         obj.hosts = ['fake-host1']
         self.assertRaises(exception.InstanceGroupSaveException,
@@ -139,7 +162,7 @@ class _TestInstanceGroupObject(object):
         obj.user_id = _INST_GROUP_DB['user_id']
         obj.project_id = _INST_GROUP_DB['project_id']
         obj.members = _INST_GROUP_DB['members']
-        obj.policies = _INST_GROUP_DB['policies']
+        obj.policies = [_INST_GROUP_DB['policy']['policy']]
         obj.updated_at = _TS_NOW
         obj.created_at = _TS_NOW
         obj.create()
@@ -153,7 +176,9 @@ class _TestInstanceGroupObject(object):
              'updated_at': _TS_NOW,
              },
             members=_INST_GROUP_DB['members'],
-            policies=_INST_GROUP_DB['policies'])
+            policies=[_INST_GROUP_DB['policy']['policy']],
+            policy=None,
+            rules=None)
         mock_notify.assert_called_once_with(
             self.context, "create",
             {'uuid': _DB_UUID,
@@ -163,7 +188,7 @@ class _TestInstanceGroupObject(object):
              'created_at': _TS_NOW,
              'updated_at': _TS_NOW,
              'members': _INST_GROUP_DB['members'],
-             'policies': _INST_GROUP_DB['policies'],
+             'policies': [_INST_GROUP_DB['policy']['policy']],
              'server_group_id': _DB_UUID})
 
         def _group_matcher(group):
@@ -175,7 +200,7 @@ class _TestInstanceGroupObject(object):
                 group.created_at == _TS_NOW and
                 group.updated_at == _TS_NOW and
                 group.members == _INST_GROUP_DB['members'] and
-                group.policies == _INST_GROUP_DB['policies'] and
+                group.policies == [_INST_GROUP_DB['policy']['policy']] and
                 group.id == 1)
 
         group_matcher = test_utils.CustomMockCallMatcher(_group_matcher)
@@ -271,12 +296,31 @@ class _TestInstanceGroupObject(object):
                                             filters=expected_filters)
 
     def test_obj_make_compatible(self):
-        obj = objects.InstanceGroup(self.context, **_INST_GROUP_DB)
+        obj = objects.InstanceGroup(self.context, **_INST_GROUP_OBJ_VALS)
         obj_primitive = obj.obj_to_primitive()
         self.assertNotIn('metadetails', obj_primitive)
         obj.obj_make_compatible(obj_primitive, '1.6')
         self.assertEqual({}, obj_primitive['metadetails'])
 
+    def test_obj_make_compatible_pre_1_11(self):
+        none_policy_group = copy.deepcopy(_INST_GROUP_DB)
+        none_policy_group['policy'] = None
+        dbs = [_INST_GROUP_DB, none_policy_group]
+        data = lambda x: x['nova_object.data']
+        for db in dbs:
+            ig = objects.InstanceGroup()
+            obj = ig._from_db_object(self.context, ig, db)
+            # Latest version obj has policy and policies
+            obj_primitive = obj.obj_to_primitive()
+            self.assertIn('policy', data(obj_primitive))
+            self.assertIn('policies', data(obj_primitive))
+            # Before 1.10, only has polices which is the list of policy name
+            obj_primitive = obj.obj_to_primitive('1.10')
+            self.assertNotIn('policy', data(obj_primitive))
+            self.assertIn('policies', data(obj_primitive))
+            self.assertEqual([db['policy']['policy']] if db['policy'] else [],
+                             data(obj_primitive)['policies'])
+
     @mock.patch.object(objects.InstanceList, 'get_by_filters')
     def test_load_hosts(self, mock_get_by_filt):
         mock_get_by_filt.return_value = [objects.Instance(host='host1'),

nova/tests/unit/objects/test_objects.py

@@ -1103,7 +1103,7 @@ object_data = {
     'InstanceExternalEvent': '1.2-23eb6ba79cde5cd06d3445f845ba4589',
     'InstanceFault': '1.2-7ef01f16f1084ad1304a513d6d410a38',
     'InstanceFaultList': '1.2-6bb72de2872fe49ded5eb937a93f2451',
-    'InstanceGroup': '1.10-1a0c8c7447dc7ecb9da53849430c4a5f',
+    'InstanceGroup': '1.11-852ac511d30913ee88f3c3a869a8f30a',
     'InstanceGroupList': '1.8-90f8f1a445552bb3bbc9fa1ae7da27d4',
     'InstanceInfoCache': '1.5-cd8b96fefe0fc8d4d337243ba0bf0e1e',
     'InstanceList': '2.4-d2c5723da8c1d08e07cb00160edfd292',

nova/tests/unit/objects/test_request_spec.py

@@ -232,7 +232,7 @@ class _TestRequestSpecObject(object):
         spec = objects.RequestSpec()
         spec._populate_group_info(filt_props)
         self.assertIsInstance(spec.instance_group, objects.InstanceGroup)
-        self.assertEqual(['affinity'], spec.instance_group.policies)
+        self.assertEqual('affinity', spec.instance_group.policy)
         self.assertEqual(['fake1'], spec.instance_group.hosts)
         self.assertEqual(['fake-instance1'], spec.instance_group.members)