diff --git a/nova/api/openstack/compute/server_groups.py b/nova/api/openstack/compute/server_groups.py index 52e357f8ab69..025bca8ada74 100644 --- a/nova/api/openstack/compute/server_groups.py +++ b/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) diff --git a/nova/db/sqlalchemy/api_models.py b/nova/db/sqlalchemy/api_models.py index 01610e6350eb..c465e3459111 100644 --- a/nova/db/sqlalchemy/api_models.py +++ b/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] diff --git a/nova/objects/instance_group.py b/nova/objects/instance_group.py index ec30a76e3f23..1ae57068a915 100644 --- a/nova/objects/instance_group.py +++ b/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.save(context.session) + group._policies = [db_policy] + 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, diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 2119f90bf205..302ce9643660 100644 --- a/nova/objects/request_spec.py +++ b/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 diff --git a/nova/tests/functional/db/test_instance_group.py b/nova/tests/functional/db/test_instance_group.py index a2449e53ef7a..17d6f2d4834a 100644 --- a/nova/tests/functional/db/test_instance_group.py +++ b/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(self, create_group, db_group, - allow_missing=('deleted', 'deleted_at')) + ovo_fixture.compare_obj( + 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_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, - allow_missing=('deleted', 'deleted_at')) + ovo_fixture.compare_obj( + 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() diff --git a/nova/tests/unit/objects/test_instance_group.py b/nova/tests/unit/objects/test_instance_group.py index f8fa7f490119..41d0b9792a74 100644 --- a/nova/tests/unit/objects/test_instance_group.py +++ b/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'), diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 457446908bb2..2c02d94a49a6 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/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', diff --git a/nova/tests/unit/objects/test_request_spec.py b/nova/tests/unit/objects/test_request_spec.py index 4649327cbb87..9852232f55b0 100644 --- a/nova/tests/unit/objects/test_request_spec.py +++ b/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)