Merge lp:~rvb/maas-test/import-images-api into lp:maas-test

[Raphaël Badin]

No node can be booted until the boot images have been reported back to the region: wait until the report_boot_images task has populated the BootImage table.

I've tested this change, with success, in the lab: http://d-jenkins.ubuntu-ci:8080/view/MAAS/job/maas-test-manual/55/console.

[Jeroen T. Vermeulen (jtv)]

Thanks for fixing this — and let's not forget, diagnosing it!

Notes as usual:

Maybe in the docstring for wait_until_boot_images_scanned, say “until the master cluster has reported boot images” (so as not to give the impression that we just wait for them to appear in /var/lib/maas)?

.

Any particular reason why wait_until_boot_images_scanned returns the number of images? It seems a bit arbitrary (why not the actual list, for instance?) and introduces testing for something that's not actually used. Why not just return nothing, and document the fact that either a timeout or a failure to request the listing will raise an exception?

By the way, the initial assignment for nb_images looks dead to me.

.

Also in wait_until_boot_images_scanned, it turns out that RuntimeError isn't really for this kind of error, but rather for errors originating in the Python runtime. And, it's deprecated anyway. Better derive an ad-hoc exception class from Exception.

The exception's error message says “cluster” instead of “the cluster,” which makes it look a little weird to me. But maybe it would be more helpful to say e.g. “Boot image download timed out: cluster reported no images”?

.

In test_maasfixture.py, I wonder if it would make sense to make the patch for wait_until_boot_images_scanned a standard part of make_maas_fixture, with an option to skip it?

.

What's going on with the separate make_kvm_fixture steps? AFAIK you don't need to do that — just call make_maas_fixture and it will create a KVMFixture for you. In test_list_boot_images_lists_images there's even a commented-out line that did it that way. Is there a difference? Or was this a transient change during development that is no longer needed?

.

Setup gets a little lengthy in test_list_boot_images_lists_images. Might it make sense to extract this bit into a helper?

        uuid = self.getUniqueString()
        self.patch(
            maas_fixture, 'get_master_ng_uuid',
            mock.MagicMock(return_value=uuid))

review: Approve

[lp:~rvb/maas-test/import-images-api]

152. By Raphaël Badin on 2014-04-09

Review fixes.

[Raphaël Badin (rvb)]

> Thanks for fixing this — and let's not forget, diagnosing it!
>
> Notes as usual:
>
> Maybe in the docstring for wait_until_boot_images_scanned, say “until the
> master cluster has reported boot images” (so as not to give the impression
> that we just wait for them to appear in /var/lib/maas)?

Right, done.

> Any particular reason why wait_until_boot_images_scanned returns the number of
> images? It seems a bit arbitrary (why not the actual list, for instance?) and
> introduces testing for something that's not actually used. Why not just
> return nothing, and document the fact that either a timeout or a failure to
> request the listing will raise an exception?

Okay, done.

> Also in wait_until_boot_images_scanned, it turns out that RuntimeError isn't
> really for this kind of error, but rather for errors originating in the Python
> runtime. And, it's deprecated anyway. Better derive an ad-hoc exception
> class from Exception.
>
> The exception's error message says “cluster” instead of “the cluster,” which
> makes it look a little weird to me. But maybe it would be more helpful to say
> e.g. “Boot image download timed out: cluster reported no images”?

Done.

> In test_maasfixture.py, I wonder if it would make sense to make the patch for
> wait_until_boot_images_scanned a standard part of make_maas_fixture, with an
> option to skip it?

I'd rather not do that, I know all the setup steps look a bit redundant but I'd rather keep the setup explicit than mess things up with helpers that I fear will soon become spaghetti code.

> What's going on with the separate make_kvm_fixture steps? AFAIK you don't
> need to do that — just call make_maas_fixture and it will create a KVMFixture
> for you. In test_list_boot_images_lists_images there's even a commented-out
> line that did it that way. Is there a difference? Or was this a transient
> change during development that is no longer needed?

True, fixed.

> Setup gets a little lengthy in test_list_boot_images_lists_images. Might it
> make sense to extract this bit into a helper?
>
> uuid = self.getUniqueString()
> self.patch(
> maas_fixture, 'get_master_ng_uuid',
> mock.MagicMock(return_value=uuid))

Again, I don't think it's worth it.

Thanks for the review!

=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2014-04-08 10:57:13 +0000
+++ maastest/maasfixture.py	2014-04-09 11:15:47 +0000
@@ -17,6 +17,7 @@
     ]
 
 
+from datetime import timedelta
 import httplib
 import json
 import logging
@@ -75,6 +76,10 @@
 ]
 
 
+class NoBootImagesError(Exception):
+    """No boot images present on the master cluster."""
+
+
 class MAASFixture(Fixture):
     """A fixture for a MAAS server."""
 
@@ -241,6 +246,28 @@
             check_call=True)
         logging.info("Done importing boot images.")
 
+    def wait_until_boot_images_scanned(self):
+        """Wait until the master cluster has reported boot images."""
+        # Poll until the boot images have been reported.
+        timeout = timedelta(minutes=10).total_seconds()
+        for _ in utils.retries(timeout=timeout, delay=10):
+            boot_images = self.list_boot_images()
+            if len(boot_images) > 0:
+                return
+
+        raise NoBootImagesError(
+            "Boot image download timed out: cluster reported no images.")
+
+    def list_boot_images(self):
+        """Return the boot images of the master cluster."""
+        ng_uuid = self.get_master_ng_uuid()
+        uri = utils.get_uri('nodegroups/%s/boot-images/' % ng_uuid)
+        response = self.admin_maas_client.get(uri)
+        if response.code != httplib.OK:
+            raise Exception(
+                "Error getting boot images for cluster '%s'" % ng_uuid)
+        return json.loads(response.read())
+
     def configure(self, name, value):
         """Set a config value in MAAS."""
         uri = utils.get_uri('maas/')
@@ -293,6 +320,7 @@
         self.import_maas_images(
             series=self.series, architecture=self.architecture,
             simplestreams_filter=self.simplestreams_filter)
+        self.wait_until_boot_images_scanned()
         self.configure_default_series(self.series)
         if self.kvm_fixture.direct_ip is not None:
             self.check_cluster_connected()

=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2014-04-09 06:57:50 +0000
+++ maastest/tests/test_maasfixture.py	2014-04-09 11:15:47 +0000
@@ -31,6 +31,7 @@
     utils,
     )
 from maastest.maas_enums import NODEGROUPINTERFACE_MANAGEMENT
+from maastest.maasfixture import NoBootImagesError
 from maastest.testing.factory import make_file
 import mock
 import netaddr
@@ -416,6 +417,9 @@
         self.patch(
             maas_fixture, 'query_api_key',
             mock.MagicMock(return_value='fake:api:key'))
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
+
         with maas_fixture:
             pass
         mock_logging = mock.MagicMock()
@@ -570,6 +574,8 @@
         key = 'fake:api:key'
         self.patch(
             maas_fixture, 'query_api_key', mock.MagicMock(return_value=key))
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
 
         with maas_fixture:
             pass
@@ -595,7 +601,8 @@
             mock.MagicMock(return_value='fake:api:key'))
         self.patch(
             maas_fixture, 'collect_logs', mock.MagicMock())
-
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
         with maas_fixture:
             pass
 
@@ -663,6 +670,8 @@
             maas_fixture, 'import_maas_images', mock.MagicMock())
         self.patch(
             maas_fixture, 'log_connection_details', mock.MagicMock())
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
 
         with maas_fixture:
             pass
@@ -696,6 +705,8 @@
             mock.MagicMock(return_value='fake:api:key'))
         self.patch(
             maas_fixture, 'configure_http_proxy', mock.MagicMock())
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
 
         with maas_fixture:
             pass
@@ -719,6 +730,8 @@
             maas_fixture, 'check_cluster_connected', mock.MagicMock())
         self.patch(
             maas_fixture, 'configure_cluster', mock.MagicMock())
+        self.patch(
+            maas_fixture, 'wait_until_boot_images_scanned', mock.MagicMock())
 
         with maas_fixture:
             pass
@@ -767,3 +780,44 @@
                 'sudo', 'maas-region-admin', 'dumpdata',
                 'metadataserver.NodeCommissionResult']),
             maas_fixture.kvm_fixture.run_command.mock_calls)
+
+    def test_wait_until_boot_images_scanned_times_out_eventually(self):
+        maas_fixture = self.make_maas_fixture()
+        self.patch(
+            maas_fixture, 'list_boot_images',
+            mock.MagicMock(return_value=[]))
+        self.patch(
+            utils, 'retries', mock.MagicMock(return_value=[(0, 1), (0, 1)]))
+
+        exception = self.assertRaises(
+            NoBootImagesError, maas_fixture.wait_until_boot_images_scanned)
+        message = text_type(exception)
+        self.assertIn(
+            "Boot image download timed out: cluster reported no images.",
+            message)
+
+    def test_wait_until_boot_images_scanned_returns_with_error(self):
+        maas_fixture = self.make_maas_fixture()
+        self.patch(
+            maas_fixture, 'list_boot_images',
+            mock.MagicMock(return_value=['non-empty1', 'non-empty2']))
+
+        self.assertEqual(None, maas_fixture.wait_until_boot_images_scanned())
+
+    def test_list_boot_images_lists_images(self):
+        maas_fixture = self.make_maas_fixture()
+        response = [{'fake-response': self.getUniqueString()}]
+        self.patch(maas_fixture, 'admin_maas_client', mock.MagicMock())
+        call_mock = self.patch_MAASClient(
+            maas_fixture.admin_maas_client, 'get', response)
+        uuid = self.getUniqueString()
+        self.patch(
+            maas_fixture, 'get_master_ng_uuid',
+            mock.MagicMock(return_value=uuid))
+
+        self.assertEqual(
+            (
+                response,
+                [mock.call('/api/1.0/nodegroups/%s/boot-images/' % uuid)],
+            ),
+            (maas_fixture.list_boot_images(), call_mock.mock_calls))