Merge lp:~robru/cupstream2distro/fix-locking into lp:cupstream2distro

[Robert Bruce Park]

Fix silo locking properly.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1180
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/855/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/855/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1181
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/856/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/856/rebuild

review: Approve (continuous-integration)

[Barry Warsaw (barry)]

High level comment: while it's not unreasonable to use the low-level flock
interface to do simple locking, you might want to consider using one of
several library alternatives. Locking can be tricky and there may be corner
cases lurking, and the libraries have generally been more thoroughly tested.
Totally up to you of course, so I'm just putting this out there.

The two I like are flufl.lock and lockfile, both of which have Python 2 and 3
versions in the archive. Obviously I wrote flufl.lock and it's the only one
that I'm aware of that is safe across NFS mounts

http://pythonhosted.org/flufl.lock/
http://pythonhosted.org/lockfile/

(We should be up-to-date with flufl.lock, but I think lockfile is a minor rev
behind upstream - I'll ping the maintainer.)

Take a look at the semantics of the two packages; they differ so one might be
more appropriate to your use case than the other.

Aside from that...

On Oct 27, 2015, at 10:42 PM, Robert Bruce Park wrote:

>=== modified file 'cupstream2distro/silomanager.py'
>--- cupstream2distro/silomanager.py 2015-10-22 18:10:30 +0000
>+++ cupstream2distro/silomanager.py 2015-10-27 22:41:56 +0000
>@@ -22,6 +22,7 @@
> import re
> import os
> import json
>+import fcntl
> import logging
> import requests
> import signal
>@@ -47,7 +48,6 @@
> STABLE_OVERLAY_PPA,
> )
> from cupstream2distro.utils import (
>- atomic_write,
> env,
> memoize,
> log_value_of,
>@@ -197,8 +197,7 @@
> if self.strict:
> signal.signal(signal.SIGTERM, self.cleanup_on_kill)
> sys.excepthook = self.cleanup_on_exception
>- with atomic_write(self.lockfile, 'w') as lock:
>- lock.write(str(os.getpid()))
>+ self.enforce_lock()

The trickiest part of locks IMHO is ensuring you don't get deadlocked in the
face of exceptions and errors. The context manager syntax (i.e. the `with`
statement) pretty much guarantees that the locked resource will be freed if
anything goes wrong. Of course, the process could still be kill -9'd, etc,
but that's why for example flufl.lock has a timeout feature. Eventually, all
locks will auto-expire.

How confident are you that there aren't corner cases which would deadlock the
silo?

[Robert Bruce Park (robru)]

On Oct 28, 2015 8:36 AM, "Barry Warsaw" <email address hidden> wrote:
> How confident are you that there aren't corner cases which would deadlock
the
> silo?

Very confident, because this isn't long running code. Any error or
exception will cause the process to exit, which as far as i can tell,
guarantees the file handle be closed and the lock released.

Also NFS mounts aren't a concern but I'll take a look at flufl in a sec.

[Robert Bruce Park (robru)]

Ok I've just tested this further, and not even os._exit() can achieve a deadlock. It seems flock is smart enough to clear the lock when the process exits even if it doesn't exit cleanly, so I'm gonna go ahead and merge this.

review: Approve

[Robert Bruce Park (robru)]

(also confirmed kill -9 releases the lock too)

[Barry Warsaw (barry)]

Stevens confirms flocks are released at process exit and fd closure.

[lp:~robru/cupstream2distro/fix-locking]

1182. By Robert Bruce Park on 2015-10-28

Clean up behavior when trying to run two jobs simultaneously.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1182
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/857/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/857/rebuild

review: Approve (continuous-integration)

[lp:~robru/cupstream2distro/fix-locking]

1183. By Robert Bruce Park on 2015-10-28

Unify redundant BlockingIOError handling.

1184. By Robert Bruce Park on 2015-10-28

Drop problematic finally clause.

1185. By Robert Bruce Park on 2015-10-28

finally: clause triggers even on unhandled exceptions and I do not want that.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1185
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/858/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/858/rebuild

review: Approve (continuous-integration)

=== modified file 'citrain/build.py'
--- citrain/build.py	2015-10-27 19:35:44 +0000
+++ citrain/build.py	2015-10-28 19:46:37 +0000
@@ -88,10 +88,11 @@
     except CITrainError as err:
         logging.error(str(err))
         silo_state.status = 'Build failed: ' + str(err)
-        return 1
-    finally:
         silo_state.mark_dirty()
         silo_state.save_config()
+        return 1
+    silo_state.mark_dirty()
+    silo_state.save_config()
     return 0
 
 

=== modified file 'citrain/merge_clean.py'
--- citrain/merge_clean.py	2015-10-27 19:35:44 +0000
+++ citrain/merge_clean.py	2015-10-28 19:46:37 +0000
@@ -52,9 +52,9 @@
     except CITrainError as err:
         logging.error(str(err))
         silo_state.status = 'Merge&Clean failed: ' + str(err)
+        silo_state.save_config()
         return 1
-    finally:
-        silo_state.save_config()
+    silo_state.save_config()
     return 0
 
 

=== modified file 'citrain/migration.py'
--- citrain/migration.py	2015-10-27 19:35:44 +0000
+++ citrain/migration.py	2015-10-28 19:46:37 +0000
@@ -45,8 +45,8 @@
         BuildBase.failures.clear()
         try:
             silo_state.enforce_lock()
-        except CITrainError as err:
-            logging.info(str(err) + '\n')
+        except BlockingIOError:
+            logging.info('Silo is in use, skipping.\n')
             continue
 
         if 'dirty' not in silo_state.tokenize():

=== modified file 'citrain/publisher.py'
--- citrain/publisher.py	2015-10-27 19:35:44 +0000
+++ citrain/publisher.py	2015-10-28 19:46:37 +0000
@@ -60,9 +60,9 @@
         logging.error(str(err))
         if type(err) is not NoStatusError:
             silo_state.status = 'Publish failed: ' + str(err)
+        silo_state.save_config()
         return 1
-    finally:
-        silo_state.save_config()
+    silo_state.save_config()
     # This bit needs to be in a separate block because it's so slow that it
     # makes the last call to save_config() end up losing races with
     # check-publication-migration. Then you get logs that say

=== modified file 'cupstream2distro/silomanager.py'
--- cupstream2distro/silomanager.py	2015-10-28 19:05:41 +0000
+++ cupstream2distro/silomanager.py	2015-10-28 19:46:37 +0000
@@ -231,10 +231,7 @@
 
     def enforce_lock(self):
         """Raise PrepError if this silo is locked by a running process."""
-        try:
-            fcntl.flock(self.lock_fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-        except BlockingIOError:
-            raise PrepError('Silo {} is locked.'.format(self))
+        fcntl.flock(self.lock_fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
 
     def assign(self, requestid):
         """Assign a new requestid to an available silo."""

=== modified file 'cupstream2distro/utils.py'
--- cupstream2distro/utils.py	2015-10-19 21:34:21 +0000
+++ cupstream2distro/utils.py	2015-10-28 19:46:37 +0000
@@ -187,7 +187,11 @@
         if set(['--help', '-h']) & set(sys.argv):
             sys.stdout.write(doc)
         else:
-            sys.exit(main())
+            try:
+                sys.exit(main())
+            except BlockingIOError:
+                logging.error('Another job is already running in this silo.')
+                sys.exit(2)
 
 
 def call(*args, **kwargs):

=== modified file 'tests/unit/test_script_migration.py'
--- tests/unit/test_script_migration.py	2015-10-21 23:12:36 +0000
+++ tests/unit/test_script_migration.py	2015-10-28 19:46:37 +0000
@@ -75,7 +75,7 @@
     def test_main_skip_locked_silos(self):
         """Skip over silos that are currently used by other processes."""
         states = self.script.SiloState.iterate.return_value = [Mock()]
-        states[0].enforce_lock.side_effect = self.script.CITrainError
+        states[0].enforce_lock.side_effect = BlockingIOError
         self.assertEqual(self.script.main(), 0)
         self.assertEqual(self.script.Manager.mock_calls, [])
         self.assertEqual(self.script.merge_main.mock_calls, [])

=== modified file 'tests/unit/test_silomanager.py'
--- tests/unit/test_silomanager.py	2015-10-27 22:40:32 +0000
+++ tests/unit/test_silomanager.py	2015-10-28 19:46:37 +0000
@@ -126,7 +126,7 @@
         for silo in ('one', 'two', 'three'):
             os.mkdir(join(self.tempdir, silo))
             states.append(SiloState(silo, primary=True))
-        with self.assertRaisesRegexp(PrepError, 'Silo two is locked'):
+        with self.assertRaises(BlockingIOError):
             SiloState('two', primary=True)
         del states[1]
         SiloState('two', primary=True)  # No exception raised.

=== modified file 'tests/unit/test_utils.py'
--- tests/unit/test_utils.py	2015-09-24 05:06:40 +0000
+++ tests/unit/test_utils.py	2015-10-28 19:46:37 +0000
@@ -136,6 +136,14 @@
             self.assertEqual(sys_mock.exit.mock_calls, [])
             sys_mock.stdout.write.assert_called_with(helper * 5)
 
+    @patch('cupstream2distro.utils.sys')
+    def test_run_script_locked(self, sys_mock):
+        """Log & exit when silo is locked."""
+        main_mock = Mock(side_effect=BlockingIOError)
+        sys_mock.argv = []
+        run_script('__main__', 'docbrown', main_mock)
+        sys_mock.exit.assert_called_once_with(2)
+
     def test_singleton(self):
         """There Can Be Only One."""
         @singleton