Skip to content

Commit

Permalink
(PCP-306) Use last_run_report mtime to check for updates
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@MikaelSmith
MikaelSmith committed May 5, 2016
1 parent adf2560 commit 0a4f4f4
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 34 deletions.
33 changes: 18 additions & 15 deletions modules/pxp-module-puppet
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand All @@ -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
Expand All @@ -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})
Expand All @@ -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()
Expand Down
50 changes: 31 additions & 19 deletions modules/spec/unit/modules/puppet_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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

Expand Down

0 comments on commit 0a4f4f4

Please sign in to comment.