From eabfedc378764faf29399c623bc9875aaf0aea23 Mon Sep 17 00:00:00 2001 From: Anton Chuchkalov Date: Fri, 22 Apr 2016 12:18:45 +0300 Subject: [PATCH 1/3] CID-512: add informative message in case of rollback --- devops-service/workers/stack_bootstrap_worker.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/devops-service/workers/stack_bootstrap_worker.rb b/devops-service/workers/stack_bootstrap_worker.rb index 48aa105..aac12cc 100644 --- a/devops-service/workers/stack_bootstrap_worker.rb +++ b/devops-service/workers/stack_bootstrap_worker.rb @@ -45,8 +45,13 @@ class StackBootstrapWorker < Worker def bootstrap_or_rollback_if_failed(options) bootstrap_result = executor.bootstrap_just_persisted(jid) puts_and_flush Devops::Messages.t("worker.stack_bootstrap.bootstrap_result.#{bootstrap_result.reason}") - if bootstrap_result.bootstrap_error? && !options[:skip_rollback] - rollback_stack! + # move try_rollback to stack executor + if bootstrap_result.bootstrap_error? + if options[:skip_rollback] + puts_and_flush "\nSkip rollback because of skip_rollback option set to true." + else + rollback_stack! + end end bootstrap_result.code end From 62187788125aea22f4a65ba577aaa7cc3cdaa083 Mon Sep 17 00:00:00 2001 From: Anton Chuchkalov Date: Fri, 22 Apr 2016 14:25:08 +0300 Subject: [PATCH 2/3] CID-512: pass skip_rollback option to bootstrappers --- devops-service/app/api2/handlers/stack.rb | 1 + .../db/mongo/models/stack/stack_base.rb | 3 +- .../lib/executors/stack_executor.rb | 8 ++++- .../stack_executor/group_bootstrapper.rb | 8 +++-- .../prioritized_groups_bootstrapper.rb | 20 +++++++---- .../stack_executor/group_bootstrapper_spec.rb | 17 ++++++++-- .../prioritized_groups_bootstrapper_spec.rb | 33 +++++++++++++++---- .../spec/executors/stack_executor_spec.rb | 16 +++++++-- .../spec/models/stack/stack_ec2_spec.rb | 4 +-- 9 files changed, 84 insertions(+), 26 deletions(-) diff --git a/devops-service/app/api2/handlers/stack.rb b/devops-service/app/api2/handlers/stack.rb index 5e0452d..d22eae5 100644 --- a/devops-service/app/api2/handlers/stack.rb +++ b/devops-service/app/api2/handlers/stack.rb @@ -45,6 +45,7 @@ module Devops stack = self.stack(id) stack.sync_status_and_events! Devops::Db.connector.stack_update(stack) + # do not remove syncing status and events, just add stack sync worker here. stack end diff --git a/devops-service/db/mongo/models/stack/stack_base.rb b/devops-service/db/mongo/models/stack/stack_base.rb index 55e924f..c2707be 100644 --- a/devops-service/db/mongo/models/stack/stack_base.rb +++ b/devops-service/db/mongo/models/stack/stack_base.rb @@ -122,8 +122,7 @@ module Devops end def skip_rollback? - result = launch_options['skip_rollback'] - result.nil? ? true : result + launch_options['skip_rollback'] || false end class << self diff --git a/devops-service/lib/executors/stack_executor.rb b/devops-service/lib/executors/stack_executor.rb index 88bc262..6feebaa 100644 --- a/devops-service/lib/executors/stack_executor.rb +++ b/devops-service/lib/executors/stack_executor.rb @@ -53,7 +53,13 @@ module Devops @just_persisted_by_priority.each do |priority, servers| puts_and_flush "Servers with priority '#{priority}': #{servers.map(&:id).join(", ")}" end - PrioritizedGroupsBootstrapper.new(out, jid, @just_persisted_by_priority).bootstrap_servers_by_priority + groups_bootstrapper = PrioritizedGroupsBootstrapper.new( + out: out, + jid: jid, + servers_by_priority: @just_persisted_by_priority, + skip_rollback: @stack.skip_rollback? + ) + groups_bootstrapper.bootstrap_servers_by_priority end def delete_stale_servers diff --git a/devops-service/lib/executors/stack_executor/group_bootstrapper.rb b/devops-service/lib/executors/stack_executor/group_bootstrapper.rb index 7341fae..59c2251 100644 --- a/devops-service/lib/executors/stack_executor/group_bootstrapper.rb +++ b/devops-service/lib/executors/stack_executor/group_bootstrapper.rb @@ -14,8 +14,10 @@ class Devops::Executor::StackExecutor end end - def initialize(out, jid, servers) - @out, @jid, @servers = out, jid, servers + # TODO: move to keyword arguments + def initialize(options) + @out, @jid, @servers = options.fetch(:out), options.fetch(:jid), options.fetch(:servers) + @skip_rollback = options.fetch(:skip_rollback) @server_bootstrap_jobs = {} end @@ -40,7 +42,7 @@ class Devops::Executor::StackExecutor server_attrs: server.to_mongo_hash, bootstrap_template: 'omnibus', owner: server.created_by, - skip_rollback: true + skip_rollback: @skip_rollback ) @out.puts "Start bootstraping server '#{server.id}' job (job id: #{job_id})." @server_bootstrap_jobs[server.id] = job_id diff --git a/devops-service/lib/executors/stack_executor/prioritized_groups_bootstrapper.rb b/devops-service/lib/executors/stack_executor/prioritized_groups_bootstrapper.rb index d7c6e01..25daea4 100644 --- a/devops-service/lib/executors/stack_executor/prioritized_groups_bootstrapper.rb +++ b/devops-service/lib/executors/stack_executor/prioritized_groups_bootstrapper.rb @@ -11,18 +11,26 @@ class Devops::Executor::StackExecutor set_result_codes bootstrap_error: 1, deploy_error: 2 end - def initialize(out, jid, servers_with_priorities) - @out, @jid, @servers_with_priorities = out, jid, servers_with_priorities + # @param servers_by_priority [Hash] is a Hash like + # {1 => [server1, server2]} + # TODO: move to keyword arguments + def initialize(options) + @out, @jid = options.fetch(:out), options.fetch(:jid) + @servers_by_priority = options.fetch(:servers_by_priority) + @skip_rollback = options.fetch(:skip_rollback) end - # @param servers_with_priorities [Hash] is a Hash like - # {1 => [server1, server2]} # Starts bootstrapping another group only after successful bootstrapping of previous. def bootstrap_servers_by_priority # don't try to use detect here: it returns priority instead of error sorted_priorities.each do |priority| puts_and_flush "Bootstrap servers with priority '#{priority}':" - bootstrapper = GroupBootstrapper.new(@out, @jid, @servers_with_priorities[priority]) + bootstrapper = GroupBootstrapper.new( + out: @out, + jid: @jid, + servers: @servers_by_priority[priority], + skip_rollback: @skip_rollback + ) bootstrap_results = bootstrapper.bootstrap_group error = find_error(bootstrap_results) return error if error @@ -33,7 +41,7 @@ class Devops::Executor::StackExecutor private def sorted_priorities - @servers_with_priorities.keys.sort.reverse + @servers_by_priority.keys.sort.reverse end def find_error(results) diff --git a/devops-service/spec/executors/stack_executor/group_bootstrapper_spec.rb b/devops-service/spec/executors/stack_executor/group_bootstrapper_spec.rb index 12db01b..65608fa 100644 --- a/devops-service/spec/executors/stack_executor/group_bootstrapper_spec.rb +++ b/devops-service/spec/executors/stack_executor/group_bootstrapper_spec.rb @@ -4,7 +4,8 @@ class Devops::Executor::StackExecutor let(:out) { double(:out, puts: nil, flush: nil) } let(:jid) { 1000 } let(:servers) { [build(:server, id: 'a'), build(:server, id: 'b')] } - let(:bootstrapper) { described_class.new(out, jid, servers ) } + let(:bootstrapper) { described_class.new(out: out, jid: jid, servers: servers, skip_rollback: false) } + let(:bootstrapper_with_skip_rollback) { described_class.new(out: out, jid: jid, servers: servers, skip_rollback: true) } let(:bootstrap_job_ids) { %w(100 200) } describe '#bootstrap_group' do @@ -15,12 +16,22 @@ class Devops::Executor::StackExecutor allow_any_instance_of(Devops::Helpers::JobWaiter).to receive(:wait) { 0 } end - it 'start bootstrap workers and add subreports' do - expect(Worker).to receive(:start_async).with(BootstrapWorker, hash_including(:server_attrs, :bootstrap_template, :owner)) + it 'starts bootstrap workers and add subreports' do + expect(Worker).to receive(:start_async).with(BootstrapWorker, hash_including(:server_attrs, :bootstrap_template, :owner, :skip_rollback)) expect(stubbed_connector).to receive(:add_report_subreports).with(jid, bootstrap_job_ids) bootstrapper.bootstrap_group end + it 'starts workers with skip_rollback=true if was built with skip_rollback=true option' do + expect(Worker).to receive(:start_async).with(anything, hash_including(skip_rollback: true)) + bootstrapper_with_skip_rollback.bootstrap_group + end + + it 'starts workers with skip_rollback=false if was built with skip_rollback=false option' do + expect(Worker).to receive(:start_async).with(anything, hash_including(skip_rollback: false)) + bootstrapper.bootstrap_group + end + it 'returns :ok result if everything ok' do expect( bootstrapper.bootstrap_group.first.reason ).to eq :ok end diff --git a/devops-service/spec/executors/stack_executor/prioritized_groups_bootstrapper_spec.rb b/devops-service/spec/executors/stack_executor/prioritized_groups_bootstrapper_spec.rb index 5c7d09d..61ad346 100644 --- a/devops-service/spec/executors/stack_executor/prioritized_groups_bootstrapper_spec.rb +++ b/devops-service/spec/executors/stack_executor/prioritized_groups_bootstrapper_spec.rb @@ -5,7 +5,13 @@ module Devops::Executor RSpec.describe PrioritizedGroupsBootstrapper, stubbed_connector: true do let(:out) { double(:out, puts: nil, flush: nil) } let(:jid) { 1000 } - let(:groups_bootstrapper) { described_class.new(out, jid, @servers_by_priority) } + let(:groups_bootstrapper) { + described_class.new(out: out, jid: jid, servers_by_priority: @servers_by_priority, skip_rollback: false) + } + let(:groups_bootstrapper_with_skip_rollback) { + described_class.new(out: out, jid: jid, servers_by_priority: @servers_by_priority, skip_rollback: true) + } + before do @array1 = []; @array2 = []; @array3 = [] @servers_by_priority = {2 => @array2, 1 => @array1, 3 => @array3} @@ -14,12 +20,25 @@ module Devops::Executor describe '#bootstrap_servers_by_priority' do subject { groups_bootstrapper.bootstrap_servers_by_priority } - it 'bootstraps servers in order by priorities, separately' do - allow(GroupBootstrapper).to receive(:new) { instance_double(GroupBootstrapper, bootstrap_group: []) } - expect(GroupBootstrapper).to receive(:new).with(out, jid, @array3).ordered - expect(GroupBootstrapper).to receive(:new).with(out, jid, @array2).ordered - expect(GroupBootstrapper).to receive(:new).with(out, jid, @array1).ordered - expect(subject).to be_ok + context "with mocked #bootstrap_group" do + before { allow(GroupBootstrapper).to receive(:new) { instance_double(GroupBootstrapper, bootstrap_group: []) } } + + it 'bootstraps servers in order by priorities, separately' do + expect(GroupBootstrapper).to receive(:new).with(hash_including(servers: @array3)).ordered + expect(GroupBootstrapper).to receive(:new).with(hash_including(servers: @array2)).ordered + expect(GroupBootstrapper).to receive(:new).with(hash_including(servers: @array1)).ordered + expect(subject).to be_ok + end + + it 'passes skip_rollback: true if was build with skip_rollback: true' do + expect(GroupBootstrapper).to receive(:new).with(hash_including(skip_rollback: true)) + groups_bootstrapper_with_skip_rollback.bootstrap_servers_by_priority + end + + it 'passes skip_rollback: false if was build with skip_rollback: false' do + expect(GroupBootstrapper).to receive(:new).with(hash_including(skip_rollback: false)) + subject + end end it 'it returns :bootstrap_error result if error occured during bootstrap' do diff --git a/devops-service/spec/executors/stack_executor_spec.rb b/devops-service/spec/executors/stack_executor_spec.rb index 0ae24e9..db314dc 100644 --- a/devops-service/spec/executors/stack_executor_spec.rb +++ b/devops-service/spec/executors/stack_executor_spec.rb @@ -76,10 +76,22 @@ class Devops::Executor::StackExecutor end describe '#bootstrap_just_persisted' do - it 'calls PrioritizedGroupsBootstrapper#bootstrap_servers_by_priority' do - expect_any_instance_of(PrioritizedGroupsBootstrapper).to receive(:bootstrap_servers_by_priority) + subject { executor_with_stack.persist_new_servers executor_with_stack.bootstrap_just_persisted(1000) + } + + it 'passes skip_rollback option equals to @stack.skip_rollback?' do + allow(stack).to receive(:skip_rollback?) { 'test' } + expect(PrioritizedGroupsBootstrapper).to receive(:new).with( + hash_including(skip_rollback: 'test') + ).and_return(double('groups bootstrapper', bootstrap_servers_by_priority: nil)) + subject + end + + it 'calls PrioritizedGroupsBootstrapper#bootstrap_servers_by_priority' do + expect_any_instance_of(PrioritizedGroupsBootstrapper).to receive(:bootstrap_servers_by_priority) + subject end end diff --git a/devops-service/spec/models/stack/stack_ec2_spec.rb b/devops-service/spec/models/stack/stack_ec2_spec.rb index 0105c1b..ee57b44 100644 --- a/devops-service/spec/models/stack/stack_ec2_spec.rb +++ b/devops-service/spec/models/stack/stack_ec2_spec.rb @@ -79,8 +79,8 @@ RSpec.describe Devops::Model::StackEc2, type: :model do expect(stack.skip_rollback?).to be true end - it 'returns true by default' do - expect(described_class.new.skip_rollback?).to be true + it 'returns false by default' do + expect(described_class.new.skip_rollback?).to be false end end From e58e8c301831ab44cf14318a7059fae4fb3da260 Mon Sep 17 00:00:00 2001 From: Anton Chuchkalov Date: Fri, 22 Apr 2016 15:43:59 +0300 Subject: [PATCH 3/3] CID-512: add unknown error to bootstrap errors --- .../lib/executors/server_executor/server_operation_result.rb | 2 +- devops-service/workers/stack_bootstrap_worker.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/devops-service/lib/executors/server_executor/server_operation_result.rb b/devops-service/lib/executors/server_executor/server_operation_result.rb index 5c015f1..b410780 100644 --- a/devops-service/lib/executors/server_executor/server_operation_result.rb +++ b/devops-service/lib/executors/server_executor/server_operation_result.rb @@ -17,7 +17,7 @@ module Devops ) def one_of_bootstrap_errors? - [:server_bootstrap_fail, :server_not_in_chef_nodes, :server_bootstrap_unknown_error].include?(reason) + [:server_bootstrap_fail, :server_not_in_chef_nodes, :server_bootstrap_unknown_error, :unknown_error].include?(reason) end end diff --git a/devops-service/workers/stack_bootstrap_worker.rb b/devops-service/workers/stack_bootstrap_worker.rb index aac12cc..59d1f55 100644 --- a/devops-service/workers/stack_bootstrap_worker.rb +++ b/devops-service/workers/stack_bootstrap_worker.rb @@ -48,7 +48,7 @@ class StackBootstrapWorker < Worker # move try_rollback to stack executor if bootstrap_result.bootstrap_error? if options[:skip_rollback] - puts_and_flush "\nSkip rollback because of skip_rollback option set to true." + puts_and_flush "\nSkip rollback because skip_rollback option set to true." else rollback_stack! end