From 892894bfb534277642d91ad130515ca8465de582 Mon Sep 17 00:00:00 2001 From: "Vladimir Sharshov (warpc)" Date: Thu, 2 Mar 2017 19:16:31 +0300 Subject: [PATCH] task_deploy: no implicit conversion of String into Integer Now Astute will not calculate fault tolerance groups and critical node uids twice. Change-Id: I3bf2dd0ffc0fc74fd9c670bd50b32e3285ae7e2a Closes-Bug: #1669499 --- lib/astute/task_deployment.rb | 73 +++++++++++++++++-------------- spec/unit/task_deployment_spec.rb | 30 ++++++------- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/lib/astute/task_deployment.rb b/lib/astute/task_deployment.rb index e1a3ec49..cfdd2a19 100644 --- a/lib/astute/task_deployment.rb +++ b/lib/astute/task_deployment.rb @@ -25,6 +25,7 @@ module Astute 'failed' => {'status' => 'error', 'error_type' => 'deploy'} } + attr_reader :ctx, :cluster_class, :node_class def initialize(context, cluster_class=TaskCluster, node_class=TaskNode) @ctx = context @cluster_class = cluster_class @@ -95,7 +96,7 @@ module Astute support_virtual_node(tasks_graph) unzip_graph(tasks_graph, tasks_directory) - cluster = @cluster_class.new + cluster = cluster_class.new cluster.node_concurrency.maximum = Astute.config.max_nodes_per_call cluster.stop_condition { Thread.current[:gracefully_stop] } @@ -107,21 +108,23 @@ module Astute NODE_STATUSES_TRANSITIONS ) - offline_uids = fail_offline_nodes( - tasks_graph, - cluster.node_statuses_transitions - ) - setup_fault_tolerance_behavior( tasks_metadata['fault_tolerance_groups'], cluster, tasks_graph.keys ) critical_uids = critical_node_uids(cluster.fault_tolerance_groups) + offline_uids = detect_offline_nodes(tasks_graph.keys) + + fail_offline_nodes( + :offline_uids => offline_uids, + :critical_uids => critical_uids, + :node_statuses_transitions => cluster.node_statuses_transitions + ) tasks_graph.keys.each do |node_id| - node = @node_class.new(node_id, cluster) - node.context = @ctx + node = node_class.new(node_id, cluster) + node.context = ctx node.set_critical if critical_uids.include?(node_id) node.set_as_sync_point if sync_point?(node_id) node.set_status_failed if offline_uids.include?(node_id) @@ -254,11 +257,11 @@ module Astute def report_deploy_result(result) if result[:success] && result.fetch(:failed_nodes, []).empty? - @ctx.report('status' => 'ready', 'progress' => 100) + ctx.report('status' => 'ready', 'progress' => 100) elsif result[:success] && result.fetch(:failed_nodes, []).present? - @ctx.report('status' => 'ready', 'progress' => 100) + ctx.report('status' => 'ready', 'progress' => 100) else - @ctx.report( + ctx.report( 'status' => 'error', 'progress' => 100, 'error' => result[:status] @@ -270,7 +273,7 @@ module Astute return unless Astute.config.enable_graph_file graph_file = File.join( Astute.config.graph_dot_dir, - "graph-#{@ctx.task_id}.dot" + "graph-#{ctx.task_id}.dot" ) File.open(graph_file, 'w') { |f| f.write(deployment.to_dot) } Astute.logger.info("Check graph into file #{graph_file}") @@ -308,30 +311,32 @@ module Astute critical_nodes end - def fail_offline_nodes(tasks_graph, node_statuses_transitions) - offline_uids = detect_offline_nodes(tasks_graph.keys) - if offline_uids.present? - nodes = offline_uids.map do |uid| - {'uid' => uid, - 'error_msg' => 'Node is not ready for deployment: '\ - 'mcollective has not answered' - }.merge(node_statuses_transitions.fetch('failed', {})) - end + def fail_offline_nodes(args={}) + critical_uids = args.fetch(:critical_uids, []) + offline_uids = args.fetch(:offline_uids, []) + node_statuses_transitions = args.fetch(:node_statuses_transitions, {}) - @ctx.report_and_update_status( - 'nodes' => nodes, - 'error' => 'Node is not ready for deployment' - ) + return if offline_uids.blank? - missing_required = critical_node_uids(tasks_graph) & offline_uids - if missing_required.present? - error_message = "Critical nodes are not available for deployment: " \ - "#{missing_required}" - raise Astute::DeploymentEngineError, error_message - end + nodes = offline_uids.map do |uid| + {'uid' => uid, + 'error_msg' => 'Node is not ready for deployment: '\ + 'mcollective has not answered' + }.merge(node_statuses_transitions.fetch('failed', {})) + end + + ctx.report_and_update_status( + 'nodes' => nodes, + 'error' => 'Node is not ready for deployment' + ) + + missing_required = critical_uids & offline_uids + if missing_required.present? + error_message = "Critical nodes are not available for deployment: " \ + "#{missing_required}" + raise Astute::DeploymentEngineError, error_message end - offline_uids end def detect_offline_nodes(uids) @@ -344,7 +349,7 @@ module Astute if uids.present? Astute.config.mc_retries.times.each do systemtype = MClient.new( - @ctx, + ctx, "systemtype", uids, _check_result=false, @@ -368,7 +373,7 @@ module Astute node_report = cluster.nodes.inject([]) do |node_progress, node| node_progress += [{'uid' => node[0].to_s, 'progress' => 100}] end - @ctx.report('nodes' => node_report) + ctx.report('nodes' => node_report) end end diff --git a/spec/unit/task_deployment_spec.rb b/spec/unit/task_deployment_spec.rb index 4e956dc4..4b360d91 100644 --- a/spec/unit/task_deployment_spec.rb +++ b/spec/unit/task_deployment_spec.rb @@ -143,7 +143,7 @@ describe Astute::TaskDeployment do describe '#deploy' do it 'should run deploy' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) @@ -155,7 +155,7 @@ describe Astute::TaskDeployment do end it 'should not raise error if deployment info not provided' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) @@ -218,7 +218,7 @@ describe Astute::TaskDeployment do task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) - task_deployment.expects(:fail_offline_nodes).returns([]) + task_deployment.expects(:detect_offline_nodes).returns([]) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) task_deployment.deploy( @@ -232,7 +232,7 @@ describe Astute::TaskDeployment do task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) - task_deployment.expects(:fail_offline_nodes).returns([]) + task_deployment.expects(:detect_offline_nodes).returns([]) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) task_deployment.deploy( @@ -245,7 +245,7 @@ describe Astute::TaskDeployment do Astute::TaskPreDeploymentActions.any_instance.stubs(:process) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) Astute::TaskCluster.any_instance.expects(:stop_condition) @@ -261,7 +261,7 @@ describe Astute::TaskDeployment do before(:each) do task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) Deployment::Concurrency::Counter.any_instance .stubs(:maximum=).with( @@ -334,7 +334,7 @@ describe Astute::TaskDeployment do context 'dry_run' do it 'should not run actual deployment if dry_run is set to True' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) @@ -350,7 +350,7 @@ describe Astute::TaskDeployment do context 'noop_run' do it 'should run noop deployment without error states' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) @@ -373,7 +373,7 @@ describe Astute::TaskDeployment do it 'should setup max nodes per call using config' do Astute.config.max_nodes_per_call = 33 - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) @@ -396,7 +396,7 @@ describe Astute::TaskDeployment do context 'subgraphs' do it 'should call subgraph set up if subgraphs are present' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) Astute::TaskCluster.any_instance.expects(:run).returns({:success => true}) @@ -424,7 +424,7 @@ describe Astute::TaskDeployment do tasks_directory: tasks_directory) end it 'should not call subgraph setup if subgraphs are not present' do - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.stubs(:report) Astute::TaskCluster.any_instance.expects(:run).returns({:success => true}) @@ -451,7 +451,7 @@ describe Astute::TaskDeployment do it 'succeed status and 100 progress for all nodes' do Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.expects(:report).with('nodes' => [ {'uid' => '1', 'progress' => 100}, @@ -474,7 +474,7 @@ describe Astute::TaskDeployment do :failed_nodes => [failed_node], :failed_tasks => [failed_task], :status => 'Failed because of'}) - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) task_deployment.stubs(:write_graph_to_file) ctx.expects(:report).with('nodes' => [ {'uid' => '1', 'progress' => 100}, @@ -503,7 +503,7 @@ describe Astute::TaskDeployment do it 'should write if disable' do Astute.config.enable_graph_file = false - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) ctx.stubs(:report) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true}) @@ -521,7 +521,7 @@ describe Astute::TaskDeployment do it 'should write graph if enable' do Astute.config.enable_graph_file = true - task_deployment.stubs(:fail_offline_nodes).returns([]) + task_deployment.stubs(:detect_offline_nodes).returns([]) ctx.stubs(:report) Astute::TaskCluster.any_instance.stubs(:run).returns({:success => true})