Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PCP-306) Use last_run_report mtime to check for updates #416

Merged
merged 2 commits into from
May 10, 2016

Conversation

MikaelSmith
Copy link
Contributor

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.

@MikaelSmith
Copy link
Contributor Author

MikaelSmith commented May 4, 2016

/cc @nicklewis

The only case where I see this getting us into trouble is if the environment undergoes some sort of rollback or someone manually copies an old report into place after a run. Both of those seem unlikely.

Update: ok, I can think of another case. Updating time could result in the new catalog being written with an identical mtime. This seems unlikely, as time is measured in microseconds, and will only result in an unknown status. However it could be improved by checking checksum, which should be unique because transaction_uuid will change with each run. Could also compare transaction_uuid or time from the report, but that seems potentially more expensive and error prone (parsing JSON).

@MikaelSmith
Copy link
Contributor Author

AppVeyor won't pass until #414 is merged.

@parisiale
Copy link
Contributor

AppVeyor is happy after #414 fix.
👍

Pinging @nicklewis.

Update links to point to version 1.0. The pcp-specifications repo was
reorganized to separate different versions of the specification.
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.
@parisiale
Copy link
Contributor

Also, pinging @mruzicka.

@nicklewis
Copy link
Contributor

This approach makes sense to me.

@parisiale
Copy link
Contributor

Will merge as soon as puppet-agent CI is back to green.

@parisiale parisiale merged commit 5ad7e3b into puppetlabs:master May 10, 2016
@MikaelSmith MikaelSmith deleted the PCP-306 branch May 11, 2016 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants