From adf2560e935ea957960495b86756ca253cc5faee Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 4 May 2016 11:44:04 -0700 Subject: [PATCH 1/2] (maint) Update pcp-specifications links Update links to point to version 1.0. The pcp-specifications repo was reorganized to separate different versions of the specification. --- README.md | 10 +++++----- acceptance/lib/pxp-agent/test_helper.rb | 6 +++--- modules/README.md | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index b0e25dcb..45f9749d 100644 --- a/README.md +++ b/README.md @@ -361,13 +361,13 @@ Tickets: File bug tickets at https://tickets.puppet.com/browse/PCP and add the [nssm]: https://nssm.cc [modules_docs]: https://github.com/puppetlabs/pxp-agent/blob/master/modules/README.md [pcp-broker]: https://github.com/puppetlabs/pcp-broker -[pcp_specs_root]: https://github.com/puppetlabs/pcp-specifications/blob/master/pcp/README.md +[pcp_specs_root]: https://github.com/puppetlabs/pcp-specifications/blob/master/pcp/versions/1.0/README.md [pxp-module-puppet_docs]: https://github.com/puppetlabs/pxp-agent/blob/master/lib/tests/resources/modules/reverse_valid [pxp-module-puppet_script]: https://github.com/puppetlabs/pxp-agent/blob/master/modules/pxp-module-puppet.md -[pxp_specs_actions]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/actions.md -[pxp_specs_request_response]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/request_response.md -[pxp_specs_root]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/README.md -[pxp_specs_transaction_status]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/transaction_status.md +[pxp_specs_actions]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/actions.md +[pxp_specs_request_response]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/request_response.md +[pxp_specs_root]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/README.md +[pxp_specs_transaction_status]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/transaction_status.md [websocketpp]: https://github.com/zaphoyd/websocketpp [MinGW-w64]: http://mingw-w64.sourceforge.net/ [Chocolatey]: https://chocolatey.org diff --git a/acceptance/lib/pxp-agent/test_helper.rb b/acceptance/lib/pxp-agent/test_helper.rb index e32eabae..dbd0f68f 100644 --- a/acceptance/lib/pxp-agent/test_helper.rb +++ b/acceptance/lib/pxp-agent/test_helper.rb @@ -128,7 +128,7 @@ def assert_eventmachine_thread_running end # Query pcp-broker's inventory of associated clients -# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pcp/inventory.md +# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pcp/versions/1.0/inventory.md # @param broker hostname or beaker host object of the pcp-broker host # @param query pattern of client identities that you want to check e.g. pcp://*/agent for all agents, or pcp://*/* for everything # @return Ruby array of strings that are the client identities that match the query pattern @@ -233,7 +233,7 @@ def is_not_associated?(broker, identity, retries = 60) end # Make an rpc_blocking_request to pxp-agent -# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/request_response.md +# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/request_response.md # @param broker hostname or beaker host object of the machine running PCP broker # @param targets array of PCP identities to send request to # e.g. ["pcp://client01.example.com/agent","pcp://client02.example.com/agent"] @@ -250,7 +250,7 @@ def rpc_blocking_request(broker, targets, end # Make an rpc_non_blocking_request to pxp-agent -# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/request_response.md +# Reference: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/request_response.md # @param broker hostname or beaker host object of the machine running PCP broker # @param targets array of PCP identities to send request to # e.g. ["pcp://client01.example.com/agent","pcp://client02.example.com/agent"] diff --git a/modules/README.md b/modules/README.md index fa077d99..1fec57a5 100644 --- a/modules/README.md +++ b/modules/README.md @@ -192,4 +192,4 @@ the [PXP Transaction Response][transaction_status]. The only reserved code is `5`; such code should be used in case the `output_files` entry was included, but the module failed to write the action's results on file. -[transaction_status]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/transaction_status.md +[transaction_status]: https://github.com/puppetlabs/pcp-specifications/blob/master/pxp/versions/1.0/transaction_status.md From 0a4f4f4af62d50156abbbd856a4f0ddbdb8fa11b Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 4 May 2016 11:56:18 -0700 Subject: [PATCH 2/2] (PCP-306) Use last_run_report mtime to check for updates Previously last_run_report would be assumed to use a newer time stamp if the triggered Puppet run successfully updated the report. NTP updates during the Puppet run could potentially violate that assumption. Switch to checking mtime on the report, and assuming if mtime has changed that a new report was written. --- modules/pxp-module-puppet | 33 +++++++++------- modules/spec/unit/modules/puppet_spec.rb | 50 +++++++++++++++--------- 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/modules/pxp-module-puppet b/modules/pxp-module-puppet index 1d595d50..30e64716 100755 --- a/modules/pxp-module-puppet +++ b/modules/pxp-module-puppet @@ -83,16 +83,15 @@ def make_error_result(exitcode, error_type, error_message) return result end -def get_result_from_report(exitcode, config, start_time) - last_run_report = check_config_print("lastrunreport",config) - if last_run_report.empty? +def get_result_from_report(last_run_report, exitcode, config, start_time) + if !File.exist?(last_run_report) return make_error_result(exitcode, Errors::NoLastRunReport, - "could not find the location of the last run report") + "#{last_run_report} doesn't exist") end - if !File.exist?(last_run_report) + if start_time && File.mtime(last_run_report) == start_time return make_error_result(exitcode, Errors::NoLastRunReport, - "#{last_run_report} doesn't exist") + "#{last_run_report} was not written") end last_run_report_yaml = {} @@ -111,13 +110,11 @@ def get_result_from_report(exitcode, config, start_time) "Puppet agent exited with a non 0 exitcode") end - if start_time.to_i < last_run_report_yaml.time.to_i - run_result["kind"] = last_run_report_yaml.kind - run_result["time"] = last_run_report_yaml.time - run_result["transaction_uuid"] = last_run_report_yaml.transaction_uuid - run_result["environment"] = last_run_report_yaml.environment - run_result["status"] = last_run_report_yaml.status - end + run_result["kind"] = last_run_report_yaml.kind + run_result["time"] = last_run_report_yaml.time + run_result["transaction_uuid"] = last_run_report_yaml.transaction_uuid + run_result["environment"] = last_run_report_yaml.environment + run_result["status"] = last_run_report_yaml.status return run_result end @@ -126,7 +123,13 @@ def start_run(config, action_input) exitcode = DEFAULT_EXITCODE cmd = make_command_array(config, action_input) env = make_environment_hash(action_input) - start_time = Time.now + + last_run_report = check_config_print("lastrunreport", config) + if last_run_report.empty? + return make_error_result(exitcode, Errors::NoLastRunReport, + "could not find the location of the last run report") + end + start_time = File.mtime(last_run_report) if File.exist?(last_run_report) run_result = Puppet::Util::Execution.execute(cmd, {:failonfail => false, :custom_environment => env}) @@ -147,7 +150,7 @@ def start_run(config, action_input) "Puppet agent is already performing a run") end - return get_result_from_report(exitcode, config, start_time) + return get_result_from_report(last_run_report, exitcode, config, start_time) end def metadata() diff --git a/modules/spec/unit/modules/puppet_spec.rb b/modules/spec/unit/modules/puppet_spec.rb index c7d16022..fb34888d 100644 --- a/modules/spec/unit/modules/puppet_spec.rb +++ b/modules/spec/unit/modules/puppet_spec.rb @@ -102,33 +102,37 @@ end describe "get_result_from_report" do + let(:last_run_report_path) { + "/opt/puppetlabs/puppet/cache/state/fake_last_run_report.yaml" + } + it "doesn't process the last_run_report if the file doens't exist" do allow(File).to receive(:exist?).and_return(false) - allow_any_instance_of(Object).to receive(:check_config_print).and_return("/opt/puppetlabs/puppet/cache/state/last_run_report.yaml") - expect(get_result_from_report(0, default_configuration, Time.now)).to be == + expect(get_result_from_report(last_run_report_path, 0, default_configuration, Time.now)).to be == {"kind" => "unknown", "time" => "unknown", "transaction_uuid" => "unknown", "environment" => "unknown", "status" => "unknown", "error_type" => "no_last_run_report", - "error" => "/opt/puppetlabs/puppet/cache/state/last_run_report.yaml doesn't exist", + "error" => "#{last_run_report_path} doesn't exist", "exitcode" => 0, "version" => 1} end it "doesn't process the last_run_report if the file cant be loaded" do + start_time = Time.now + allow(File).to receive(:mtime).and_return(start_time+1) allow(File).to receive(:exist?).and_return(true) - allow_any_instance_of(Object).to receive(:check_config_print).and_return("/opt/puppetlabs/puppet/cache/state/last_run_report.yaml") allow(YAML).to receive(:load_file).and_raise("error") - expect(get_result_from_report(0, default_configuration, Time.now)).to be == + expect(get_result_from_report(last_run_report_path, 0, default_configuration, start_time)).to be == {"kind" => "unknown", "time" => "unknown", "transaction_uuid" => "unknown", "environment" => "unknown", "status" => "unknown", "error_type" => "invalid_last_run_report", - "error" => "/opt/puppetlabs/puppet/cache/state/last_run_report.yaml could not be loaded: error", + "error" => "#{last_run_report_path} could not be loaded: error", "exitcode" => 0, "version" => 1} end @@ -144,18 +148,18 @@ allow(last_run_report).to receive(:environment).and_return("production") allow(last_run_report).to receive(:status).and_return("changed") + allow(File).to receive(:mtime).and_return(start_time) allow(File).to receive(:exist?).and_return(true) - allow_any_instance_of(Object).to receive(:check_config_print).and_return("/opt/puppetlabs/puppet/cache/state/last_run_report.yaml") - allow(YAML).to receive(:load_file).and_return(last_run_report) + allow(YAML).to receive(:load_file).with(last_run_report_path).and_return(last_run_report) - expect(get_result_from_report(-1, default_configuration, start_time)).to be == + expect(get_result_from_report(last_run_report_path, -1, default_configuration, start_time)).to be == {"kind" => "unknown", "time" => "unknown", "transaction_uuid" => "unknown", "environment" => "unknown", "status" => "unknown", - "error_type" => "agent_exit_non_zero", - "error" => "Puppet agent exited with a non 0 exitcode", + "error_type" => "no_last_run_report", + "error" => "#{last_run_report_path} was not written", "exitcode" => -1, "version" => 1} end @@ -171,11 +175,11 @@ allow(last_run_report).to receive(:environment).and_return("production") allow(last_run_report).to receive(:status).and_return("changed") + allow(File).to receive(:mtime).and_return(start_time+1) allow(File).to receive(:exist?).and_return(true) - allow_any_instance_of(Object).to receive(:check_config_print).and_return("/opt/puppetlabs/puppet/cache/state/last_run_report.yaml") - allow(YAML).to receive(:load_file).and_return(last_run_report) + allow(YAML).to receive(:load_file).with(last_run_report_path).and_return(last_run_report) - expect(get_result_from_report(-1, default_configuration, start_time)).to be == + expect(get_result_from_report(last_run_report_path, -1, default_configuration, start_time)).to be == {"kind" => "apply", "time" => run_time, "transaction_uuid" => "ac59acbe-6a0f-49c9-8ece-f781a689fda9", @@ -198,11 +202,11 @@ allow(last_run_report).to receive(:environment).and_return("production") allow(last_run_report).to receive(:status).and_return("changed") + allow(File).to receive(:mtime).and_return(start_time+1) allow(File).to receive(:exist?).and_return(true) - allow_any_instance_of(Object).to receive(:check_config_print).and_return("/opt/puppetlabs/puppet/cache/state/last_run_report.yaml") - allow(YAML).to receive(:load_file).and_return(last_run_report) + allow(YAML).to receive(:load_file).with(last_run_report_path).and_return(last_run_report) - expect(get_result_from_report(0, default_configuration, start_time)).to be == + expect(get_result_from_report(last_run_report_path, 0, default_configuration, start_time)).to be == {"kind" => "apply", "time" => run_time, "transaction_uuid" => "ac59acbe-6a0f-49c9-8ece-f781a689fda9", @@ -218,17 +222,25 @@ double(:runoutcome) } + let(:last_run_report) { + "/opt/puppetlabs/puppet/cache/state/last_run_report.yaml" + } + + before :each do + allow_any_instance_of(Object).to receive(:check_config_print).and_return(last_run_report) + end + it "populates output when it terminated normally" do allow(Puppet::Util::Execution).to receive(:execute).and_return(runoutcome) allow(runoutcome).to receive(:exitstatus).and_return(0) - expect_any_instance_of(Object).to receive(:get_result_from_report).with(0, default_configuration, anything) + expect_any_instance_of(Object).to receive(:get_result_from_report).with(last_run_report, 0, default_configuration, anything) start_run(default_configuration, default_input) end it "populates output when it terminated with a non 0 code" do allow(Puppet::Util::Execution).to receive(:execute).and_return(runoutcome) allow(runoutcome).to receive(:exitstatus).and_return(1) - expect_any_instance_of(Object).to receive(:get_result_from_report).with(1, default_configuration, anything) + expect_any_instance_of(Object).to receive(:get_result_from_report).with(last_run_report, 1, default_configuration, anything) start_run(default_configuration, default_input) end