From d1c6dd606758493dd7dc4f1e3a7d82c98e45d1e2 Mon Sep 17 00:00:00 2001 From: Bulat Gaifullin Date: Thu, 24 Sep 2015 13:39:05 +0300 Subject: [PATCH] Plugin is able to extend existing network roles The same network role from different plugins can be merged according to policy of merge, that allows each plugin to extend the VIP for role. TestCheckVIPsNames was deleted, because this patch makes it obsolete. Change-Id: I81e773e53ab67b4e7d424805134c0d86dcc7a43a Closes-Bug: #1487011 --- .../nailgun/db/sqlalchemy/models/release.py | 2 +- nailgun/nailgun/errors/__init__.py | 4 +- nailgun/nailgun/objects/cluster.py | 17 +-- nailgun/nailgun/plugins/manager.py | 33 ++++-- nailgun/nailgun/plugins/merge_policies.py | 103 ++++++++++++++++++ nailgun/nailgun/task/manager.py | 2 +- .../nailgun/test/unit/test_network_check.py | 65 ++++++++--- nailgun/nailgun/test/unit/test_objects.py | 31 +++++- .../test/unit/test_plugins_merge_policies.py | 90 +++++++++++++++ 9 files changed, 306 insertions(+), 41 deletions(-) create mode 100644 nailgun/nailgun/plugins/merge_policies.py create mode 100644 nailgun/nailgun/test/unit/test_plugins_merge_policies.py diff --git a/nailgun/nailgun/db/sqlalchemy/models/release.py b/nailgun/nailgun/db/sqlalchemy/models/release.py index c5288b7046..6cefb973aa 100644 --- a/nailgun/nailgun/db/sqlalchemy/models/release.py +++ b/nailgun/nailgun/db/sqlalchemy/models/release.py @@ -49,7 +49,7 @@ class Release(Base): nullable=False, default=consts.RELEASE_STATES.unavailable ) - networks_metadata = Column(JSON, default=[]) + networks_metadata = Column(JSON, default={}) attributes_metadata = Column(JSON, default={}) volumes_metadata = Column(JSON, default={}) modes_metadata = Column(JSON, default={}) diff --git a/nailgun/nailgun/errors/__init__.py b/nailgun/nailgun/errors/__init__.py index 06d290d8d9..ba1b2fe3b9 100644 --- a/nailgun/nailgun/errors/__init__.py +++ b/nailgun/nailgun/errors/__init__.py @@ -109,7 +109,9 @@ default_messages = { "CannotFindExtension": "Cannot find extension", # unknown - "UnknownError": "Unknown error" + "UnknownError": "Unknown error", + + "UnresolvableConflict": "Unresolvable conflict" } diff --git a/nailgun/nailgun/objects/cluster.py b/nailgun/nailgun/objects/cluster.py index b5af21c9f3..79f99984ec 100644 --- a/nailgun/nailgun/objects/cluster.py +++ b/nailgun/nailgun/objects/cluster.py @@ -38,6 +38,7 @@ from nailgun.objects import NailgunObject from nailgun.objects import Release from nailgun.objects.serializers.cluster import ClusterSerializer from nailgun.plugins.manager import PluginManager +from nailgun.plugins.merge_policies import NetworkRoleMergePolicy from nailgun.settings import settings from nailgun.utils import AttributesGenerator from nailgun.utils import dict_merge @@ -191,9 +192,8 @@ class Cluster(NailgunObject): @classmethod def get_default_kernel_params(cls, instance): - kernel_params = instance.attributes.editable.get("kernel_params") - if kernel_params and kernel_params.get("kernel"): - return kernel_params.get("kernel").get("value") + kernel_params = instance.attributes.editable.get("kernel_params", {}) + return kernel_params.get("kernel", {}).get("value") @classmethod def create_attributes(cls, instance): @@ -787,8 +787,8 @@ class Cluster(NailgunObject): @classmethod def get_default_group(cls, instance): - return [g for g in instance.node_groups - if g.name == consts.NODE_GROUPS.default][0] + default = consts.NODE_GROUPS.default + return next(g for g in instance.node_groups if g.name == default) @classmethod def create_default_group(cls, instance): @@ -967,14 +967,15 @@ class Cluster(NailgunObject): db().flush() @classmethod - def get_network_roles(cls, instance): + def get_network_roles( + cls, instance, merge_policy=NetworkRoleMergePolicy()): """Method for receiving network roles for particular cluster :param instance: nailgun.db.sqlalchemy.models.Cluster instance + :param merge_policy: the policy to merge same roles :returns: List of network roles' descriptions """ - return (instance.release.network_roles_metadata + - PluginManager.get_network_roles(instance)) + return PluginManager.get_network_roles(instance, merge_policy) @classmethod def set_network_template(cls, instance, template): diff --git a/nailgun/nailgun/plugins/manager.py b/nailgun/nailgun/plugins/manager.py index 02de05c28b..47e8453aa9 100644 --- a/nailgun/nailgun/plugins/manager.py +++ b/nailgun/nailgun/plugins/manager.py @@ -85,19 +85,32 @@ class PluginManager(object): return cluster_plugins @classmethod - def get_network_roles(cls, cluster): - core_roles = cluster.release.network_roles_metadata - known_roles = set(role['id'] for role in core_roles) + def get_network_roles(cls, cluster, merge_policy): + """Returns the network roles from plugins. + + The roles cluster and plugins will be mixed + according to merge policy. + """ + + instance_roles = cluster.release.network_roles_metadata + all_roles = dict((role['id'], role) for role in instance_roles) + conflict_roles = dict() - plugin_roles = [] - conflict_roles = {} for plugin in cluster.plugins: for role in plugin.network_roles_metadata: role_id = role['id'] - if role_id in known_roles: - conflict_roles[role_id] = plugin.name - known_roles.add(role_id) - plugin_roles.extend(plugin.network_roles_metadata) + if role_id in all_roles: + try: + merge_policy.apply_patch( + all_roles[role_id], + role + ) + except errors.UnresolvableConflict as e: + logger.error("cannot merge plugin {0}: {1}" + .format(plugin.name, e)) + conflict_roles[role_id] = plugin.name + else: + all_roles[role_id] = role if conflict_roles: raise errors.NetworkRoleConflict( @@ -106,7 +119,7 @@ class PluginManager(object): ', '.join(conflict_roles), ', '.join(set(conflict_roles.values())))) - return plugin_roles + return list(all_roles.values()) @classmethod def get_plugins_deployment_tasks(cls, cluster): diff --git a/nailgun/nailgun/plugins/merge_policies.py b/nailgun/nailgun/plugins/merge_policies.py new file mode 100644 index 0000000000..f41240fb07 --- /dev/null +++ b/nailgun/nailgun/plugins/merge_policies.py @@ -0,0 +1,103 @@ +# Copyright 2015 Mirantis, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import abc +import six + +from nailgun.errors import errors + + +@six.add_metaclass(abc.ABCMeta) +class MergePolicy(object): + """Policy to merge attributes of plugins.""" + + @abc.abstractmethod + def apply_patch(self, target, patch): + """Applies patch to the target. + + :param target: the origin object, the target can be modified. + :param patch: the modifications for merging with original + :return: the patched object. + """ + + +class NetworkRoleMergePolicy(MergePolicy): + """Policy for merging network roles.""" + + def __init__(self): + self.rules = {'vip': NetworkRoleMergePolicy._patch_vips} + + @staticmethod + def _patch_vips(target, patch): + """Patches VIP attribute for network role. + + :param: target: the origin VIPs. + :type target: list + :param patch: the VIPs, that will be added to origin + :type patch: list + :return: the patched VIPs + """ + seen = dict((vip['name'], vip) for vip in target) + for vip in patch: + if vip['name'] in seen: + if vip != seen[vip['name']]: + raise ValueError( + "VIP '{0}' conflicts with existing one" + .format(vip['name']) + ) + else: + seen[vip['name']] = vip + target.append(vip) + + return target + + def apply_patch(self, target, patch): + """Tries to apply patch to the target. + + Conflicts will be resolved according to the + predefined rules. + + :param target: the origin network role + :type target: dict + :param patch: the modifications for merging with origin + :type patch: dict + :raises: errors.UnresolvableConflict + """ + + target_props = target['properties'] + patch_props = patch['properties'] + + conflict = set(target_props) & set(patch_props) + mergeable = set(self.rules) + + # Exclude fields that has same value + for name in (conflict - mergeable): + if target_props[name] != patch_props[name]: + raise errors.UnresolvableConflict( + "Cannot apply patch for attribute {0}: conflict" + .format(name) + ) + conflict.remove(name) + + for name in conflict: + try: + target_props[name] = self.rules[name]( + target_props[name], + patch_props[name] + ) + except Exception as e: + raise errors.UnresolvableConflict( + "Cannot apply patch for attribute {0}: {1}" + .format(name, e) + ) diff --git a/nailgun/nailgun/task/manager.py b/nailgun/nailgun/task/manager.py index 14b4ce28e4..ad0a603e99 100644 --- a/nailgun/nailgun/task/manager.py +++ b/nailgun/nailgun/task/manager.py @@ -423,7 +423,7 @@ class ApplyChangesTaskManager(TaskManager, DeploymentCheckMixin): # example, enabled plugins contain conflicting network # configuration network_info = self.serialize_network_cfg(self.cluster) - except errors.DuplicatedVIPNames as e: + except (errors.DuplicatedVIPNames, errors.NetworkRoleConflict) as e: raise errors.CheckBeforeDeploymentError(e.message) logger.info( diff --git a/nailgun/nailgun/test/unit/test_network_check.py b/nailgun/nailgun/test/unit/test_network_check.py index 445139e1f9..c3a3a64ba3 100644 --- a/nailgun/nailgun/test/unit/test_network_check.py +++ b/nailgun/nailgun/test/unit/test_network_check.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import copy from mock import MagicMock from mock import patch @@ -600,25 +601,61 @@ class TestCheckVIPsNames(BaseIntegrationTest): ) self.cluster = self.env.clusters[0] + self.plugin = self.env.create_plugin(cluster=self.cluster) self.task = Task(cluster_id=self.cluster.id) self.db.add(self.task) self.db.flush() - def test_check_vip_names(self): - # in order VIPAssigningConflict error to be raised within - # 'check_before_deployment' VIP names introduced by plugins - # for the cluster must overlap with those in network configuration - # of the cluster itself, so we make here such overlapping + def test_fail_if_vips_for_cluster_are_not_unique(self): cluster_net_roles = self.cluster.release.network_roles_metadata - - with patch( - 'nailgun.objects.cluster.PluginManager.get_network_roles', - new=MagicMock(return_value=cluster_net_roles) - ): - - with self.assertRaises(errors.CheckBeforeDeploymentError) as exc: - ApplyChangesTaskManager(self.cluster.id)\ - .check_before_deployment(self.task) + cluster_net_roles[1]["properties"]["vip"].append({ + "name": "vip1" + }) + cluster_net_roles[0]["properties"]["vip"].append({ + "name": "vip1" + }) + self.db.flush() + with self.assertRaises(errors.CheckBeforeDeploymentError) as exc: + ApplyChangesTaskManager(self.cluster.id)\ + .check_before_deployment(self.task) self.assertIn('Duplicate VIP names found in network configuration', exc.exception.message) + + def test_check_vip_names_is_merged(self): + cluster_net_roles = self.cluster.release.network_roles_metadata + cluster_net_roles[0]["properties"]["vip"].append({ + "name": "vip1", "namespace": "vip_ns1" + }) + plugin_net_roles = [copy.deepcopy(cluster_net_roles[0])] + plugin_net_roles[0]["properties"]["vip"] = [{ + "name": "vip1", "namespace": "vip_ns1" + }] + self.plugin.network_roles_metadata = plugin_net_roles + self.db.flush() + self.assertNotRaises( + errors.CheckBeforeDeploymentError, + ApplyChangesTaskManager(self.cluster.id) + .check_before_deployment, + self.task + ) + + def test_fail_if_vips_cannot_be_merged(self): + cluster_net_roles = self.cluster.release.network_roles_metadata + cluster_net_roles[0]["properties"]["vip"].append({ + "name": "vip1", "namespace": "vip_ns1" + }) + plugin_net_roles = [copy.deepcopy(cluster_net_roles[0])] + plugin_net_roles[0]["properties"]["vip"] = [{ + "name": "vip1", "namespace": "vip_ns2" + }] + self.plugin.network_roles_metadata = plugin_net_roles + self.db.flush() + with self.assertRaises(errors.CheckBeforeDeploymentError) as exc: + ApplyChangesTaskManager(self.cluster.id)\ + .check_before_deployment(self.task) + + self.assertIn( + 'Cannot override existing network roles', + exc.exception.message + ) diff --git a/nailgun/nailgun/test/unit/test_objects.py b/nailgun/nailgun/test/unit/test_objects.py index ed669ec332..bbfdfac2ca 100644 --- a/nailgun/nailgun/test/unit/test_objects.py +++ b/nailgun/nailgun/test/unit/test_objects.py @@ -53,6 +53,7 @@ from nailgun.network.neutron import NeutronManager70 from nailgun import objects from nailgun.plugins.manager import PluginManager from nailgun.test import base +from nailgun.utils import dict_merge class TestObjects(BaseIntegrationTest): @@ -955,8 +956,8 @@ class TestClusterObject(BaseTestCase): ] } } - network_role.update(kwargs) - return network_role + + return dict_merge(network_role, kwargs) def test_network_defaults(self): cluster = objects.Cluster.get_by_uid(self.env.create(api=True)['id']) @@ -1081,7 +1082,7 @@ class TestClusterObject(BaseTestCase): def test_get_network_roles(self): cluster = self.env.clusters[0] - self.assertEqual( + self.assertItemsEqual( objects.Cluster.get_network_roles(cluster), cluster.release.network_roles_metadata) @@ -1135,6 +1136,23 @@ class TestClusterObject(BaseTestCase): cluster.release.network_roles_metadata + network_roles) def test_get_plugin_network_roles_fail(self): + plugins_kw_list = [ + self.env.get_default_plugin_metadata( + name='test_plugin_{0}'.format(idx), + network_roles_metadata=[ + self._get_network_role_metadata( + properties={'gateway': bool(idx)} + ) + ] + ) for idx in six.moves.range(2) + ] + + cluster = self._create_cluster_with_plugins(plugins_kw_list) + self.assertRaises( + errors.NetworkRoleConflict, + objects.Cluster.get_network_roles, cluster) + + def test_merge_network_roles(self): network_roles = [self._get_network_role_metadata()] plugins_kw_list = [ self.env.get_default_plugin_metadata( @@ -1144,9 +1162,10 @@ class TestClusterObject(BaseTestCase): ] cluster = self._create_cluster_with_plugins(plugins_kw_list) - self.assertRaises( - errors.NetworkRoleConflict, - objects.Cluster.get_network_roles, cluster) + self.assertItemsEqual( + cluster.release.network_roles_metadata + network_roles, + objects.Cluster.get_network_roles(cluster) + ) def test_get_volumes_metadata_when_plugins_are_enabled(self): plugin_volumes_metadata = { diff --git a/nailgun/nailgun/test/unit/test_plugins_merge_policies.py b/nailgun/nailgun/test/unit/test_plugins_merge_policies.py new file mode 100644 index 0000000000..0c37dbe60d --- /dev/null +++ b/nailgun/nailgun/test/unit/test_plugins_merge_policies.py @@ -0,0 +1,90 @@ +# -*- coding: utf-8 -*- + +# Copyright 2015 Mirantis, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +from nailgun import consts +from nailgun.errors import errors +from nailgun.plugins.merge_policies import NetworkRoleMergePolicy +from nailgun.test.base import BaseTestCase + + +class TestNetworkRoleMergePolicy(BaseTestCase): + def setUp(self): + super(TestNetworkRoleMergePolicy, self).setUp() + self.policy = NetworkRoleMergePolicy() + + @staticmethod + def _make_plugin_network_role(**kwargs): + properties = { + 'subnet': True, + 'gateway': False, + 'vip': [] + } + + properties.update(kwargs) + + return { + 'id': 'test_network_role', + 'default_mapping': consts.NETWORKS.public, + 'properties': properties + } + + def test_apply_path(self): + target = self._make_plugin_network_role(vip=[{'name': 'test_vip_a'}]) + patch = self._make_plugin_network_role(vip=[{'name': 'test_vip_b'}]) + self.policy.apply_patch(target, patch) + expected = self._make_plugin_network_role( + vip=[{'name': 'test_vip_a'}, {'name': 'test_vip_b'}] + ) + + self.assertDictEqual(expected, target) + + def test_apply_patch_vips_without_duplicates(self): + target = self._make_plugin_network_role( + vip=[{'name': 'test_vip_a'}, {'name': 'test_vip_b'}] + ) + patch = self._make_plugin_network_role(vip=[{'name': 'test_vip_a'}]) + self.policy.apply_patch(target, patch) + self.assertItemsEqual( + [{'name': 'test_vip_a'}, {'name': 'test_vip_b'}], + target['properties']['vip'] + ) + + def test_apply_patch_fail_if_conflict(self): + with self.assertRaisesRegexp(errors.UnresolvableConflict, 'subnet'): + self.policy.apply_patch( + self._make_plugin_network_role(subnet=True), + self._make_plugin_network_role(subnet=False) + ) + + with self.assertRaisesRegexp(errors.UnresolvableConflict, 'prop1'): + self.policy.apply_patch( + self._make_plugin_network_role(prop1=0.1), + self._make_plugin_network_role(prop1=1) + ) + + def test_apply_path_fail_if_vip_conflict(self): + with self.assertRaisesRegexp( + errors.UnresolvableConflict, + "VIP 'test' conflicts with existing one"): + self.policy.apply_patch( + self._make_plugin_network_role( + vip=[{"name": "test", "value": 1}] + ), + self._make_plugin_network_role( + vip=[{"name": "test", "value": 2}] + ) + )