From b9641d9ff0275938aaa85458aaacfe8afd03fca9 Mon Sep 17 00:00:00 2001 From: Reid Vandewiele Date: Tue, 5 Jan 2021 15:35:40 -0800 Subject: [PATCH 1/3] Handle exit code 11 from replica upgrade task gracefully Code 11 means "PuppetDB sync in progress but not yet complete". We don't want to abort upgrade on this code. Commit also refactors task to make it easier to unit test. --- .sync.yml | 2 + CHANGELOG.md | 2 +- metadata.json | 2 +- spec/spec_helper.rb | 4 + spec/unit/task/puppet_infra_upgrade_spec.rb | 43 +++++++ tasks/puppet_infra_upgrade.rb | 130 +++++++++++--------- 6 files changed, 123 insertions(+), 60 deletions(-) create mode 100644 spec/unit/task/puppet_infra_upgrade_spec.rb diff --git a/.sync.yml b/.sync.yml index b2375f02..e97335cf 100644 --- a/.sync.yml +++ b/.sync.yml @@ -14,6 +14,8 @@ Rakefile: changelog_since_tag: '2.1.0' extras: - 'PuppetSyntax.exclude_paths = ["plans/**/*.pp", "vendor/**/*"]' +spec/spec_helper.rb: + mock_with: ':rspec' .gitignore: paths: - '.rerun.json' diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a65d52b..c5966f3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Improvements -- Handle exit code 11 from replica upgrade (still waiting for PuppetDB sync to complete) gracefully. +- Handle exit code 11 from replica upgrade task gracefully. Code 11 means "PuppetDB sync in progress but not yet complete". ## 2.4.5 ### Summary diff --git a/metadata.json b/metadata.json index 29b82361..14aecacf 100644 --- a/metadata.json +++ b/metadata.json @@ -62,7 +62,7 @@ "version_requirement": ">= 6.0.2 < 7.0.0" } ], - "pdk-version": "1.18.0", + "pdk-version": "1.18.1", "template-url": "https://github.com/puppetlabs/pdk-templates.git#1.18.0", "template-ref": "tags/1.18.0-0-g095317c" } diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d3778cac..b367fded 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,5 +1,9 @@ # frozen_string_literal: true +RSpec.configure do |c| + c.mock_with :rspec +end + require 'puppetlabs_spec_helper/module_spec_helper' require 'rspec-puppet-facts' diff --git a/spec/unit/task/puppet_infra_upgrade_spec.rb b/spec/unit/task/puppet_infra_upgrade_spec.rb new file mode 100644 index 00000000..6a8e924f --- /dev/null +++ b/spec/unit/task/puppet_infra_upgrade_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' +require_relative '../../../tasks/puppet_infra_upgrade' + +describe PEAdm::Task::PuppetInfraUpgrade do + context 'replica' do + let(:upgrade) do + described_class.new('type' => 'replica', + 'targets' => ['replica.example.com'], + 'wait_until_connected_timeout' => 120) + end + + it 'Returns when the command exits with an expected code' do + status_dbl_0 = double('Status', :exitstatus => 0) + status_dbl_11 = double('Status', :exitstatus => 11) + allow(STDOUT).to receive(:puts) + allow(upgrade).to receive(:wait_until_connected) + + allow(Open3).to receive(:capture2e).and_return(['hello world', status_dbl_0]) + expect(upgrade.execute!).to eq(nil) + + allow(Open3).to receive(:capture2e).and_return(['hello world', status_dbl_11]) + expect(upgrade.execute!).to eq(nil) + end + + it 'Exits non-zero when the command exits with an unexpected code' do + status_dbl_1 = double('Status', :exitstatus => 1) + allow(STDOUT).to receive(:puts) + allow(upgrade).to receive(:wait_until_connected) + allow(Open3).to receive(:capture2e).and_return(['hello world', status_dbl_1]) + expect { upgrade.execute! }.to raise_error(SystemExit) do |error| + expect(error.status).to eq(1) + end + end + end + + context 'compiler' do + let(:upgrade) do + described_class.new('type' => 'compiler', + 'targets' => ['replica.example.com'], + 'wait_until_connected_timeout' => 120) + end + end +end diff --git a/tasks/puppet_infra_upgrade.rb b/tasks/puppet_infra_upgrade.rb index 0cde0884..9c5c7663 100755 --- a/tasks/puppet_infra_upgrade.rb +++ b/tasks/puppet_infra_upgrade.rb @@ -8,76 +8,90 @@ require 'timeout' require 'etc' -def main - params = JSON.parse(STDIN.read) - type = params['type'] - targets = params['targets'] - timeout = params['wait_until_connected_timeout'] - token_file = params['token_file'] || File.join(Etc.getpwuid.dir, '.puppetlabs', 'token') +class PEAdm + class Task + class PuppetInfraUpgrade + def initialize(params) + @type = params['type'] + @targets = params['targets'] + @timeout = params['wait_until_connected_timeout'] + @token_file = params['token_file'] + end - exit 0 if targets.empty? + def execute! + exit 0 if @targets.empty? + token_file = @token_file || File.join(Etc.getpwuid.dir, '.puppetlabs', 'token') - cmd = ['/opt/puppetlabs/bin/puppet-infrastructure', '--render-as', 'json', 'upgrade'] - cmd << '--token-file' << token_file unless params['token_file'].nil? - cmd << type << targets.join(',') + cmd = ['/opt/puppetlabs/bin/puppet-infrastructure', '--render-as', 'json', 'upgrade'] + cmd << '--token-file' << token_file unless @token_file.nil? + cmd << @type << @targets.join(',') - wait_until_connected(nodes: targets, token_file: token_file, timeout: timeout) + wait_until_connected(nodes: @targets, token_file: token_file, timeout: @timeout) - stdouterr, status = Open3.capture2e(*cmd) - puts stdouterr - if status.success? - exit 0 - elsif status.exitstatus == 11 # Waiting for PuppetDB sync to complete, but otherwise successful - exit 0 - else - exit status.exitstatus - end -end + stdouterr, status = Open3.capture2e(*cmd) + STDOUT.puts stdouterr -def inventory_uri - @inventory_uri ||= URI.parse('https://localhost:8143/orchestrator/v1/inventory') -end + # Exit code 11 indicates PuppetDB sync in progress, just not yet + # finished. We consider that success. + if [0, 11].include?(status.exitstatus) + return + else + exit status.exitstatus + end + end -def request_object(nodes:, token_file:) - token = File.read(token_file) - body = { - 'nodes' => nodes, - }.to_json + def inventory_uri + @inventory_uri ||= URI.parse('https://localhost:8143/orchestrator/v1/inventory') + end - request = Net::HTTP::Post.new(inventory_uri.request_uri) - request['Content-Type'] = 'application/json' - request['X-Authentication'] = token - request.body = body + def request_object(nodes:, token_file:) + token = File.read(token_file) + body = { + 'nodes' => nodes, + }.to_json - request -end + request = Net::HTTP::Post.new(inventory_uri.request_uri) + request['Content-Type'] = 'application/json' + request['X-Authentication'] = token + request.body = body -def http_object - http = Net::HTTP.new(inventory_uri.host, inventory_uri.port) - http.use_ssl = true - http.verify_mode = OpenSSL::SSL::VERIFY_NONE + request + end - http -end + def http_object + http = Net::HTTP.new(inventory_uri.host, inventory_uri.port) + http.use_ssl = true + http.verify_mode = OpenSSL::SSL::VERIFY_NONE + + http + end -def wait_until_connected(nodes:, token_file:, timeout: 120) - http = http_object - request = request_object(nodes: nodes, token_file: token_file) - inventory = {} - Timeout.timeout(timeout) do - loop do - response = http.request(request) - raise unless response.is_a? Net::HTTPSuccess - inventory = JSON.parse(response.body) - break if inventory['items'].all? { |item| item['connected'] } - sleep(1) + def wait_until_connected(nodes:, token_file:, timeout: 120) + http = http_object + request = request_object(nodes: nodes, token_file: token_file) + inventory = {} + Timeout.timeout(timeout) do + loop do + response = http.request(request) + raise unless response.is_a? Net::HTTPSuccess + inventory = JSON.parse(response.body) + break if inventory['items'].all? { |item| item['connected'] } + sleep(1) + end + end + rescue Timeout::Error + raise 'Timed out waiting for nodes to be connected to orchestrator: ' + + inventory['items'].reject { |item| item['connected'] } + .map { |item| item['name'] } + .to_s + end end end -rescue Timeout::Error - raise 'Timed out waiting for nodes to be connected to orchestrator: ' + - inventory['items'].reject { |item| item['connected'] } - .map { |item| item['name'] } - .to_s end -main +# Run the task if we got piped input. In order to enable unit testing, do not +# run the task if input is a tty. +unless STDIN.tty? + upgrade = PEAdm::Task::PuppetInfraUpgrade.new(JSON.parse(STDIN.read)) + upgrade.execute! +end From ec2e9d2a94d661d896be12c896146bf06926b749 Mon Sep 17 00:00:00 2001 From: Reid Vandewiele Date: Tue, 12 Jan 2021 10:49:18 -0800 Subject: [PATCH 2/3] Catch orchestrator errors like "permission denied" Print better data when something goes wrong. Example: when the token used to connect to the orchestrator is not permitted to perform the requested operation. --- tasks/puppet_infra_upgrade.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tasks/puppet_infra_upgrade.rb b/tasks/puppet_infra_upgrade.rb index 9c5c7663..8115db51 100755 --- a/tasks/puppet_infra_upgrade.rb +++ b/tasks/puppet_infra_upgrade.rb @@ -73,7 +73,9 @@ def wait_until_connected(nodes:, token_file:, timeout: 120) Timeout.timeout(timeout) do loop do response = http.request(request) - raise unless response.is_a? Net::HTTPSuccess + unless response.is_a? Net::HTTPSuccess + raise "Unexpected result from orchestrator: #{response.class}\n#{response}" + end inventory = JSON.parse(response.body) break if inventory['items'].all? { |item| item['connected'] } sleep(1) From d41dfe0d65ade0b222dd2512bab108dfb7fc76ed Mon Sep 17 00:00:00 2001 From: Reid Vandewiele Date: Tue, 12 Jan 2021 14:28:44 -0800 Subject: [PATCH 3/3] Make unit testing mode controlled by env var This feels more user-defined and unlikely to be affected by future Bolt product changes, compared to relying on whether or not a tty is present. --- spec/spec_helper_local.rb | 4 ++++ tasks/puppet_infra_upgrade.rb | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/spec/spec_helper_local.rb b/spec/spec_helper_local.rb index 3622c6b4..5f9bc2da 100644 --- a/spec/spec_helper_local.rb +++ b/spec/spec_helper_local.rb @@ -2,6 +2,10 @@ require 'puppet' +# This environment variable can be read by Ruby Bolt tasks to prevent unwanted +# auto-execution, enabling easy unit testing. +ENV["RSPEC_UNIT_TEST_MODE"] ||= "TRUE" + if Gem::Version.new(Puppet.version) >= Gem::Version.new('6.0.0') begin require 'bolt_spec/plans' diff --git a/tasks/puppet_infra_upgrade.rb b/tasks/puppet_infra_upgrade.rb index 8115db51..3da754d8 100755 --- a/tasks/puppet_infra_upgrade.rb +++ b/tasks/puppet_infra_upgrade.rb @@ -91,9 +91,10 @@ def wait_until_connected(nodes:, token_file:, timeout: 120) end end -# Run the task if we got piped input. In order to enable unit testing, do not -# run the task if input is a tty. -unless STDIN.tty? +# Run the task unless an environment flag has been set, signaling not to. The +# environment flag is used to disable auto-execution and enable Ruby unit +# testing of this task. +unless ENV['RSPEC_UNIT_TEST_MODE'] upgrade = PEAdm::Task::PuppetInfraUpgrade.new(JSON.parse(STDIN.read)) upgrade.execute! end