0.9.0: Pull gpg homedir creation out of the main scriptworker process. #22

Merged
merged 5 commits into from Nov 1, 2016

Projects

None yet

3 participants

@escapewindow
Member

Per the commit message:

Originally, gpg homedir creation happened before scriptworker launched,
and then a background async task polled git and rebuilt the gpg homedirs
if it found a new valid git commit.  In order to sign the gpg keys in
the background, we used async pexpect.

However, as noted in #21, async pexpect isn't reliable currently.  Our
options included fixing the upstream pexpect bug or pulling the gpg
homedir creation into a separate process that we can call via cron.

This patch allows us to use that latter approach.

Fixes #21 .

@JohanLorenzo , I'll let you thumb wrestle with @lundjordan with review now that he's back :) I'm happy to have either of you review. Thanks to both of you!

I'm going to release scriptworker 0.9.0 off this PR once it's approved.

escapewindow added some commits Oct 28, 2016
@escapewindow escapewindow Pull gpg homedir creation out of the main scriptworker process.
Originally, gpg homedir creation happened before scriptworker launched,
and then a background async task polled git and rebuilt the gpg homedirs
if it found a new valid git commit.  In order to sign the gpg keys in
the background, we used async pexpect.

However, as noted in #21, async pexpect isn't reliable currently.  Our
options included fixing the upstream pexpect bug or pulling the gpg
homedir creation into a separate process that we can call via cron.

This patch allows us to use that latter approach.
c71530d
@escapewindow escapewindow update changelog
b8eda20
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling b8eda20 on escapewindow:gpg-sync into 97dcafb on mozilla-releng:master.

@escapewindow escapewindow stricter gpg homedir lockfile usage.
Previously, we were creating the gpg lockfile before recreating the
worker gpg homedirs, but after recreating ~/.gnupg to verify the git
repo commits.  This meant we were recreating ~/.gnupg every five
minutes, and could have resulted in a long-running process overrunning
the next, especially if we ever shorten the frequency.

Now we create the lockfile before doing any of it, and clean up after
we're done.  I plan to file a bug about nagios checks for the age of the
lockfile and the various scriptworker logs, so we catch if any of these
take significantly longer than expected.
58e4b1a
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 58e4b1a on escapewindow:gpg-sync into 97dcafb on mozilla-releng:master.

@JohanLorenzo
JohanLorenzo approved these changes Nov 1, 2016 edited View changes

End of 👍 wrestling 😄

Your changes make sense to me. I just noticed a few nits. Thanks!

scriptworker/gpg.py
+ )
+ try:
+ with open(context.config['last_good_git_revision_file'], "w") as fh:
+ print(revision, file=fh, end='')
@JohanLorenzo
JohanLorenzo Nov 1, 2016 Contributor

Out of curiosity, what's the difference with fh.write(revision)?

@escapewindow
escapewindow Nov 1, 2016 Member

Looks like the same thing :)

scriptworker/gpg.py
+ with open(context.config['last_good_git_revision_file'], "w") as fh:
+ print(revision, file=fh, end='')
+ except OSError as exc:
+ raise ScriptWorkerGPGException(str(exc))
@JohanLorenzo
JohanLorenzo Nov 1, 2016 Contributor

Shouldn't we wrap the exception under another type of Error? IIUC, the current usage of ScriptWorkerGPGException is limited to actual GPG operations.

@escapewindow
escapewindow Nov 1, 2016 Member

Now that we're only calling this from external endpoints, I think we don't have to catch the OSError at all. It does the trick and kills the process, while removing the lockfile in the finally block. I removed the try/except block.

scriptworker/gpg.py
-def create_initial_gpg_homedirs():
- """Create the initial gpg homedirs.
+def check_lockfile(context, name):
@JohanLorenzo
JohanLorenzo Nov 1, 2016 Contributor

check_lockfile() sounds ambiguous to me. I'm not sure if True means whether the file exists or I'm good to proceed. How about is_lockfile_present()?

@escapewindow
escapewindow Nov 1, 2016 Member

sounds good to me!

scriptworker/gpg.py
+ if os.path.exists(lockfile):
+ log.warning("Skipping {}: lockfile {} exists!".format(name, lockfile))
+ return True
+
@JohanLorenzo
JohanLorenzo Nov 1, 2016 Contributor

Nit: False doesn't seem returned even though the documentation reads a bool is returned

escapewindow added some commits Nov 1, 2016
@escapewindow escapewindow address review comments f59475a
@escapewindow escapewindow 0.9.0
2f8e623
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 2f8e623 on escapewindow:gpg-sync into 97dcafb on mozilla-releng:master.

@escapewindow escapewindow merged commit b401d2f into mozilla-releng:master Nov 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@escapewindow
Member

Thanks again!

@escapewindow escapewindow deleted the escapewindow:gpg-sync branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment