launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-820796 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #820796 in Launchpad itself: "Lock before publishing"
  https://bugs.launchpad.net/launchpad/+bug/820796

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-820796/+merge/70701

= Summary =

The publish-ftpmaster script invokes the process-accepted and publish-distro scripts.  Each of those operates on distro archives held in the filesystem, and could experience ill effects from concurrent runs on the same filesystem.

They can, however, all run concurrently on separate filesystems, since those will hold different archives.  Unless we introduce network-shared drives for either the archives or the locks, that is.  We're not planning the latter, but we are planning to run the scripts for the collective derived distros on a single dedicated server separate from the one we use for Ubuntu.

The conflicts between script runs get a little too complex for fine-grained locking in the presence of different types of archive (distro, PPA, copy, partner, debug), different distributions (Ubuntu and its derivates; we don't publish Debian), and multi-distro runs.  We could in principle grab multiple locks per script run, but that gets hairy and we currently have no reason to believe it's required.


== Proposed fix ==

A single global lock for the publisher scripts.  The lock is held in the filesystem, so there's one instance per server which suits us just fine.


== Pre-implementation notes ==

William claims that archive admins may sometimes run publish-distro or process-accepted manually, or perhaps just publish-ftpmaster which in turn runs the other two.  Julian is not aware of any such usage and feels it would be irresponsible to try without disabling the publishing cron jobs first.

What this boils down to is "we see no immediate danger but the distro archives are worth a pair of braces in addition to the belt."

For the derived distros we certainly have no practice of manual runs, and that's exactly where we would expect the most false contention between scripts.


== Implementation details ==

One of the scripts neglected to lock.  That's fixed here.

You'll also see another extraneous change: the little Ubuntu-specific plugin script that signs Releases files was annoyingly breaksome on dogfood, which doesn't have the required GPG setup.  Rather than continually working arouhd this, I made the script tolerate (but log) the absence of the GPG setup.


== Tests ==

I failed to come up with any worthwhile tests.  All the changes are essentially changes in policy, not mechanism.  There's little point in testing that each of the scripts I changed has the expected lockfilename variable set.

On the other hand it's just too damned costly to test that the scripts really do create the expected lock file.  We'd have to fork off a script instance (which takes a while), wait somehow for it to get to the right point in its code path, check for the lock file, and then cause the script to continue or terminate.  I verified manually instead.


== Demo and Q/A ==

Try running any combination of these scripts concurrently.  Only the first one should succeed.


= Launchpad lint =

There's one bit of expected lint, but nothing new.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/scripts/publishdistro.py
  lib/lp/archivepublisher/publishing.py
  scripts/publish-distro.py
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/soyuz/scripts/processaccepted.py
  cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/10-sign-releases

./scripts/publish-distro.py
       8: '_pythonpath' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/bug-820796/+merge/70701
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-820796 into lp:launchpad.