Merge ~ltrager/maas:lp1787621 into maas:master

Proposed by Lee Trager
Status: Merged
Approved by: Lee Trager
Approved revision: 475fd4d11564218a4c599d46f6f1ff7f24f6d3d0
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ltrager/maas:lp1787621
Merge into: maas:master
Diff against target: 200 lines (+19/-70)
4 files modified
src/maasserver/clusterrpc/osystems.py (+1/-17)
src/maasserver/clusterrpc/tests/test_osystems.py (+1/-42)
src/maasserver/websockets/handlers/bootresource.py (+14/-8)
src/maasserver/websockets/handlers/tests/test_bootresource.py (+3/-3)
Reviewer Review Type Date Requested Status
MAAS Lander Needs Fixing
Blake Rouse (community) Approve
Review via email: mp+354673@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

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

STATUS: SUCCESS
COMMIT: 2f8bf60d361b15e0c442b65fa2f7418a1218cded

review: Approve
Revision history for this message
Blake Rouse (blake-rouse) wrote :

Looks good, except for the question I have inline.

review: Needs Information
Revision history for this message
Lee Trager (ltrager) wrote :

Thanks for the review, answered your question inline.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

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.

Revision history for this message
Lee Trager (ltrager) wrote :

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.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

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

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :

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
Revision history for this message
MAAS Lander (maas-lander) wrote :
Revision history for this message
Andres Rodriguez (andreserl) wrote :

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] }

~ltrager/maas:lp1787621 updated
475fd4d... by Lee Trager

Fix lint

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 @@
1# Copyright 2014-2016 Canonical Ltd. This software is licensed under the1# Copyright 2014-2018 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Obtain OS information from clusters."""4"""Obtain OS information from clusters."""
@@ -22,7 +22,6 @@ from maasserver.rpc import (
22from maasserver.utils import async22from maasserver.utils import async
23from maasserver.utils.orm import get_one23from maasserver.utils.orm import get_one
24from provisioningserver.rpc.cluster import (24from provisioningserver.rpc.cluster import (
25 GetOSReleaseTitle,
26 GetPreseedData,25 GetPreseedData,
27 ListOperatingSystems,26 ListOperatingSystems,
28 ValidateLicenseKey,27 ValidateLicenseKey,
@@ -82,21 +81,6 @@ def gen_all_known_operating_systems():
8281
8382
84@synchronous83@synchronous
85def get_os_release_title(osystem, release):
86 """Get the title for the operating systems release."""
87 title = ""
88 responses = async.gather(
89 partial(client, GetOSReleaseTitle, osystem=osystem, release=release)
90 for client in getAllClients())
91 for response in suppress_failures(responses):
92 if response["title"] != "":
93 title = response["title"]
94 if title == "":
95 return None
96 return title
97
98
99@synchronous
100def get_preseed_data(preseed_type, node, token, metadata_url):84def get_preseed_data(preseed_type, node, token, metadata_url):
101 """Obtain optional preseed data for this OS, preseed type, and node.85 """Obtain optional preseed data for this OS, preseed type, and node.
10286
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 @@
1# Copyright 2014-2016 Canonical Ltd. This software is licensed under the1# Copyright 2014-2018 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Tests for the `osystems` module."""4"""Tests for the `osystems` module."""
@@ -12,7 +12,6 @@ from collections import (
1212
13from maasserver.clusterrpc.osystems import (13from maasserver.clusterrpc.osystems import (
14 gen_all_known_operating_systems,14 gen_all_known_operating_systems,
15 get_os_release_title,
16 get_preseed_data,15 get_preseed_data,
17 validate_license_key,16 validate_license_key,
18)17)
@@ -155,46 +154,6 @@ class TestGenAllKnownOperatingSystems(MAASServerTestCase):
155 gen_all_known_operating_systems())154 gen_all_known_operating_systems())
156155
157156
158class TestGetOSReleaseTitle(MAASServerTestCase):
159 """Tests for `get_os_release_title`."""
160
161 def test_ignores_failures_when_talking_to_clusters(self):
162 factory.make_RackController()
163 factory.make_RackController()
164 factory.make_RackController()
165 self.useFixture(RunningClusterRPCFixture())
166
167 title = factory.make_name('title')
168 clients = getAllClients()
169 for index, client in enumerate(clients):
170 callRemote = self.patch(client._conn, "callRemote")
171 if index == 0:
172 # The first client found returns dummy title.
173 example = {"title": title}
174 callRemote.return_value = succeed(example)
175 else:
176 # All clients but the first raise an exception.
177 callRemote.side_effect = ZeroDivisionError()
178
179 # The only title to get through is that from the first. The
180 # failures arising from communicating with the other clusters have all
181 # been suppressed.
182 self.assertEqual(
183 title,
184 get_os_release_title("bogus-os", "bogus-release"))
185
186 def test_returns_None_if_title_is_blank(self):
187 factory.make_RackController()
188 self.useFixture(RunningClusterRPCFixture())
189
190 clients = getAllClients()
191 for index, client in enumerate(clients):
192 callRemote = self.patch(client._conn, "callRemote")
193 example = {"title": ""}
194 callRemote.return_value = succeed(example)
195 self.assertIsNone(get_os_release_title("bogus-os", "bogus-release"))
196
197
198class TestGetPreseedData(MAASServerTestCase):157class TestGetPreseedData(MAASServerTestCase):
199 """Tests for `get_preseed_data`."""158 """Tests for `get_preseed_data`."""
200159
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 @@
1# Copyright 2016-2017 Canonical Ltd. This software is licensed under the1# Copyright 2016-2018 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""The BootResource handler for the WebSocket connection."""4"""The BootResource handler for the WebSocket connection."""
@@ -26,7 +26,6 @@ from maasserver.clusterrpc.boot_images import (
26 get_common_available_boot_images,26 get_common_available_boot_images,
27 is_import_boot_images_running,27 is_import_boot_images_running,
28)28)
29from maasserver.clusterrpc.osystems import get_os_release_title
30from maasserver.enum import (29from maasserver.enum import (
31 BOOT_RESOURCE_TYPE,30 BOOT_RESOURCE_TYPE,
32 NODE_STATUS,31 NODE_STATUS,
@@ -53,6 +52,7 @@ from provisioningserver.config import (
53 DEFAULT_IMAGES_URL,52 DEFAULT_IMAGES_URL,
54 DEFAULT_KEYRINGS_PATH,53 DEFAULT_KEYRINGS_PATH,
55)54)
55from provisioningserver.drivers.osystem import OperatingSystemRegistry
56from provisioningserver.import_images.download_descriptions import (56from provisioningserver.import_images.download_descriptions import (
57 download_all_image_descriptions,57 download_all_image_descriptions,
58 image_passes_filter,58 image_passes_filter,
@@ -286,7 +286,8 @@ class BootResourceHandler(Handler):
286 if 'title' in image.extra and image.extra != '':286 if 'title' in image.extra and image.extra != '':
287 title = image.extra['title']287 title = image.extra['title']
288 else:288 else:
289 title = get_os_release_title(image.os, image.release)289 osystem = OperatingSystemRegistry['ubuntu-core']
290 title = osystem.get_release_title(image.release)
290 if title is None:291 if title is None:
291 title = '%s/%s' % (image.os, image.release)292 title = '%s/%s' % (image.os, image.release)
292 images.append({293 images.append({
@@ -306,11 +307,13 @@ class BootResourceHandler(Handler):
306 Q(os='ubuntu') | Q(os='ubuntu-core')).filter(bootloader_type=None)307 Q(os='ubuntu') | Q(os='ubuntu-core')).filter(bootloader_type=None)
307 for image in qs:308 for image in qs:
308 resource = self.get_matching_resource_for_image(resources, image)309 resource = self.get_matching_resource_for_image(resources, image)
310 title = None
309 if 'title' in image.extra and image.extra != '':311 if 'title' in image.extra and image.extra != '':
310 title = image.extra['title']312 title = image.extra['title']
311 else:313 elif image.os in OperatingSystemRegistry:
312 title = get_os_release_title(image.os, image.release)314 osystem = OperatingSystemRegistry[image.os]
313 if title is None:315 title = osystem.get_release_title(image.release)
316 if not title:
314 title = '%s/%s' % (image.os, image.release)317 title = '%s/%s' % (image.os, image.release)
315 images.append({318 images.append({
316 'name': '%s/%s/%s/%s' % (319 'name': '%s/%s/%s/%s' % (
@@ -432,8 +435,11 @@ class BootResourceHandler(Handler):
432 if resource.name.startswith('ubuntu/'):435 if resource.name.startswith('ubuntu/'):
433 return format_ubuntu_distro_series(series)436 return format_ubuntu_distro_series(series)
434 else:437 else:
435 title = get_os_release_title(os, series)438 title = None
436 if title is None:439 if os in OperatingSystemRegistry:
440 osystem = OperatingSystemRegistry[os]
441 title = osystem.get_release_title(series)
442 if not title:
437 return resource.name443 return resource.name
438 else:444 else:
439 return title445 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 @@
1# Copyright 2016-2017 Canonical Ltd. This software is licensed under the1# Copyright 2016-2018 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Tests for `maasserver.websockets.handlers.bootresource`"""4"""Tests for `maasserver.websockets.handlers.bootresource`"""
@@ -629,7 +629,7 @@ class TestBootResourcePoll(MAASServerTestCase, PatchOSInfoMixin):
629 self.assertEquals([{629 self.assertEquals([{
630 'name': '%s/%s/%s/%s' % (630 'name': '%s/%s/%s/%s' % (
631 cache.os, cache.arch, cache.subarch, cache.release),631 cache.os, cache.arch, cache.subarch, cache.release),
632 'title': '%s/%s' % (cache.os, cache.release),632 'title': cache.release,
633 'checked': False,633 'checked': False,
634 'deleted': False,634 'deleted': False,
635 }], ubuntu_core_images)635 }], ubuntu_core_images)
@@ -647,7 +647,7 @@ class TestBootResourcePoll(MAASServerTestCase, PatchOSInfoMixin):
647 self.assertEquals([{647 self.assertEquals([{
648 'name': '%s/%s/%s/%s' % (648 'name': '%s/%s/%s/%s' % (
649 cache.os, cache.arch, cache.subarch, cache.release),649 cache.os, cache.arch, cache.subarch, cache.release),
650 'title': '%s/%s' % (cache.os, cache.release),650 'title': cache.release,
651 'checked': True,651 'checked': True,
652 'deleted': False,652 'deleted': False,
653 }], ubuntu_core_images)653 }], ubuntu_core_images)

Subscribers

People subscribed via source and target branches