From b1be601da45a72e624b5da04ece8a75af29b2c7e Mon Sep 17 00:00:00 2001 From: Bulat Gaifullin Date: Thu, 24 Dec 2015 17:23:57 +0300 Subject: [PATCH] Disabled unnecessary checks for version of tasks The versions check must be disabled for tasks that are not be added to graph. The versions check must be disabled for tasks with type "stage". Closes-Bug: #1529104 Change-Id: I128176ba8860968f84eada8b75998734ae53165c --- .../orchestrator/task_based_deployment.py | 51 ++++++----- .../test/integration/test_task_deploy.py | 10 +-- .../test/unit/test_task_based_deployment.py | 89 +++++++++++++------ 3 files changed, 98 insertions(+), 52 deletions(-) diff --git a/nailgun/nailgun/orchestrator/task_based_deployment.py b/nailgun/nailgun/orchestrator/task_based_deployment.py index f5e8f3524b..1517e32d8b 100644 --- a/nailgun/nailgun/orchestrator/task_based_deployment.py +++ b/nailgun/nailgun/orchestrator/task_based_deployment.py @@ -139,10 +139,30 @@ class TaskProcessor(object): 'required_for', 'cross-depended-by' ) + min_supported_task_version = StrictVersion(consts.TASK_CROSS_DEPENDENCY) + def __init__(self): # stores mapping between ids of generated tasks and origin task self.origin_task_ids = dict() + @classmethod + def ensure_task_based_deploy_allowed(cls, task): + """Raises error if task does not support cross-dependencies. + + :param task: the task instance + :raises: errors.TaskBaseDeploymentNotAllowed + """ + if task.get('type') == consts.ORCHESTRATOR_TASK_TYPES.stage: + return + + task_version = StrictVersion(task.get('version', '1.0.0')) + if task_version < cls.min_supported_task_version: + logger.warning( + "Task '%s' does not supported task based deploy.", + task['id'] + ) + raise errors.TaskBaseDeploymentNotAllowed + def get_origin(self, task_id): """Gets the origin ID of task. @@ -190,12 +210,20 @@ class TaskProcessor(object): task_iter = iter(serialized_tasks) frame = collections.deque(itertools.islice(task_iter, 2), maxlen=2) - if len(frame) < 2: + + # in case if there is no nodes was resolved + # the serializers return empty list of task + if len(frame) == 0: + return + + # check only if task will be add to graph + self.ensure_task_based_deploy_allowed(origin_task) + + if len(frame) == 1: # It is simple case when chain contains only 1 task # do nothing # check that that frame is not empty - if len(frame) == 1: - yield self._convert_task(frame.pop(), origin_task) + yield self._convert_task(frame.pop(), origin_task) return # it is chain of tasks, need to properly handle them @@ -345,8 +373,6 @@ class TaskProcessor(object): class TasksSerializer(object): """The deploy tasks serializer.""" - min_supported_task_version = StrictVersion(consts.TASK_CROSS_DEPENDENCY) - def __init__(self, cluster, nodes): """Initializes. @@ -388,7 +414,6 @@ class TasksSerializer(object): tasks_groups = collections.defaultdict(set) for task in tasks: - self.ensure_task_based_deploy_allowed(task) if task.get('type') == consts.ORCHESTRATOR_TASK_TYPES.group: tasks_for_role = task.get('tasks') if tasks_for_role: @@ -566,17 +591,3 @@ class TasksSerializer(object): "no candidates in nodes '%s'.", name, ", ".join(six.moves.map(str, node_ids)) ) - - @classmethod - def ensure_task_based_deploy_allowed(cls, task): - """Raises error if task is supported task based deployment. - - :param task: the task instance - """ - task_version = StrictVersion(task.get('version', '1.0.0')) - if task_version < cls.min_supported_task_version: - logger.warning( - "Task '%s' does not supported task based deploy.", - task['id'] - ) - raise errors.TaskBaseDeploymentNotAllowed diff --git a/nailgun/nailgun/test/integration/test_task_deploy.py b/nailgun/nailgun/test/integration/test_task_deploy.py index 3bd255a287..5cefa35409 100644 --- a/nailgun/nailgun/test/integration/test_task_deploy.py +++ b/nailgun/nailgun/test/integration/test_task_deploy.py @@ -20,7 +20,7 @@ import mock from nailgun import consts from nailgun.errors import errors from nailgun.objects import Task -from nailgun.orchestrator.task_based_deployment import TasksSerializer +from nailgun.orchestrator.task_based_deployment import TaskProcessor from nailgun.test.base import BaseIntegrationTest from nailgun.test.base import fake_tasks from nailgun.utils import reverse @@ -77,7 +77,7 @@ class TestTaskDeploy(BaseIntegrationTest): args, kwargs = rpc_cast.call_args return args[1][1] - @mock.patch.object(TasksSerializer, "ensure_task_based_deploy_allowed") + @mock.patch.object(TaskProcessor, "ensure_task_based_deploy_allowed") def test_task_deploy_used_if_option_enabled(self, _): self.enable_deploy_task(True) message = self.get_deploy_message() @@ -87,7 +87,7 @@ class TestTaskDeploy(BaseIntegrationTest): message["args"] ) - @mock.patch.object(TasksSerializer, "ensure_task_based_deploy_allowed") + @mock.patch.object(TaskProcessor, "ensure_task_based_deploy_allowed") def test_fallback_to_granular_deploy(self, ensure_allowed): ensure_allowed.side_effect = errors.TaskBaseDeploymentNotAllowed self.enable_deploy_task(True) @@ -110,7 +110,7 @@ class TestTaskDeploy(BaseIntegrationTest): message["args"] ) - @mock.patch.object(TasksSerializer, "ensure_task_based_deploy_allowed") + @mock.patch.object(TaskProcessor, "ensure_task_based_deploy_allowed") @mock.patch('nailgun.plugins.adapters.os.path.exists', return_value=True) @mock.patch('nailgun.plugins.adapters.PluginAdapterBase._load_tasks') def test_task_deploy_with_plugins(self, load_tasks, *_): @@ -144,7 +144,7 @@ class TestTaskDeploy(BaseIntegrationTest): ) @fake_tasks(mock_rpc=False, fake_rpc=False) - @mock.patch.object(TasksSerializer, "ensure_task_based_deploy_allowed") + @mock.patch.object(TaskProcessor, "ensure_task_based_deploy_allowed") @mock.patch('nailgun.rpc.cast') def test_task_deploy_specified_tasks(self, rpc_cast, *_): self.enable_deploy_task(True) diff --git a/nailgun/nailgun/test/unit/test_task_based_deployment.py b/nailgun/nailgun/test/unit/test_task_based_deployment.py index e1c3ac0119..872c24b452 100644 --- a/nailgun/nailgun/test/unit/test_task_based_deployment.py +++ b/nailgun/nailgun/test/unit/test_task_based_deployment.py @@ -74,9 +74,12 @@ class TestTaskSerializers(BaseTestCase): tasks = [ { "id": "test1", "role": ["controller"], - "type": "stage", "version": "2.0.0" + "type": "puppet", "version": "2.0.0", "parameters": {} + }, + { + "id": "test2", "role": ["compute"], "type": "puppet", + "parameters": {} }, - {"id": "test2", "role": ["compute"], "type": "stage"}, ] self.assertRaises( task_based_deployment.errors.TaskBaseDeploymentNotAllowed, @@ -84,8 +87,28 @@ class TestTaskSerializers(BaseTestCase): self.env.clusters[-1], self.env.nodes, tasks ) + def test_serialize_success_if_all_applicable_task_has_version_2(self): + tasks = [ + { + "id": "test1", "role": ["controller"], + "type": "puppet", "version": "2.0.0", "parameters": {} + }, + { + "id": "test2", "role": ["cinder"], "type": "puppet", + "parameters": {} + }, + ] + self.assertNotRaises( + task_based_deployment.errors.TaskBaseDeploymentNotAllowed, + self.serializer.serialize, + self.env.clusters[-1], self.env.nodes, tasks + ) + def test_process_task_de_duplication(self): - task = {"id": "test", "type": "puppet", "parameters": {}} + task = { + "id": "test", "type": "puppet", "parameters": {}, + "version": "2.0.0" + } self.serializer.process_task( task, ["1"], task_based_deployment.NullResolver ) @@ -107,7 +130,8 @@ class TestTaskSerializers(BaseTestCase): def test_process_skipped_task(self): task = { - "id": "test", "type": "puppet", "parameters": {}, 'skipped': True + "id": "test", "type": "puppet", "version": "2.0.0", + "parameters": {}, 'skipped': True, } self.serializer.process_task( task, ["1"], task_based_deployment.NullResolver @@ -148,8 +172,10 @@ class TestTaskSerializers(BaseTestCase): self.serializer.expand_task_groups( {'role': ['task1', 'task2']}, { - 'task1': {'id': 'task1', 'type': 'skipped', 'role': '*'}, - 'task2': {'id': 'task2', 'type': 'skipped', 'role': '*'} + 'task1': {'id': 'task1', 'version': '2.0.0', + 'type': 'skipped', 'role': '*'}, + 'task2': {'id': 'task2', 'version': '2.0.0', + 'type': 'skipped', 'role': '*'} } ) self.assertIn('1', self.serializer.tasks_per_node) @@ -294,23 +320,6 @@ class TestTaskSerializers(BaseTestCase): "not_exists", "1, 2, 3" ) - def test_ensure_task_based_deployment_allowed(self): - self.assertRaises( - task_based_deployment.errors.TaskBaseDeploymentNotAllowed, - self.serializer.ensure_task_based_deploy_allowed, - {'id': 'task'} - ) - self.assertRaises( - task_based_deployment.errors.TaskBaseDeploymentNotAllowed, - self.serializer.ensure_task_based_deploy_allowed, - {'id': 'task', 'version': '1.2.3'} - ) - self.assertNotRaises( - task_based_deployment.errors.TaskBaseDeploymentNotAllowed, - self.serializer.ensure_task_based_deploy_allowed, - {'id': 'task', 'version': consts.TASK_CROSS_DEPENDENCY} - ) - class TestNoopSerializer(BaseTestCase): def setUp(self): @@ -410,8 +419,9 @@ class TestDeploymentTaskSerializer(BaseUnitTest): ) -class TestTaskProcessor(BaseUnitTest): +class TestTaskProcessor(BaseTestCase): def setUp(self): + super(TestTaskProcessor, self).setUp() self.processor = task_based_deployment.TaskProcessor() def test_link_tasks_on_same_node(self): @@ -527,12 +537,14 @@ class TestTaskProcessor(BaseUnitTest): def test_process_tasks_if_not_chain(self): origin_task = { - 'id': 'task', 'requires': ['a'], 'cross-depends': [{'name': 'b'}], + 'id': 'task', 'version': '2.0.0', + 'requires': ['a'], 'cross-depends': [{'name': 'b'}], 'required_for': ['c'], 'cross-depended-by': [{'name': 'd'}] } serialized = iter([{'type': 'puppet'}]) - tasks = self.processor.process_tasks(origin_task, serialized) + tasks = list(self.processor.process_tasks(origin_task, serialized)) + del origin_task['version'] self.assertItemsEqual( [dict(origin_task, type='puppet')], tasks @@ -541,7 +553,8 @@ class TestTaskProcessor(BaseUnitTest): def test_process_if_chain(self): origin_task = { - 'id': 'task', 'requires': ['a'], 'cross-depends': [{'name': 'b'}], + 'id': 'task', 'version': '2.0.0', + 'requires': ['a'], 'cross-depends': [{'name': 'b'}], 'required_for': ['c'], 'cross-depended-by': [{'name': 'd'}] } serialized = iter([ @@ -574,3 +587,25 @@ class TestTaskProcessor(BaseUnitTest): self.assertEqual('task', self.processor.get_origin('task_start')) self.assertEqual('task', self.processor.get_origin('task#1')) self.assertEqual('task', self.processor.get_origin('task_end')) + + def test_ensure_task_based_deployment_allowed(self): + self.assertRaises( + task_based_deployment.errors.TaskBaseDeploymentNotAllowed, + self.processor.ensure_task_based_deploy_allowed, + {'id': 'task'} + ) + self.assertRaises( + task_based_deployment.errors.TaskBaseDeploymentNotAllowed, + self.processor.ensure_task_based_deploy_allowed, + {'id': 'task', 'version': '1.2.3'} + ) + self.assertNotRaises( + task_based_deployment.errors.TaskBaseDeploymentNotAllowed, + self.processor.ensure_task_based_deploy_allowed, + {'id': 'task', 'version': consts.TASK_CROSS_DEPENDENCY} + ) + self.assertNotRaises( + task_based_deployment.errors.TaskBaseDeploymentNotAllowed, + self.processor.ensure_task_based_deploy_allowed, + {'id': 'task', 'type': consts.ORCHESTRATOR_TASK_TYPES.stage} + )