Merge lp:~allenap/maas/database-locks-rererevisited into lp:~maas-committers/maas/trunk

[Gavin Panella]

Use non-transactional advisory locks for critical regions of DNS and security activity.

Previously locks were obtained after transactions had begun. This meant that, once the lock was acquired, the transaction may be working with stale data. This causes breakage when dealing with the shared-secret on the filesystem and when writing DNS zones with old data.

[Blake Rouse (blake-rouse)]

Looks good as well. No bug for this either?

review: Approve

[Gavin Panella (allenap)]

> Looks good as well. No bug for this either?

I'm going to go with bug 1459168. Thanks again!

[Gavin Panella (allenap)]

FTR, I QA'ed this at home first, and it seems to work as intended.

[Andres Rodriguez (andreserl)]

Does it make sense to backport this to 1.8 ?

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/database-locks-rererevisited into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://security.ubuntu.com trusty-security Release
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing python-seamicroclient python...

=== modified file 'src/maasserver/dns/config.py'
--- src/maasserver/dns/config.py	2015-07-01 18:58:34 +0000
+++ src/maasserver/dns/config.py	2015-08-17 16:24:03 +0000
@@ -14,11 +14,8 @@
 __metaclass__ = type
 __all__ = [
     'dns_add_zones',
-    'dns_add_zones_now',
     'dns_update_all_zones',
-    'dns_update_all_zones_now',
     'dns_update_zones',
-    'dns_update_zones_now',
     ]
 
 from itertools import (
@@ -46,6 +43,7 @@
 from maasserver.utils.orm import (
     post_commit,
     transactional,
+    with_connection,
 )
 from provisioningserver.dns.actions import (
     bind_reconfigure,
@@ -96,7 +94,6 @@
 DNS_DEFER_UPDATES = True
 
 
-@synchronised(locks.dns)
 def dns_add_zones_now(clusters):
     """Add zone files for the given cluster(s), and serve them.
 
@@ -135,7 +132,6 @@
         return None
 
 
-@synchronised(locks.dns)
 def dns_update_zones_now(clusters):
     """Update the zone files for the given cluster(s).
 
@@ -169,7 +165,6 @@
         return None
 
 
-@synchronised(locks.dns)
 def dns_update_all_zones_now(reload_retry=False, force=False):
     """Update all zone files for all clusters.
 
@@ -270,8 +265,9 @@
             self.hook.addBoth(callOut, self.reset)
         return self.hook
 
+    @with_connection  # Needed by the following lock.
+    @synchronised(locks.dns)  # Lock before beginning transaction.
     @transactional
-    @synchronised(locks.dns)
     def apply(self):
         """Apply all requested changes.
 

=== modified file 'src/maasserver/dns/tests/test_config.py'
--- src/maasserver/dns/tests/test_config.py	2015-07-01 19:02:36 +0000
+++ src/maasserver/dns/tests/test_config.py	2015-08-17 16:24:03 +0000
@@ -187,6 +187,22 @@
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
             status=NODEGROUP_STATUS.ENABLED)
 
+    def test__zone_changes_applied_while_holding_dns_lock(self):
+
+        def check_dns_is_locked(reload_retry, force):
+            self.assertTrue(locks.dns.is_locked(), "locks.dns isn't locked")
+            return False  # Prevent dns_update_zones_now from continuing.
+
+        dns_update_all_zones_now = self.patch_autospec(
+            dns_config_module, "dns_update_all_zones_now")
+        dns_update_all_zones_now.side_effect = check_dns_is_locked
+
+        consolidator.update_all_zones()
+        post_commit_hooks.fire()
+        self.assertThat(
+            dns_update_all_zones_now, MockCalledOnceWith(
+                reload_retry=ANY, force=ANY))
+
     def test__added_zones_applied_post_commit(self):
         dns_add_zones_now = self.patch_autospec(
             dns_config_module, "dns_add_zones_now")
@@ -413,35 +429,6 @@
                 fqdn, ','.join(reverse_lookup_result)))
 
 
-class TestDNSModificationUsesLocks(TestDNSServer):
-
-    def patch_is_dns_enabled(self):
-        def check_dns_is_locked():
-            self.assertTrue(
-                locks.dns.is_locked(), "locks.dns isn't locked")
-            return False  # Prevent dns_update_zones_now from continuing.
-
-        is_dns_enabled = self.patch_autospec(
-            dns_config_module, 'is_dns_enabled')
-        is_dns_enabled.side_effect = check_dns_is_locked
-        return is_dns_enabled
-
-    def test_dns_update_zones_now_uses_lock(self):
-        is_dns_enabled = self.patch_is_dns_enabled()
-        dns_config_module.dns_update_zones_now(sentinel.nodegroup)
-        self.assertThat(is_dns_enabled, MockCalledOnceWith())
-
-    def test_dns_add_zones_now_uses_lock(self):
-        is_dns_enabled = self.patch_is_dns_enabled()
-        dns_config_module.dns_add_zones_now(sentinel.nodegroup)
-        self.assertThat(is_dns_enabled, MockCalledOnceWith())
-
-    def test_dns_update_all_zones_now_config_uses_lock(self):
-        is_dns_enabled = self.patch_is_dns_enabled()
-        dns_config_module.dns_update_all_zones_now()
-        self.assertThat(is_dns_enabled, MockCalledOnceWith())
-
-
 class TestDNSConfigModifications(TestDNSServer):
 
     def test_dns_add_zones_now_loads_dns_zone(self):

=== modified file 'src/maasserver/locks.py'
--- src/maasserver/locks.py	2015-08-17 13:22:49 +0000
+++ src/maasserver/locks.py	2015-08-17 16:24:03 +0000
@@ -28,7 +28,7 @@
 
 # Lock around performing critical security-related operations, like
 # generating or signing certificates.
-security = DatabaseXactLock(2)
+security = DatabaseLock(2)
 
 # Lock used when starting up the event-loop.
 eventloop = DatabaseLock(3)
@@ -38,7 +38,7 @@
 import_images = DatabaseXactLock(4)
 
 # Lock to prevent concurrent changes to DNS configuration.
-dns = DatabaseXactLock(6)
+dns = DatabaseLock(6)
 
 # Lock to prevent concurrent acquisition of nodes.
 node_acquire = DatabaseXactLock(7)

=== modified file 'src/maasserver/security.py'
--- src/maasserver/security.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/security.py	2015-08-17 16:24:03 +0000
@@ -22,7 +22,11 @@
 
 from maasserver import locks
 from maasserver.models.config import Config
-from maasserver.utils.orm import transactional
+from maasserver.utils import synchronised
+from maasserver.utils.orm import (
+    transactional,
+    with_connection,
+)
 from provisioningserver.security import (
     get_shared_secret_filesystem_path,
     get_shared_secret_from_filesystem,
@@ -72,52 +76,50 @@
 
 
 @synchronous
+@with_connection  # Needed by the following lock.
+@synchronised(locks.security)  # Region-wide lock.
 @transactional
 def get_region_certificate():
     cert = load_region_certificate()
     if cert is None:
-        with locks.security:
-            # Load again, while holding the security lock.
-            cert = load_region_certificate()
-            if cert is None:
-                cert = generate_region_certificate()
-                save_region_certificate(cert)
+        cert = generate_region_certificate()
+        save_region_certificate(cert)
     return cert
 
 
 @synchronous
+@with_connection  # Needed by the following lock.
+@synchronised(locks.security)  # Region-wide lock.
 @transactional
 def get_shared_secret_txn():
-    # Get a full region-wide lock.
-    with locks.security:
-        # Load secret from database, if it exists.
-        secret_in_db_hex = Config.objects.get_config("rpc_shared_secret")
-        if secret_in_db_hex is None:
-            secret_in_db = None
-        else:
-            secret_in_db = to_bin(secret_in_db_hex)
-        # Load secret from the filesystem, if it exists.
-        secret_on_fs = get_shared_secret_from_filesystem()
-
-        if secret_in_db is None and secret_on_fs is None:
-            secret = os.urandom(16)  # 16-bytes of crypto-standard noise.
-            Config.objects.set_config("rpc_shared_secret", to_hex(secret))
-            set_shared_secret_on_filesystem(secret)
-        elif secret_in_db is None:
-            secret = secret_on_fs
-            Config.objects.set_config("rpc_shared_secret", to_hex(secret))
-        elif secret_on_fs is None:
-            secret = secret_in_db
-            set_shared_secret_on_filesystem(secret)
-        elif secret_in_db == secret_on_fs:
-            secret = secret_in_db  # or secret_on_fs.
-        else:
-            raise AssertionError(
-                "The secret stored in the database does not match the secret "
-                "stored on the filesystem at %s. Please investigate." %
-                get_shared_secret_filesystem_path())
-
-        return secret
+    # Load secret from database, if it exists.
+    secret_in_db_hex = Config.objects.get_config("rpc_shared_secret")
+    if secret_in_db_hex is None:
+        secret_in_db = None
+    else:
+        secret_in_db = to_bin(secret_in_db_hex)
+    # Load secret from the filesystem, if it exists.
+    secret_on_fs = get_shared_secret_from_filesystem()
+
+    if secret_in_db is None and secret_on_fs is None:
+        secret = os.urandom(16)  # 16-bytes of crypto-standard noise.
+        Config.objects.set_config("rpc_shared_secret", to_hex(secret))
+        set_shared_secret_on_filesystem(secret)
+    elif secret_in_db is None:
+        secret = secret_on_fs
+        Config.objects.set_config("rpc_shared_secret", to_hex(secret))
+    elif secret_on_fs is None:
+        secret = secret_in_db
+        set_shared_secret_on_filesystem(secret)
+    elif secret_in_db == secret_on_fs:
+        secret = secret_in_db  # or secret_on_fs.
+    else:
+        raise AssertionError(
+            "The secret stored in the database does not match the secret "
+            "stored on the filesystem at %s. Please investigate." %
+            get_shared_secret_filesystem_path())
+
+    return secret
 
 
 @asynchronous(timeout=10)