Merge ~ltrager/maas:lp1787621 into maas:master

[Lee Trager]

LP: #1787621 - Retrieve undefined release title directly from OS registry.

MAAS tries to retrieve the resource title from the database. If it has not
been defined in the configured stream or by user during custom image upload
MAAS asks the OS driver to return a release. Each OS driver does this through
string manipulation and does not need on disk access to the OS images. As
such it is safe to call the OS driver directly from the region and not use an
RPC call to all rack controllers.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1787621 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2f8bf60d361b15e0c442b65fa2f7418a1218cded

review: Approve

[Blake Rouse (blake-rouse)]

Looks good, except for the question I have inline.

review: Needs Information

[Lee Trager (ltrager)]

Thanks for the review, answered your question inline.

[Andres Rodriguez (andreserl)]

uhmmm I don't like hardocing this... Why is it only for Ubuntu Core in the
first place? Why not for other OS?

On Tue, Sep 11, 2018 at 11:54 AM, Lee Trager <email address hidden>
wrote:

> Thanks for the review, answered your question inline.
>
> Diff comments:
>
> > diff --git a/src/maasserver/websockets/handlers/bootresource.py
> b/src/maasserver/websockets/handlers/bootresource.py
> > index 4183f22..78315b2 100644
> > --- a/src/maasserver/websockets/handlers/bootresource.py
> > +++ b/src/maasserver/websockets/handlers/bootresource.py
> > @@ -286,7 +286,8 @@ class BootResourceHandler(Handler):
> > if 'title' in image.extra and image.extra != '':
> > title = image.extra['title']
> > else:
> > - title = get_os_release_title(image.os, image.release)
> > + osystem = OperatingSystemRegistry['ubuntu-core']
>
> This is a method only for Ubuntu Core(format_ubuntu_core-images) It
> iterates over BootSourceCache.objects.filter(os='ubuntu-core') so we know
> it will always be ubuntu-core.
>
> > + title = osystem.get_release_title(image.release)
> > if title is None:
> > title = '%s/%s' % (image.os, image.release)
> > images.append({
>
>
> --
> https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/354673
> You are reviewing the proposed merge of ~ltrager/maas:lp1787621 into
> maas:master.
>

--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[Lee Trager (ltrager)]

Ubuntu Core is hard coded as its used in a method specifically for Ubuntu Core. The images that method processes comes from BootSourceCache.objects.filter(os='ubuntu-core').

As discussed in stand up I've removed the unused RPC code on the region.

[Blake Rouse (blake-rouse)]

Thanks for the explanation, and for the removal of the unused code.

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1787621 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3898/console
COMMIT: d6f0c9c02fae4706352d5e20482d1cc7c8a33909

review: Needs Fixing

[MAAS Lander (maas-lander)]

LANDING
-b lp1787621 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3900/consoleText

[Andres Rodriguez (andreserl)]

Can you please fix this for landing?

+ cd /run/build/maas
+ sudo -u ubuntu -E -H make lint
src/maasserver/clusterrpc/osystems.py:24:1: F401 'provisioningserver.rpc.cluster.GetOSReleaseTitle' imported but unused
Makefile:356: recipe for target 'lint-py' failed
make: *** [lint-py] Error 123
[Pipeline] }

475fd4d... by Lee Trager on 2018-09-12

Fix lint

diff --git a/src/maasserver/clusterrpc/osystems.py b/src/maasserver/clusterrpc/osystems.py
index 9ae6af6..e126af4 100644
--- a/src/maasserver/clusterrpc/osystems.py
+++ b/src/maasserver/clusterrpc/osystems.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Obtain OS information from clusters."""
@@ -22,7 +22,6 @@ from maasserver.rpc import (
 from maasserver.utils import async
 from maasserver.utils.orm import get_one
 from provisioningserver.rpc.cluster import (
-    GetOSReleaseTitle,
     GetPreseedData,
     ListOperatingSystems,
     ValidateLicenseKey,
@@ -82,21 +81,6 @@ def gen_all_known_operating_systems():
 
 
 @synchronous
-def get_os_release_title(osystem, release):
-    """Get the title for the operating systems release."""
-    title = ""
-    responses = async.gather(
-        partial(client, GetOSReleaseTitle, osystem=osystem, release=release)
-        for client in getAllClients())
-    for response in suppress_failures(responses):
-        if response["title"] != "":
-            title = response["title"]
-    if title == "":
-        return None
-    return title
-
-
-@synchronous
 def get_preseed_data(preseed_type, node, token, metadata_url):
     """Obtain optional preseed data for this OS, preseed type, and node.
 
diff --git a/src/maasserver/clusterrpc/tests/test_osystems.py b/src/maasserver/clusterrpc/tests/test_osystems.py
index e07d6ad..c2ad218 100644
--- a/src/maasserver/clusterrpc/tests/test_osystems.py
+++ b/src/maasserver/clusterrpc/tests/test_osystems.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the `osystems` module."""
@@ -12,7 +12,6 @@ from collections import (
 
 from maasserver.clusterrpc.osystems import (
     gen_all_known_operating_systems,
-    get_os_release_title,
     get_preseed_data,
     validate_license_key,
 )
@@ -155,46 +154,6 @@ class TestGenAllKnownOperatingSystems(MAASServerTestCase):
             gen_all_known_operating_systems())
 
 
-class TestGetOSReleaseTitle(MAASServerTestCase):
-    """Tests for `get_os_release_title`."""
-
-    def test_ignores_failures_when_talking_to_clusters(self):
-        factory.make_RackController()
-        factory.make_RackController()
-        factory.make_RackController()
-        self.useFixture(RunningClusterRPCFixture())
-
-        title = factory.make_name('title')
-        clients = getAllClients()
-        for index, client in enumerate(clients):
-            callRemote = self.patch(client._conn, "callRemote")
-            if index == 0:
-                # The first client found returns dummy title.
-                example = {"title": title}
-                callRemote.return_value = succeed(example)
-            else:
-                # All clients but the first raise an exception.
-                callRemote.side_effect = ZeroDivisionError()
-
-        # The only title to get through is that from the first. The
-        # failures arising from communicating with the other clusters have all
-        # been suppressed.
-        self.assertEqual(
-            title,
-            get_os_release_title("bogus-os", "bogus-release"))
-
-    def test_returns_None_if_title_is_blank(self):
-        factory.make_RackController()
-        self.useFixture(RunningClusterRPCFixture())
-
-        clients = getAllClients()
-        for index, client in enumerate(clients):
-            callRemote = self.patch(client._conn, "callRemote")
-            example = {"title": ""}
-            callRemote.return_value = succeed(example)
-        self.assertIsNone(get_os_release_title("bogus-os", "bogus-release"))
-
-
 class TestGetPreseedData(MAASServerTestCase):
     """Tests for `get_preseed_data`."""
 
diff --git a/src/maasserver/websockets/handlers/bootresource.py b/src/maasserver/websockets/handlers/bootresource.py
index 4183f22..78315b2 100644
--- a/src/maasserver/websockets/handlers/bootresource.py
+++ b/src/maasserver/websockets/handlers/bootresource.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The BootResource handler for the WebSocket connection."""
@@ -26,7 +26,6 @@ from maasserver.clusterrpc.boot_images import (
     get_common_available_boot_images,
     is_import_boot_images_running,
 )
-from maasserver.clusterrpc.osystems import get_os_release_title
 from maasserver.enum import (
     BOOT_RESOURCE_TYPE,
     NODE_STATUS,
@@ -53,6 +52,7 @@ from provisioningserver.config import (
     DEFAULT_IMAGES_URL,
     DEFAULT_KEYRINGS_PATH,
 )
+from provisioningserver.drivers.osystem import OperatingSystemRegistry
 from provisioningserver.import_images.download_descriptions import (
     download_all_image_descriptions,
     image_passes_filter,
@@ -286,7 +286,8 @@ class BootResourceHandler(Handler):
             if 'title' in image.extra and image.extra != '':
                 title = image.extra['title']
             else:
-                title = get_os_release_title(image.os, image.release)
+                osystem = OperatingSystemRegistry['ubuntu-core']
+                title = osystem.get_release_title(image.release)
             if title is None:
                 title = '%s/%s' % (image.os, image.release)
             images.append({
@@ -306,11 +307,13 @@ class BootResourceHandler(Handler):
             Q(os='ubuntu') | Q(os='ubuntu-core')).filter(bootloader_type=None)
         for image in qs:
             resource = self.get_matching_resource_for_image(resources, image)
+            title = None
             if 'title' in image.extra and image.extra != '':
                 title = image.extra['title']
-            else:
-                title = get_os_release_title(image.os, image.release)
-            if title is None:
+            elif image.os in OperatingSystemRegistry:
+                osystem = OperatingSystemRegistry[image.os]
+                title = osystem.get_release_title(image.release)
+            if not title:
                 title = '%s/%s' % (image.os, image.release)
             images.append({
                 'name': '%s/%s/%s/%s' % (
@@ -432,8 +435,11 @@ class BootResourceHandler(Handler):
             if resource.name.startswith('ubuntu/'):
                 return format_ubuntu_distro_series(series)
             else:
-                title = get_os_release_title(os, series)
-                if title is None:
+                title = None
+                if os in OperatingSystemRegistry:
+                    osystem = OperatingSystemRegistry[os]
+                    title = osystem.get_release_title(series)
+                if not title:
                     return resource.name
                 else:
                     return title
diff --git a/src/maasserver/websockets/handlers/tests/test_bootresource.py b/src/maasserver/websockets/handlers/tests/test_bootresource.py
index fa71918..ebaefe8 100644
--- a/src/maasserver/websockets/handlers/tests/test_bootresource.py
+++ b/src/maasserver/websockets/handlers/tests/test_bootresource.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maasserver.websockets.handlers.bootresource`"""
@@ -629,7 +629,7 @@ class TestBootResourcePoll(MAASServerTestCase, PatchOSInfoMixin):
         self.assertEquals([{
             'name': '%s/%s/%s/%s' % (
                 cache.os, cache.arch, cache.subarch, cache.release),
-            'title': '%s/%s' % (cache.os, cache.release),
+            'title': cache.release,
             'checked': False,
             'deleted': False,
         }], ubuntu_core_images)
@@ -647,7 +647,7 @@ class TestBootResourcePoll(MAASServerTestCase, PatchOSInfoMixin):
         self.assertEquals([{
             'name': '%s/%s/%s/%s' % (
                 cache.os, cache.arch, cache.subarch, cache.release),
-            'title': '%s/%s' % (cache.os, cache.release),
+            'title': cache.release,
             'checked': True,
             'deleted': False,
         }], ubuntu_core_images)