From 8fa70e5dfc73e52d52c94ec2781ff185fdcca481 Mon Sep 17 00:00:00 2001 From: Yikun Jiang Date: Fri, 29 Jun 2018 21:10:11 +0800 Subject: [PATCH] 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 | 3 +- nova/db/sqlalchemy/api_models.py | 4 - nova/objects/instance_group.py | 79 +++++++++++++++++-- nova/objects/request_spec.py | 2 +- .../functional/db/test_instance_group.py | 18 +++-- .../tests/unit/objects/test_instance_group.py | 64 ++++++++++++--- nova/tests/unit/objects/test_objects.py | 2 +- nova/tests/unit/objects/test_request_spec.py | 2 +- 8 files changed, 143 insertions(+), 31 deletions(-) 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)