Merge lp:~jibel/ubuntu-cdimage/support_for_multilayer into lp:ubuntu-cdimage

Proposed by Jean-Baptiste Lallement
Status: Merged
Merged at revision: 1792
Proposed branch: lp:~jibel/ubuntu-cdimage/support_for_multilayer
Merge into: lp:ubuntu-cdimage
Prerequisite: lp:~vorlon/ubuntu-cdimage/prune-obsolete-code
Diff against target: 358 lines (+130/-81) (has conflicts)
5 files modified
etc/crontab (+1/-1)
etc/livefs-launchpad (+1/-0)
etc/qa-products (+1/-0)
lib/cdimage/livefs.py (+43/-46)
lib/cdimage/tests/test_livefs.py (+84/-34)
Text conflict in etc/purge-days
To merge this branch: bzr merge lp:~jibel/ubuntu-cdimage/support_for_multilayer
Reviewer Review Type Date Requested Status
Steve Langasek Approve
Review via email: mp+364105@code.launchpad.net

This proposal supersedes a proposal from 2018-11-26.

Commit message

Adds support for multilayer images

* Made artefacts download generic: Don't hardcode artefact names and ensure they can be matched via regexp, on launchpad or local directory build.
* Fixed test suite
* Removed obsolete edubuntu server-squashfs test (LP: #1154601)
* Ensure we are still compatible with tests using a local server and with a local build.
* Add new ubuntu:canary subproject in automated builds

Extract from the discussion with cjwatson about these changes:
"""""
didrocks hey cjwatson! jibel and I have a question on ubuntu-cdimage: as with the new layered images we are building, we might not know in advance the squashfs, size, manifest files. We want to avoid to hardcode more filenames that already exists. In livefs.py, there are basically 2 code paths to download assets. One is using LP API (and so, can list all artefacts attached to a specific build) and the other
didrocks one is just for filesystem + http:// download. We thought to match against regexp, which works in the LP case and for filesystem (via os.listdir()). However, tests are failing as they are using http mock server. I wonder if in production, only the LP path is took. If so, can we just detect "http" for tests and either create a manifest that we download or rely on hardcoded filenames?
cjwatson As far as I know it's only ever the LP path in production, indeed
didrocks so, do you think special casing tests, like not using the regexp for them, is fine?
cjwatson Yeah, I think so
didrocks perfect! Thanks a lot cjwatson :)
cjwatson The http:// case may even be legacy - not sure
didrocks yeah, sounds like it, but we preferred asking you directly :)
cjwatson Obviously for good software engineering reasons keeping special cases for tests to a minimum is a good idea, but creating a sort of meta-manifest doesn't seem unreasonable, maybe with a fallback if it doesn't exist
didrocks yeah, the best would be ofc to rewrite the tests to mock LP
didrocks but I think that's out of our current scope :)
"""""

Description of the change

This is the support for multi layer images based on dead code removal branch from from Steve.

To post a comment you must log in.
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote : Posted in a previous version of this proposal

Rebased on trunk, could you please review? Thanks

Revision history for this message
Steve Langasek (vorlon) : Posted in a previous version of this proposal
review: Needs Fixing
Revision history for this message
Didier Roche (didrocks) wrote : Posted in a previous version of this proposal

Thanks for the review!

We just rebased on your cleanup branch to avoid adapating tests and code that are going to be removed soon. All your comments should hopefully now be addressed :)

Revision history for this message
Didier Roche (didrocks) wrote :

FYI, some answers to your comments were in https://code.launchpad.net/~jibel/ubuntu-cdimage/support_for_multilayer/+merge/359512 (+ selecting the correct diff version, can't link directly apparently).

Revision history for this message
Steve Langasek (vorlon) wrote :

Thanks, getting close now, mostly I just want you to delete some more code ;)

Also since you have (IMHO reasonably) based this on my other branch which has not yet landed, landing this will now necessarily block on review of that other branch, which I've asked sil2100 to look at.

review: Needs Fixing
Revision history for this message
Steve Langasek (vorlon) :
1804. By Jean-Baptiste Lallement

* Removed dead code
* Fixed PEP8 warning
* Updated tests to match the code removal and use the LP code path
* Added negative test to download from remote URL

Revision history for this message
Didier Roche (didrocks) :
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

Thanks for the review.

We tried to explain our choices to show that some decisions were not arbitrary. No issue on pending on the other MP, as stated before, there was no point anyway to modify tests and code that are going to be removed.

So, we mixed a little bit of dead code removal as you requested on this MP, and adapted the tests as the API isn’t called in the way the tests were calling them.

Revision history for this message
Steve Langasek (vorlon) wrote :

I still get the following linting failures from ./run-tests on cosmic, do you also see this?

./lib/cdimage/livefs.py:552:21: E201 whitespace after '['
./lib/cdimage/livefs.py:552:69: E202 whitespace before ']'
./lib/cdimage/livefs.py:586:42: E225 missing whitespace around operator
./lib/cdimage/tests/test_livefs.py:1294: local variable 'url' is assigned to but never used

review: Needs Fixing
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

I didn't notice these linter errors on cosmic because pep8 and pyflakes were not installed in the build env.
test_pep8_clean and test_pyflakes_clean should probably fail (or as least explicitly skip) when the linter is not installed instead of returning.

1805. By Jean-Baptiste Lallement

Fixed linter errors

1806. By Jean-Baptiste Lallement

Factorized download_live_items_* tests and removed duplicated tests
Added test for filtering several artefacts by suffix
Added test for downloading several artefacts

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

We removed duplicated test and added one to cover filtering of artefacts and another to ensure that several artefacts are downloaded.

The remaining point is the question about server-squashfs (cf inline comment)

Revision history for this message
Steve Langasek (vorlon) wrote :

I covered the point in my prior review that there are no actual consumers of the "server-squashfs" support apart from the tests; so both the tests and the implementation should be culled.

1807. By Jean-Baptiste Lallement

Removed all the references to server-squashfs

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

All the references to server-squashfs have been removed from livefs.py and the related tests.

Revision history for this message
Steve Langasek (vorlon) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'etc/crontab'
2--- etc/crontab 2018-10-30 13:23:58 +0000
3+++ etc/crontab 2019-03-12 08:44:31 +0000
4@@ -21,7 +21,7 @@
5 20 15 * * * DIST=bionic for-project ubuntu-budgie cron.daily-live --live
6 26 2 * * * DIST=bionic for-project ubuntu-mate cron.daily-live --live
7 # regular disco daily builds
8-31 7 * * * for-project ubuntu cron.daily-live --live
9+31 7 * * * for-project ubuntu cron.daily-live --live; SUBPROJECT=canary for-project ubuntu cron.daily-live --live
10 14 5 * * * for-project kubuntu cron.daily-live --live
11 50 1 * * * for-project xubuntu cron.daily-live --live
12 29 6 * * * for-project ubuntu-server cron.daily --live; for-project ubuntu-server cron.daily-preinstalled --live; SUBPROJECT=live for-project ubuntu-server cron.daily-live --live
13
14=== modified file 'etc/livefs-launchpad'
15--- etc/livefs-launchpad 2019-03-12 08:44:31 +0000
16+++ etc/livefs-launchpad 2019-03-12 08:44:31 +0000
17@@ -3,6 +3,7 @@
18 # Like etc/default-arches, "PROJECT" is in fact PROJECT[-SUBPROJECT][-LOCALE].
19
20 ubuntu daily-live * * ubuntu-cdimage/ubuntu
21+ubuntu-canary daily-live * * ubuntu-cdimage/ubuntu-canary
22 kubuntu daily-live * * ubuntu-cdimage/kubuntu
23 edubuntu dvd * * ubuntu-cdimage/edubuntu
24 xubuntu daily-live * * ubuntu-cdimage/xubuntu
25
26=== modified file 'etc/qa-products'
27--- etc/qa-products 2019-03-12 08:44:31 +0000
28+++ etc/qa-products 2019-03-12 08:44:31 +0000
29@@ -45,6 +45,7 @@
30
31 # Ubuntu Desktop
32 Ubuntu Desktop amd64 ubuntu daily-live desktop amd64 iso
33+Ubuntu Desktop (Canary) amd64 ubuntu daily-live/canary desktop amd64 iso
34 Ubuntu Desktop amd64+mac ubuntu daily-live desktop amd64+mac iso
35 Ubuntu Desktop armhf+omap4 ubuntu daily-live desktop armhf+omap4 iso
36 Ubuntu Desktop i386 ubuntu daily-live desktop i386 iso
37
38=== modified file 'lib/cdimage/livefs.py'
39--- lib/cdimage/livefs.py 2019-03-12 08:44:31 +0000
40+++ lib/cdimage/livefs.py 2019-03-12 08:44:31 +0000
41@@ -538,22 +538,32 @@
42 liveproject_subarch = liveproject
43
44 lp, lp_livefs = get_lp_livefs(config, arch)
45+ uris = []
46+ root = ""
47 if lp_livefs is not None:
48 lp_kwargs = live_build_lp_kwargs(config, lp, lp_livefs, arch)
49 lp_build = lp_livefs.getLatestBuild(
50 lp_kwargs["distro_arch_series"],
51 unique_key=lp_kwargs.get("unique_key"))
52- lp_urls = list(lp_build.getFileUrls())
53-
54- def urls_for(base):
55- for url in lp_urls:
56- if unquote(os.path.basename(url)) == base:
57- yield url
58+ uris = list(lp_build.getFileUrls())
59 else:
60 root = livecd_base(config, arch)
61+ try:
62+ uris = [os.path.join(root, u) for u in os.listdir(root)]
63+ except OSError:
64+ # fallback to exact given uri (for http://) in url_for as we can't
65+ # list content.
66+ pass
67
68- def urls_for(base):
69- yield "%s/%s" % (root, base)
70+ def urls_for(base, item):
71+ if uris:
72+ for uri in uris:
73+ filename = unquote(os.path.basename(uri))
74+ if (filename.startswith(base + '.') and
75+ filename.endswith('.' + item)):
76+ yield uri
77+ else:
78+ yield os.path.join(root, base + '.' + item)
79
80 if item in (
81 "cloop", "squashfs", "manifest", "manifest-desktop", "manifest-remove",
82@@ -561,20 +571,22 @@
83 "rootfs.tar.gz", "custom.tar.gz", "device.tar.gz",
84 "azure.device.tar.gz", "raspi2.device.tar.gz", "plano.device.tar.gz",
85 "tar.xz", "iso", "os.snap", "kernel.snap", "disk1.img.xz",
86- "dragonboard.kernel.snap", "raspi2.kernel.snap", "installer.squashfs",
87- "maas-rack.squashfs", "maas-region.squashfs",
88+ "dragonboard.kernel.snap", "raspi2.kernel.snap",
89 "img.xz", "model-assertion"
90 ):
91 if item == "ext4" and arch == "armhf+nexus7":
92 for url in urls_for(
93- "livecd.%s.%s-nexus7" % (liveproject_subarch, item)):
94+ "livecd." + liveproject_subarch, item + "-nexus7"):
95 yield url
96 elif item == "disk1.img.xz":
97 for url in urls_for(
98- "livecd.%s.%s" % (liveproject, item)):
99+ "livecd." + liveproject, item):
100 yield url
101 else:
102- for url in urls_for("livecd.%s.%s" % (liveproject_subarch, item)):
103+ for url in urls_for("livecd." + liveproject_subarch, item):
104+ # filter out redundant artefacts
105+ if url.endswith("modules.squashfs"):
106+ continue
107 yield url
108 elif item in (
109 "kernel", "initrd", "bootimg", "modules.squashfs"
110@@ -582,8 +594,8 @@
111 our_flavours = flavours(config, arch)
112 our_flavours.extend(["%s-hwe" % (f,) for f in our_flavours])
113 for flavour in our_flavours:
114- base = "livecd.%s.%s-%s" % (liveproject_subarch, item, flavour)
115- for url in urls_for(base):
116+ for url in urls_for("livecd." + liveproject_subarch,
117+ item + "-" + flavour):
118 yield url
119 elif item in (
120 "boot-%s+%s.img" % (target.ubuntu_arch, target.subarch)
121@@ -596,15 +608,13 @@
122 for target in Touch.list_targets_by_ubuntu_arch(arch)
123 ):
124 for flavour in flavours(config, arch):
125- base = "livecd.%s.%s" % (liveproject_subarch, item)
126- for url in urls_for(base):
127+ for url in urls_for("livecd." + liveproject_subarch, item):
128 yield url
129 elif item == "kernel-efi-signed":
130 if series >= "precise" and arch == "amd64":
131 for flavour in flavours(config, arch):
132- base = "livecd.%s.kernel-%s.efi.signed" % (
133- liveproject_subarch, flavour)
134- for url in urls_for(base):
135+ for url in urls_for("livecd." + liveproject_subarch,
136+ "kernel-" + flavour + ".efi.signed"):
137 yield url
138 elif item == "wubi":
139 if (project != "xubuntu" and arch in ("amd64", "i386")):
140@@ -616,7 +626,7 @@
141 "stable" % series)
142 elif item == "ltsp-squashfs":
143 if arch in ("amd64", "i386"):
144- for url in urls_for("livecd.%s-ltsp.squashfs" % liveproject):
145+ for url in urls_for("livecd." + liveproject + "-ltsp", "squashfs"):
146 yield url
147 else:
148 raise UnknownLiveItem("Unknown live filesystem item '%s'" % item)
149@@ -635,15 +645,7 @@
150 output_dir = live_output_directory(config)
151 found = False
152
153- if item == "server-squashfs":
154- original_project = config.project
155- try:
156- config["PROJECT"] = "ubuntu-server"
157- urls = list(live_item_paths(config, arch, "squashfs"))
158- finally:
159- config["PROJECT"] = original_project
160- else:
161- urls = list(live_item_paths(config, arch, item))
162+ urls = list(live_item_paths(config, arch, item))
163 if not urls:
164 return False
165
166@@ -726,14 +728,17 @@
167 except osextras.FetchError:
168 pass
169 else:
170- target = os.path.join(output_dir, "%s.%s" % (arch, item))
171- try:
172- osextras.fetch(config, urls[0], target)
173- if item in ["squashfs", "server-squashfs"]:
174- sign.sign_cdimage(config, target)
175- found = True
176- except osextras.FetchError:
177- pass
178+ for url in urls:
179+ # strip livecd.<PROJECT> and replace by arch
180+ filename = unquote(os.path.basename(url)).split('.', 2)[-1]
181+ target = os.path.join(output_dir, "%s.%s" % (arch, filename))
182+ try:
183+ osextras.fetch(config, url, target)
184+ if target.endswith("squashfs"):
185+ sign.sign_cdimage(config, target)
186+ found = True
187+ except osextras.FetchError:
188+ pass
189 return found
190
191
192@@ -796,9 +801,6 @@
193 elif download_live_items(config, arch, "cloop"):
194 got_image = True
195 elif download_live_items(config, arch, "squashfs"):
196- download_live_items(config, arch, "installer.squashfs")
197- download_live_items(config, arch, "maas-rack.squashfs")
198- download_live_items(config, arch, "maas-region.squashfs")
199 download_live_items(config, arch, "modules.squashfs")
200 got_image = True
201 elif download_live_items(config, arch, "rootfs.tar.gz"):
202@@ -904,10 +906,5 @@
203 if project == "edubuntu" and config["CDIMAGE_DVD"]:
204 for arch in config.arches:
205 if arch in ("amd64", "i386"):
206- # TODO: Disabled for raring (LP: #1154601)
207- # if series >= "raring":
208- # # Fetch the Ubuntu Server squashfs for Edubuntu Server.
209- # download_live_items(config, arch, "server-squashfs")
210-
211 # Fetch the i386 LTSP chroot for Edubuntu Terminal Server.
212 download_live_items(config, arch, "ltsp-squashfs")
213
214=== modified file 'lib/cdimage/tests/test_livefs.py'
215--- lib/cdimage/tests/test_livefs.py 2019-03-12 08:44:31 +0000
216+++ lib/cdimage/tests/test_livefs.py 2019-03-12 08:44:31 +0000
217@@ -1201,50 +1201,56 @@
218 mock_fetch.assert_called_once_with(
219 self.config, url, os.path.join(target_dir, "i386.squashfs"))
220
221- @mock.patch("cdimage.osextras.fetch")
222- def test_download_live_items_server_squashfs(self, mock_fetch):
223- self.config["PROJECT"] = "edubuntu"
224- self.config["DIST"] = "trusty"
225- self.config["IMAGE_TYPE"] = "dvd"
226- self.assertTrue(
227- download_live_items(self.config, "i386", "server-squashfs"))
228- url = ("http://cardamom.buildd/~buildd/LiveCD/trusty/ubuntu-server/"
229- "current/livecd.ubuntu-server.squashfs")
230- target_dir = os.path.join(
231- self.temp_dir, "scratch", "edubuntu", "trusty", "dvd", "live")
232- mock_fetch.assert_called_once_with(
233- self.config, url, os.path.join(target_dir, "i386.server-squashfs"))
234-
235- @mock.patch("cdimage.osextras.fetch")
236- def assert_server_live_download_items(self, series, item, filenames,
237- mock_fetch):
238- self.config["PROJECT"] = "ubuntu-server"
239- self.config["SUBPROJECT"] = "live"
240+ def assert_desktop_live_download_items(self, series, item, filenames):
241+ self.assert_live_download_items("ubuntu", "", series, item,
242+ filenames, filenames)
243+
244+ def assert_desktop_live_download_items_with_expected(self, series, item,
245+ filenames,
246+ expected_files):
247+ self.assert_live_download_items("ubuntu", "", series, item,
248+ filenames, expected_files)
249+
250+ def assert_server_live_download_items(self, series, item, filenames):
251+ self.assert_live_download_items("ubuntu-server", "live", series, item,
252+ filenames, filenames)
253+
254+ @mock.patch("cdimage.osextras.fetch")
255+ def assert_live_download_items(self, project, subproject, series, item,
256+ filenames, expected_files, mock_fetch):
257+ artefacts_dir = self.use_temp_dir()
258+ self.config["PROJECT"] = project
259+ self.config["SUBPROJECT"] = subproject
260 self.config["DIST"] = series
261 self.config["IMAGE_TYPE"] = "daily-live"
262- self.assertTrue(
263- download_live_items(self.config, "amd64", item))
264+ self.config["LIVECD"] = artefacts_dir
265 calls = []
266+ project_path = project
267+ if subproject:
268+ project_path = "%s-%s" % (project_path, subproject)
269 for filename in filenames:
270- url = ("http://kapok.buildd/~buildd/LiveCD/%s/ubuntu-server-live/"
271- "current/livecd.ubuntu-server.%s") % (series, filename)
272+ uri = os.path.join(
273+ artefacts_dir, series,
274+ project_path, "current",
275+ "livecd.%s.%s" % (project, filename))
276+ touch(uri)
277+ for filename in expected_files:
278 target = os.path.join(
279- self.temp_dir, "scratch", "ubuntu-server", series,
280+ self.temp_dir, "scratch", project, series,
281 "daily-live", "live", "amd64." + filename)
282- calls.append(mock.call(self.config, url, target))
283+ uri = os.path.join(
284+ artefacts_dir, series,
285+ project_path, "current",
286+ "livecd.%s.%s" % (project, filename))
287+ calls.append(mock.call(self.config, uri, target))
288+
289+ self.assertTrue(
290+ download_live_items(self.config, "amd64", item))
291 self.assertCountEqual(mock_fetch.call_args_list, calls)
292
293 def test_download_live_items_installer_squashfs(self):
294 self.assert_server_live_download_items(
295- "bionic", "installer.squashfs", ["installer.squashfs"])
296-
297- def test_download_live_items_maas_rack_squashfs(self):
298- self.assert_server_live_download_items(
299- "bionic", "maas-rack.squashfs", ["maas-rack.squashfs"])
300-
301- def test_download_live_items_maas_region_squashfs(self):
302- self.assert_server_live_download_items(
303- "bionic", "maas-region.squashfs", ["maas-region.squashfs"])
304+ "bionic", "squashfs", ["installer.squashfs"])
305
306 def test_download_live_server_boot_items(self):
307 self.assert_server_live_download_items(
308@@ -1257,6 +1263,50 @@
309 "bionic", "modules.squashfs",
310 ["modules.squashfs-generic", "modules.squashfs-generic-hwe"])
311
312+ @mock.patch("cdimage.osextras.fetch")
313+ def test_download_live_items_multi_layers_squashfs(self, mock_fetch):
314+ self.assert_desktop_live_download_items(
315+ "disco", "squashfs",
316+ ["minimal.standard.live.squashfs"])
317+
318+ @mock.patch("cdimage.osextras.fetch")
319+ def test_download_live_items_multiple_squashfses(self, mock_fetch):
320+ self.assert_desktop_live_download_items(
321+ "disco", "squashfs",
322+ ["minimal.squashfs",
323+ "minimal.standard.squashfs",
324+ "minimal.standard.live.squashfs"])
325+
326+ @mock.patch("cdimage.osextras.fetch")
327+ def test_download_live_items_filter_suffix(self, mock_fetch):
328+ self.assert_desktop_live_download_items_with_expected(
329+ "disco", "squashfs",
330+ ["minimal.squashfs", "minimal.size"], ["minimal.squashfs"])
331+
332+ @mock.patch("cdimage.osextras.fetch")
333+ def test_download_remote_doesnt_honor_suffix(self, mock_fetch):
334+ '''Downloading from remote (not LP or local path) will only download
335+ basic names'''
336+ self.config["PROJECT"] = "ubuntu"
337+ self.config["DIST"] = "disco"
338+ self.config["IMAGE_TYPE"] = "daily-live"
339+ target_dir = os.path.join(
340+ self.temp_dir, "scratch", "ubuntu", "disco", "daily-live", "live")
341+
342+ self.assertTrue(
343+ download_live_items(self.config, "amd64", "squashfs"))
344+ mock_fetch.assert_called_once_with(
345+ self.config,
346+ "http://kapok.buildd/~buildd/LiveCD/disco/ubuntu/" +\
347+ # This can't be livecd.ubuntu.minimal.standard.live.squashfs for
348+ # instance
349+ "current/livecd.ubuntu.squashfs",
350+ os.path.join(
351+ target_dir,
352+ # This can't be amd64.minimal.standard.live.squashfs for
353+ # instance
354+ "amd64.squashfs"))
355+
356 def test_write_autorun(self):
357 self.config["PROJECT"] = "ubuntu"
358 self.config["DIST"] = "trusty"

Subscribers

People subscribed via source and target branches