Merge lp:~allenap/maas/avoid-long-held-config-locks into lp:~maas-committers/maas/trunk

[Gavin Panella]

Don't hold a lock on the cluster configuration while finding boot images.

Log a warning when a lock has been held for too long.

[Gavin Panella (allenap)]

Just saying, but none of this would have been a problem if we had stayed with the SQLite-based configuration class.

[Andres Rodriguez (andreserl)]

lgtm! The big question, however, is why would the SQLite-based configuration would have avoided this problem, when the problem that we were actually seeing was related to the MAAS import process? (at least that was in the Lab)

review: Approve

[Gavin Panella (allenap)]

> lgtm! The big question, however, is why would the SQLite-based
> configuration would have avoided this problem, when the problem that
> we were actually seeing was related to the MAAS import process? (at
> least that was in the Lab)

It would have prevented the problem of exclusive locking; everyone could
have shared the configuration safely.

[Gavin Panella (allenap)]

Thanks for the review!

=== modified file 'src/provisioningserver/config.py'
--- src/provisioningserver/config.py	2015-08-13 20:25:12 +0000
+++ src/provisioningserver/config.py	2015-08-20 16:00:57 +0000
@@ -125,7 +125,9 @@
     contextmanager,
 )
 from copy import deepcopy
+from itertools import islice
 import json
+import logging
 import os
 from os import environ
 import os.path
@@ -133,6 +135,8 @@
 from shutil import copyfile
 import sqlite3
 from threading import RLock
+from time import time
+import traceback
 import uuid
 
 from formencode import (
@@ -158,6 +162,9 @@
 )
 import yaml
 
+
+logger = logging.getLogger(__name__)
+
 # Default result for cluster UUID if not set
 UUID_NOT_SET = '** UUID NOT SET **'
 
@@ -536,20 +543,34 @@
         **Note** that this returns a context manager which will save changes
         to the configuration on a clean exit.
         """
-        # Only one reader or writer at a time.
-        with RunLock(path).wait(timeout=5):
-            # Ensure `path` exists...
-            touch(path)
-            # before loading it in.
-            configfile = cls(path)
-            configfile.load()
-            try:
-                yield configfile
-            except:
-                raise
-            else:
-                if configfile.dirty:
-                    configfile.save()
+        time_opened = None
+        try:
+            # Only one reader or writer at a time.
+            with RunLock(path).wait(timeout=5.0):
+                time_opened = time()
+                # Ensure `path` exists...
+                touch(path)
+                # before loading it in.
+                configfile = cls(path)
+                configfile.load()
+                try:
+                    yield configfile
+                except:
+                    raise
+                else:
+                    if configfile.dirty:
+                        configfile.save()
+        finally:
+            if time_opened is not None:
+                time_open = time() - time_opened
+                if time_open >= 2.5:
+                    mini_stack = ", from ".join(
+                        "%s:%d" % (fn, lineno) for fn, lineno, _, _ in
+                        islice(reversed(traceback.extract_stack()), 2, 5))
+                    logger.warn(
+                        "Configuration file %s locked for %.1f seconds; this "
+                        "may starve other processes. Called from %s.", path,
+                        time_open, mini_stack)
 
 
 class ConfigurationMeta(type):

=== modified file 'src/provisioningserver/rpc/boot_images.py'
--- src/provisioningserver/rpc/boot_images.py	2015-06-25 22:33:37 +0000
+++ src/provisioningserver/rpc/boot_images.py	2015-08-20 16:00:57 +0000
@@ -44,7 +44,8 @@
     global CACHED_BOOT_IMAGES
     if CACHED_BOOT_IMAGES is None:
         with ClusterConfiguration.open() as config:
-            CACHED_BOOT_IMAGES = tftppath.list_boot_images(config.tftp_root)
+            tftp_root = config.tftp_root
+        CACHED_BOOT_IMAGES = tftppath.list_boot_images(tftp_root)
     return CACHED_BOOT_IMAGES