Merge lp:~blake-rouse/maas/fix-cluster-image-syncing into lp:~maas-committers/maas/trunk

[Blake Rouse]

Fix GetBootSources* RPC to return simplestreams endpoint on the region. Move the service_lock into rpc import images, so other RPC calls will block from running the import at the same time. Cleanup log messages from the import on the cluster side.

[Gavin Panella (allenap)]

This is cool stuff :) It looks good, but I'm teetering on the edge of Needs Fixing for a couple of things, but Approve has it. However, please address (or put me right on) my comments before landing.

review: Approve

=== removed file 'src/maasserver/rpc/bootsources.py'
--- src/maasserver/rpc/bootsources.py	2014-08-26 17:32:40 +0000
+++ src/maasserver/rpc/bootsources.py	1970-01-01 00:00:00 +0000
@@ -1,46 +0,0 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""RPC helpers relating to boot sources."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
-__all__ = [
-    "get_boot_sources",
-]
-
-from maasserver.models import BootSource
-from maasserver.utils.async import transactional
-from provisioningserver.utils.twisted import synchronous
-
-
-@synchronous
-@transactional
-def get_boot_sources(uuid, remove_os=False):
-    """Obtain boot sources and selections from the database.
-
-    Returns them as a structure suitable for returning in the response
-    for :py:class:`~provisioningserver.rpc.region.GetBootSources`.
-
-    :param remove_os: Remove the os field from selections. This is
-        used for v1 of get_boot_sources, as it should not include the os field.
-        For v2 of get_boot_sources, the os field is included.
-    """
-    # No longer is uuid used for this, as its now global. The uuid is just
-    # ignored.
-    sources = [
-        source.to_dict()
-        for source in BootSource.objects.all()
-    ]
-    for source in sources:
-        if remove_os:
-            for selection in source['selections']:
-                del selection['os']
-    return sources
 
=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2014-09-05 08:10:06 +0000
+++ src/maasserver/rpc/regionservice.py	2014-09-11 13:00:18 +0000
@@ -30,8 +30,8 @@
     eventloop,
     locks,
     )
+from maasserver.bootresources import get_simplestream_endpoint
 from maasserver.rpc import (
-    bootsources,
     configuration,
     events,
     leases,
@@ -133,9 +133,7 @@
         Implementation of
         :py:class:`~provisioningserver.rpc.region.GetBootSources`.
         """
-        d = deferToThread(bootsources.get_boot_sources, uuid, remove_os=True)
-        d.addCallback(lambda sources: {b"sources": sources})
-        return d
+        return {b"sources": [get_simplestream_endpoint()]}
 
     @region.GetBootSourcesV2.responder
     def get_boot_sources_v2(self, uuid):
@@ -144,9 +142,7 @@
         Implementation of
         :py:class:`~provisioningserver.rpc.region.GetBootSources`.
         """
-        d = deferToThread(bootsources.get_boot_sources, uuid)
-        d.addCallback(lambda sources: {b"sources": sources})
-        return d
+        return {b"sources": [get_simplestream_endpoint()]}
 
     @region.GetArchiveMirrors.responder
     def get_archive_mirrors(self):
 
=== removed file 'src/maasserver/rpc/tests/test_bootsources.py'
--- src/maasserver/rpc/tests/test_bootsources.py	2014-09-05 16:38:32 +0000
+++ src/maasserver/rpc/tests/test_bootsources.py	1970-01-01 00:00:00 +0000
@@ -1,42 +0,0 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for :py:module:`~maasserver.rpc.bootsources`."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
-__all__ = []
-
-from maasserver.rpc.bootsources import get_boot_sources
-from maasserver.testing.factory import factory
-from maasserver.testing.testcase import MAASServerTestCase
-
-
-class TestGetBootSources(MAASServerTestCase):
-
-    def test_returns_boot_sources_and_selections(self):
-        keyring = factory.make_bytes()
-        nodegroup = factory.make_NodeGroup()
-        source = factory.make_BootSource(keyring_data=keyring)
-        factory.make_BootSourceSelection(source)
-
-        expected = source.to_dict()
-        self.assertEqual([expected], get_boot_sources(nodegroup.uuid))
-
-    def test_removes_os_from_boot_source_selections(self):
-        keyring = factory.make_bytes()
-        nodegroup = factory.make_NodeGroup()
-        source = factory.make_BootSource(keyring_data=keyring)
-        factory.make_BootSourceSelection(source)
-
-        expected = source.to_dict()
-        del expected['selections'][0]['os']
-        self.assertEqual(
-            [expected], get_boot_sources(nodegroup.uuid, remove_os=True))
 
=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2014-09-08 20:07:07 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-09-11 13:00:18 +0000
@@ -31,6 +31,7 @@
     eventloop,
     locks,
     )
+from maasserver.bootresources import get_simplestream_endpoint
 from maasserver.enum import (
     NODE_STATUS,
     POWER_STATE,
@@ -331,37 +332,18 @@
         self.assertIsNot(responder, None)
 
     @wait_for_reactor
-    def test_get_boot_sources_can_be_called(self):
+    def test_get_boot_sources_returns_simplestreams_endpoint(self):
         uuid = factory.make_name("uuid")
 
         d = call_responder(Region(), GetBootSources, {b"uuid": uuid})
 
         def check(response):
-            self.assertEqual({b"sources": []}, response)
+            self.assertEqual(
+                {b"sources": [get_simplestream_endpoint()]},
+                response)
 
         return d.addCallback(check)
 
-    @transactional
-    def make_boot_source_selection(self, keyring):
-        nodegroup = factory.make_NodeGroup()
-        boot_source = factory.make_BootSource(keyring_data=keyring)
-        factory.make_BootSourceSelection(boot_source)
-        return nodegroup.uuid, boot_source.to_dict()
-
-    @wait_for_reactor
-    @inlineCallbacks
-    def test_get_boot_sources_with_real_cluster(self):
-        keyring = factory.make_bytes()
-
-        uuid, boot_source = yield deferToThread(
-            self.make_boot_source_selection, keyring)
-        del boot_source['selections'][0]['os']
-
-        response = yield call_responder(
-            Region(), GetBootSources, {b"uuid": uuid})
-
-        self.assertEqual({b"sources": [boot_source]}, response)
-
 
 class TestRegionProtocol_GetBootSourcesV2(TransactionTestCase):
 
@@ -371,40 +353,18 @@
         self.assertIsNot(responder, None)
 
     @wait_for_reactor
-    def test_get_boot_sources_v2_can_be_called(self):
+    def test_get_boot_sources_v2_returns_simplestreams_endpoint(self):
         uuid = factory.make_name("uuid")
 
         d = call_responder(Region(), GetBootSourcesV2, {b"uuid": uuid})
 
         def check(response):
-            self.assertEqual({b"sources": []}, response)
+            self.assertEqual(
+                {b"sources": [get_simplestream_endpoint()]},
+                response)
 
         return d.addCallback(check)
 
-    @transactional
-    def make_boot_source_selection(self, keyring):
-        nodegroup = factory.make_NodeGroup()
-        boot_source = factory.make_BootSource(keyring_data=keyring)
-        factory.make_BootSourceSelection(boot_source)
-        return nodegroup.uuid, boot_source.to_dict()
-
-    @wait_for_reactor
-    @inlineCallbacks
-    def test_get_boot_sources_v2_with_real_cluster(self):
-        keyring = factory.make_bytes()
-
-        uuid, boot_source = yield deferToThread(
-            self.make_boot_source_selection, keyring)
-
-        # keyring_data contains the b64decoded representation since AMP
-        # is fine with bytes.
-        boot_source["keyring_data"] = keyring
-
-        response = yield call_responder(
-            Region(), GetBootSourcesV2, {b"uuid": uuid})
-
-        self.assertEqual({b"sources": [boot_source]}, response)
-
 
 class TestRegionProtocol_GetArchiveMirrors(MAASTestCase):
 
 
=== modified file 'src/provisioningserver/import_images/boot_resources.py'
--- src/provisioningserver/import_images/boot_resources.py	2014-09-08 06:37:02 +0000
+++ src/provisioningserver/import_images/boot_resources.py	2014-09-11 13:00:18 +0000
@@ -222,7 +222,7 @@
     :param config: An iterable of dicts representing the sources from
         which boot images will be downloaded.
     """
-    maaslog.info("Importing boot resources.")
+    maaslog.info("Started importing boot images.")
     if len(sources) == 0:
         maaslog.warn("Can't import: no Simplestreams sources selected.")
         return
@@ -236,15 +236,13 @@
         image_descriptions = download_all_image_descriptions(sources)
         if image_descriptions.is_empty():
             maaslog.warn(
-                "No boot resources found.  "
-                "Check sources specification and connectivity.")
+                "Finished importing boot images, no boot images available.")
             return
 
         storage = provisioningserver.config.BOOT_RESOURCES_STORAGE
         meta_file_content = image_descriptions.dump_json()
         if meta_contains(storage, meta_file_content):
-            # The current maas.meta already contains the new config.  No need
-            # to rewrite anything.
+            maaslog.info("Finished importing boot images, no new images.")
             return
 
         product_mapping = map_products(image_descriptions)
@@ -252,7 +250,7 @@
         snapshot_path = download_all_boot_resources(
             sources, storage, product_mapping)
 
-    maaslog.info("Writing metadata and iSCSI targets.")
+    maaslog.info("Writing boot image metadata and iSCSI targets.")
     write_snapshot_metadata(snapshot_path, meta_file_content)
     write_targets_conf(snapshot_path)
 
@@ -261,9 +259,9 @@
 
     # If we got here, all went well.  This is now truly the "current" snapshot.
     update_current_symlink(storage, snapshot_path)
-    maaslog.info("Updating iSCSI targets.")
+    maaslog.info("Updating boot image iSCSI targets.")
     update_targets_conf(snapshot_path)
-    maaslog.info("Import done.")
+    maaslog.info("Finished importing boot images.")
 
 
 def main(args):
 
=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2014-09-08 04:08:54 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2014-09-11 13:00:18 +0000
@@ -366,8 +366,7 @@
         self.assertThat(
             boot_resources.maaslog.warn,
             MockAnyCall(
-                "No boot resources found.  "
-                "Check sources specification and connectivity."))
+                "Finished importing boot images, no boot images available."))
 
     def test_raises_ioerror_when_no_sources_file_found(self):
         self.patch_maaslog()
 
=== modified file 'src/provisioningserver/pserv_services/image_download_service.py'
--- src/provisioningserver/pserv_services/image_download_service.py	2014-08-27 06:53:37 +0000
+++ src/provisioningserver/pserv_services/image_download_service.py	2014-09-11 13:00:18 +0000
@@ -33,7 +33,6 @@
     )
 from twisted.application.internet import TimerService
 from twisted.internet.defer import (
-    DeferredLock,
     inlineCallbacks,
     returnValue,
     )
@@ -42,7 +41,6 @@
 
 
 maaslog = get_maas_logger("boot_image_download_service")
-service_lock = DeferredLock()
 
 
 class PeriodicImageDownloadService(TimerService, object):
@@ -117,21 +115,11 @@
         """Check the time the last image refresh happened and initiate a new
         one if older than 15 minutes.
         """
-        # Use a DeferredLock to prevent simultaneous downloads.
-        if service_lock.locked:
-            # Don't want to block on lock release.
+        last_modified = tftppath.maas_meta_last_modified()
+        if last_modified is None:
+            yield self._start_download()
             return
-        yield service_lock.acquire()
-        try:
-            last_modified = tftppath.maas_meta_last_modified()
-            if last_modified is None:
-                # Don't auto-refresh if the user has never manually initiated
-                # a download.
-                return
-
-            age_in_seconds = self.clock.seconds() - last_modified
-            if age_in_seconds >= timedelta(minutes=15).total_seconds():
-                yield self._start_download()
-
-        finally:
-            service_lock.release()
+
+        age_in_seconds = self.clock.seconds() - last_modified
+        if age_in_seconds >= timedelta(minutes=15).total_seconds():
+            yield self._start_download()
 
=== modified file 'src/provisioningserver/pserv_services/tests/test_image_download_service.py'
--- src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-09-01 08:40:13 +0000
+++ src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-09-11 13:00:18 +0000
@@ -34,7 +34,6 @@
 from provisioningserver.boot import tftppath
 from provisioningserver.pserv_services.image_download_service import (
     PeriodicImageDownloadService,
-    service_lock,
     )
 from provisioningserver.rpc import boot_images
 from provisioningserver.rpc.boot_images import _run_import
@@ -45,7 +44,6 @@
     )
 from provisioningserver.rpc.testing import TwistedLoggerFixture
 from provisioningserver.testing.testcase import PservTestCase
-from provisioningserver.utils.twisted import pause
 from testtools.deferredruntest import extract_result
 from twisted.application.internet import TimerService
 from twisted.internet import defer
@@ -99,7 +97,7 @@
         clock.advance(service.check_interval)
         self.assertEqual(3, len(get_mock_calls(maas_meta_last_modified)))
 
-    def test_no_download_if_no_meta_file(self):
+    def test_initiates_download_if_no_meta_file(self):
         clock = Clock()
         service = PeriodicImageDownloadService(
             sentinel.service, clock, sentinel.uuid)
@@ -108,7 +106,7 @@
             tftppath,
             'maas_meta_last_modified').return_value = None
         service.startService()
-        self.assertThat(_start_download, MockNotCalled())
+        self.assertThat(_start_download, MockCalledOnceWith())
 
     def test_initiates_download_if_15_minutes_has_passed(self):
         clock = Clock()
@@ -165,7 +163,7 @@
     def test_no_download_if_no_rpc_connections(self):
         rpc_client = Mock()
         failure = NoConnectionsAvailable()
-        rpc_client.getClient.return_value.side_effect = failure
+        rpc_client.getClient.side_effect = failure
 
         deferToThread = self.patch(boot_images, 'deferToThread')
         service = PeriodicImageDownloadService(
@@ -173,41 +171,6 @@
         service.startService()
         self.assertThat(deferToThread, MockNotCalled())
 
-    @defer.inlineCallbacks
-    def test_does_not_run_if_lock_taken(self):
-        maas_meta_last_modified = self.patch(
-            tftppath, 'maas_meta_last_modified')
-        yield service_lock.acquire()
-        self.addCleanup(service_lock.release)
-        service = PeriodicImageDownloadService(
-            sentinel.rpc, Clock(), sentinel.uuid)
-        service.startService()
-        self.assertThat(maas_meta_last_modified, MockNotCalled())
-
-    def test_takes_lock_when_running(self):
-        clock = Clock()
-        service = PeriodicImageDownloadService(
-            sentinel.rpc, clock, sentinel.uuid)
-
-        # Patch the download func so it's just a Deferred that waits for
-        # one second.
-        _start_download = self.patch(service, '_start_download')
-        _start_download.return_value = pause(1, clock)
-
-        # Set conditions for a required download:
-        one_week_ago = clock.seconds() - timedelta(minutes=15).total_seconds()
-        self.patch(
-            tftppath,
-            'maas_meta_last_modified').return_value = one_week_ago
-
-        # Lock is acquired for the first download after startup.
-        service.startService()
-        self.assertTrue(service_lock.locked)
-
-        # Lock is released once the download is done.
-        clock.advance(1)
-        self.assertFalse(service_lock.locked)
-
     def test_logs_other_errors(self):
         service = PeriodicImageDownloadService(
             sentinel.rpc, Clock(), sentinel.uuid)
 
=== modified file 'src/provisioningserver/rpc/boot_images.py'
--- src/provisioningserver/rpc/boot_images.py	2014-09-02 06:21:04 +0000
+++ src/provisioningserver/rpc/boot_images.py	2014-09-11 13:00:18 +0000
@@ -24,9 +24,12 @@
 from provisioningserver.import_images import boot_resources
 from provisioningserver.utils.env import environment_variables
 from provisioningserver.utils.twisted import synchronous
-from twisted.internet.defer import inlineCallbacks
+from twisted.internet.defer import DeferredLock
 from twisted.internet.threads import deferToThread
 
+# Lock is used so more than one import is not running at the same time.
+import_lock = DeferredLock()
+
 
 def list_boot_images():
     """List the boot images that exist on the cluster."""
@@ -47,7 +50,7 @@
         boot_resources.import_images(sources)
 
 
-@inlineCallbacks
 def import_boot_images(sources):
     """Imports the boot images from the given sources."""
-    yield deferToThread(_run_import, sources)
+    if not import_lock.locked:
+        return import_lock.run(deferToThread, _run_import, sources)
 
=== modified file 'src/provisioningserver/rpc/tests/test_boot_images.py'
--- src/provisioningserver/rpc/tests/test_boot_images.py	2014-09-08 20:25:42 +0000
+++ src/provisioningserver/rpc/tests/test_boot_images.py	2014-09-11 13:00:18 +0000
@@ -17,7 +17,10 @@
 import os
 
 from maastesting.factory import factory
-from maastesting.matchers import MockCalledOnceWith
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+    )
 from maastesting.testcase import MAASTwistedRunTest
 from mock import sentinel
 from provisioningserver.boot import tftppath
@@ -27,11 +30,14 @@
 from provisioningserver.rpc.boot_images import (
     _run_import,
     import_boot_images,
+    import_lock,
     list_boot_images,
     )
 from provisioningserver.testing.config import BootSourcesFixture
 from provisioningserver.testing.testcase import PservTestCase
+from provisioningserver.utils.twisted import pause
 from twisted.internet import defer
+from twisted.internet.task import Clock
 
 
 class TestListBootImages(PservTestCase):
@@ -113,6 +119,16 @@
     run_tests_with = MAASTwistedRunTest.make_factory(timeout=5)
 
     @defer.inlineCallbacks
+    def test__does_not_run_if_lock_taken(self):
+        yield import_lock.acquire()
+        self.addCleanup(import_lock.release)
+        deferToThread = self.patch(boot_images, 'deferToThread')
+        deferToThread.return_value = defer.succeed(None)
+        yield import_boot_images(sentinel.sources)
+        self.assertThat(
+            deferToThread, MockNotCalled())
+
+    @defer.inlineCallbacks
     def test__calls__run_import_using_deferToThread(self):
         deferToThread = self.patch(boot_images, 'deferToThread')
         deferToThread.return_value = defer.succeed(None)
@@ -120,3 +136,16 @@
         self.assertThat(
             deferToThread, MockCalledOnceWith(
                 _run_import, sentinel.sources))
+
+    def test__takes_lock_when_running(self):
+        clock = Clock()
+        deferToThread = self.patch(boot_images, 'deferToThread')
+        deferToThread.return_value = pause(1, clock)
+
+        # Lock is acquired when import is started.
+        import_boot_images(sentinel.sources)
+        self.assertTrue(import_lock.locked)
+
+        # Lock is released once the download is done.
+        clock.advance(1)
+        self.assertFalse(import_lock.locked)