tests: ubuntu-image 2.0 compatibility fixes #11073

bboozzoo · 2021-11-17T13:14:38Z

We actually want EXTRA_FUNDAMENTAL to be word split as it may contain multiple
arguments, see a related failure when preparing core 20:

+ mv core20.snap /home/image/core20.snap
+ EXTRA_FUNDAMENTAL='--extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap'
+ /snap/bin/ubuntu-image snap -w /home/image /home/image/pc.model --channel edge '--extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap' --extra-snaps /home/image/snapd_2.53.2+git628.g35bb479-dirty_amd64.snap --output /home/image/core20-amd64.img
Error: unknown flag `extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap'
-----

We actually want EXTRA_FUNDAMENTAL to be word split as it may contain multiple arguments, see a related failure when preparing core 20: + mv core20.snap /home/image/core20.snap + EXTRA_FUNDAMENTAL='--extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap' + /snap/bin/ubuntu-image snap -w /home/image /home/image/pc.model --channel edge '--extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap' --extra-snaps /home/image/snapd_2.53.2+git628.g35bb479-dirty_amd64.snap --output /home/image/core20-amd64.img Error: unknown flag `extra-snaps /home/image/pc-kernel_*.snap --snap /home/image/core20.snap' ----- Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

codecov-commenter · 2021-11-17T13:43:03Z

Codecov Report

Merging #11073 (355ed0e) into master (b19c7d4) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11073      +/-   ##
==========================================
- Coverage   78.37%   78.35%   -0.02%     
==========================================
  Files         922      923       +1     
  Lines      105128   105335     +207     
==========================================
+ Hits        82389    82537     +148     
- Misses      17609    17659      +50     
- Partials     5130     5139       +9

Flag	Coverage Δ
unittests	`78.35% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
osutil/synctree.go	`76.41% <0.00%> (-2.84%)`	⬇️
gadget/install/partition.go	`79.48% <0.00%> (-1.25%)`	⬇️
usersession/client/client.go	`81.68% <0.00%> (-0.80%)`	⬇️
overlord/ifacestate/helpers.go	`76.96% <0.00%> (-0.49%)`	⬇️
overlord/overlord.go	`80.69% <0.00%> (-0.20%)`	⬇️
wrappers/core18.go	`0.00% <0.00%> (ø)`
usersession/agent/rest_api.go	`0.00% <0.00%> (ø)`
interfaces/builtin/polkit.go	`90.16% <0.00%> (ø)`
overlord/snapstate/handlers.go	`71.15% <0.00%> (+0.03%)`	⬆️
dirs/dirs.go	`94.02% <0.00%> (+0.07%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b19c7d4...355ed0e. Read the comment docs.

mvo5

Thank you

stolowski

LGTM

sergiocazzolato

tx

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64

lgtm, but AIUI we also still want ubuntu-image 1.11 because the new go based one is not working with uc18 at least for some reason

…tra-snaps-args

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…-snaps-args

mardy

LGTM!

anonymouse64 · 2021-11-18T15:06:25Z

seems some tests are still unhappy, this PR is still using candidate which is 2.0 AIUI

bboozzoo · 2021-11-18T15:36:02Z

seems some tests are still unhappy, this PR is still using candidate which is 2.0 AIUI

Yes, that's intended, a fix using latest/stable landed in master. This branch reverts back to candidate and add bits for compatibility

anonymouse64 · 2021-11-18T15:37:36Z

ah okay thanks for clarifying I was confused if this branch was meant to fix master or not

…-snaps-args

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 · 2021-12-01T17:29:36Z

We also need to run nested tests with the new ubuntu-image, some of these will fail as discussed, since ubuntu-image will not be built with the fakestore test keys built into it. The suggestion for now was just to rebuild the ubuntu-image snap for these tests in order to inject the keys.

…-snaps-args

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2021-12-07T13:28:41Z

+ /snap/bin/ubuntu-image snap --image-size 10G /home/gopath/src/github.com/snapcore/snapd/tests/lib/assertions/developer1-20-secured.model --output-dir /tmp/work-dir/images --snap /tmp/work-dir/assets/pc-kernel_5.4.0-92.103.1_amd64.snap --snap /home/gopath/src/github.com/snapcore/snapd/tests/nested/manual/core20-grade-signed-above-testkeys-boot/snapd-from-deb.snap --snap /home/gopath/src/github.com/snapcore/snapd/tests/nested/manual/core20-grade-signed-above-testkeys-boot/new-core20.snap --snap /home/gopath/src/github.com/snapcore/snapd/tests/nested/manual/core20-grade-signed-above-testkeys-boot/extra-snaps/pc_20-0.4_amd64.snap
Error: Error preparing image: cannot override channels, add devmode snaps, local snaps, or extra snaps with a model of grade higher than dangerous

this one looks like it's intentional

bboozzoo · 2021-12-07T13:29:19Z

And someone in the forum had a similar problem? https://forum.snapcraft.io/t/cannot-build-image-error-preparing-image-cannot-override-channels-add-devmode-snaps-local-snaps-or-extra-snaps-with-a-model-of-grade-higher-than-dangerous/27836

bboozzoo · 2021-12-07T14:18:17Z

FWIW I suspect the problem with ubuntu-image is because it was not built with testkeys, thus assertions signed by testrootorg/developer1 would be rejected.

…-snaps-args

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

code has changed

bboozzoo · 2022-01-05T12:20:50Z

I've pushed patches that build ubuntu-image from Go source with withtestkeys tags, thus skipping the ubuntu-image snap completely, but it should make it possible to build signed/secured model images again. This probably warrants an agreement whether that's the right way to go.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64

lgtm, only concern I have is following main git branch which is probably as close as we can get to following the "bleeding edge" of ubuntu-image, but I think it's probably fine for now and we can think about a better risk management strategy for ubuntu-image later on (maybe we could download/install the snap from beta/candidate and check what git commit that was built with and use that as our base?)

anonymouse64 · 2022-01-10T22:18:23Z

spread.yaml

+            (
+                export GO111MODULE=off
+                # use go get so that ubuntu-image is built with current snapd sources
+                go get github.com/canonical/ubuntu-image/cmd/ubuntu-image


it would be nice to fetch a specific tag/branch/etc rather than just pulling straight from main, but not sure what is appropriate

There are tags which correspond to ubuntu-image package versions, eg. 2.1+22.04ubuntu2

Yeah I saw those, they are not great in that we would need to keep constantly bumping them I think given the tags are for the development release and it seems unlikely there will be constant SRU's for the package into jammy after it's released given that there is the snap and most people will consume it through the snap I think.

spread.yaml

ca-certificates were outdated on those hosts, thus go getting ubuntu-image failed https certificate checks failed Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2022-01-11T12:18:58Z

Turns out we cannot universally use ubuntu-image built directly from Go, as it runs mkfs.ext4 -d when creating the image. However, the version of e2fsprogs shipped with 16.04 do not did not support -d 😢 Fortunately, AFAICT we do not need to run the tests that would fail if ubuntu-image does not know about test keys on 16.04, so I'm hoping this next run will be successful and we can finally merge this branch.

Since the host mkfs.ext4 is too old and does not support -d option. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

There may be byte differences in size, so use swapon --show which uses rounded numbers in the output. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 · 2022-01-11T17:18:33Z

tests/core/swapfiles/task.yaml

+  # use swapon as it rounds up the numbers nicely
+  retry -n 60 --wait 1 bash -c "swapon --show | MATCH '\s+file\s+200M\s'"


is this related to ubuntu-image ?

Yes, for some reason there were byte size changes here (204796 -> 204784, 3 pages?). I suspect this can be caused by a different fs configuration, as looking at /usr/bin/mkswapfile the could should do the right and create 200MB sized file using fallocate and then runs mkswap on that. OTOH swapon --show correctly reports 200M size in both cases.

hmm interesting, but sure makes sense

anonymouse64

lgtm, wondering why the swap changes?

mardy

LGTM, added a minor question.

mardy · 2022-01-12T08:56:01Z

tests/lib/prepare.sh

+    if os.query is-core16; then
+        # the new ubuntu-image expects mkfs to support -d option, which was not
+        # supported yet by the version of mkfs that shipped with Ubuntu 16.04
+        snap install ubuntu-image --channel="$UBUNTU_IMAGE_SNAP_CHANNEL" --classic


maybe we should define (and export) the UBUNTU_IMAGE variable at this point, since we currently define it in two places?

It would have to be a separate helper in one of the scripts, just exporting it here would not make it availble in other shell fragments. OTOH maybe we could pull a trick and symlink to the right one, but that sounds like a followup material anyway.

bboozzoo added the Simple 😃 A small PR which can be reviewed quickly label Nov 17, 2021

mvo5 approved these changes Nov 17, 2021

View reviewed changes

stolowski approved these changes Nov 17, 2021

View reviewed changes

sergiocazzolato approved these changes Nov 17, 2021

View reviewed changes

bboozzoo added 2 commits November 17, 2021 20:32

tests/lib/prepare: replace --extra-snaps with --snap

f2efdf2

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/prepare-image-uboot: replace --extra-snaps with --snap

0906c04

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 approved these changes Nov 17, 2021

View reviewed changes

sergiocazzolato and others added 4 commits November 18, 2021 08:53

fix output-dir

367aae1

Unify the name of the image

e55fbf2

Merge branch 'tests-use-ubuntu-image-from-stable' into boozzoo/fix-ex…

74c1e40

…tra-snaps-args

spread: revert ubuntu-image back to candidate channel

94b14f6

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo changed the title ~~tests/lib/prepare: allow word splitting of EXTRA_FUNDAMENTAL~~ tests: ubuntu-image 2.0 compatibility fixes Nov 18, 2021

This was referenced Nov 18, 2021

tests: use ubuntu-image 1.11 from stable channel #11077

Merged

tests: use --snap instead of --extra-snaps to create the core image #11075

Closed

Merge remote-tracking branch 'upstream/master' into boozzoo/fix-extra…

8e314b9

…-snaps-args

mardy previously approved these changes Nov 18, 2021

View reviewed changes

bboozzoo added 3 commits November 23, 2021 08:29

Merge remote-tracking branch 'upstream/master' into boozzoo/fix-extra…

a346ca7

…-snaps-args

Merge remote-tracking branch 'upstream/master' into boozzoo/fix-extra…

1f3fd34

…-snaps-args

spread: try ubuntu-image from latest/edge

3145e7e

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 added the Run nested The PR also runs tests inluded in nested suite label Dec 1, 2021

anonymouse64 closed this Dec 1, 2021

anonymouse64 reopened this Dec 1, 2021

bboozzoo added 2 commits December 7, 2021 10:46

Merge remote-tracking branch 'upstream/master' into boozzoo/fix-extra…

302edac

…-snaps-args

tests/lib/nested: drop quoting of ubuntu-image arguments

f2c08d0

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

test/lib/nested: ubuntu-image no longer supports --output

2a97a75

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added 2 commits January 5, 2022 12:33

Merge remote-tracking branch 'upstream/master' into boozzoo/fix-extra…

cb7a66b

…-snaps-args

spread, tests: build a custom ubuntu-image with test keys

2b214b5

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo requested review from anonymouse64 and mvo5 January 5, 2022 12:19

bboozzoo added 2 commits January 5, 2022 16:08

tests/lib/pkgdb: install mtools

c3b5ed9

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

spread, tests: use current snapd sources when building ubuntu-image

b80ebab

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 reviewed Jan 10, 2022

View reviewed changes

anonymouse64 approved these changes Jan 10, 2022

View reviewed changes

spread: update ca-certificates on host when running nested tests

3c62808

ca-certificates were outdated on those hosts, thus go getting ubuntu-image failed https certificate checks failed Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added 2 commits January 11, 2022 15:59

spread, tests/lib/prepare: use ubuntu-image from snap on 16.04

1013140

Since the host mkfs.ext4 is too old and does not support -d option. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/core/swapfiles: better size checks

355ed0e

There may be byte differences in size, so use swapon --show which uses rounded numbers in the output. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo force-pushed the boozzoo/fix-extra-snaps-args branch from 97f9f8b to 355ed0e Compare January 11, 2022 15:14

bboozzoo requested review from sergiocazzolato, mardy and stolowski January 11, 2022 15:15

anonymouse64 reviewed Jan 11, 2022

View reviewed changes

anonymouse64 approved these changes Jan 11, 2022

View reviewed changes

mardy approved these changes Jan 12, 2022

View reviewed changes

mvo5 merged commit d53b1bb into snapcore:master Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: ubuntu-image 2.0 compatibility fixes #11073

tests: ubuntu-image 2.0 compatibility fixes #11073

bboozzoo commented Nov 17, 2021

codecov-commenter commented Nov 17, 2021 •

edited

mvo5 left a comment

stolowski left a comment

sergiocazzolato left a comment

anonymouse64 left a comment

mardy left a comment

anonymouse64 commented Nov 18, 2021

bboozzoo commented Nov 18, 2021

anonymouse64 commented Nov 18, 2021

anonymouse64 commented Dec 1, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Jan 5, 2022

anonymouse64 left a comment

anonymouse64 Jan 10, 2022

bboozzoo Jan 11, 2022

anonymouse64 Jan 11, 2022

bboozzoo commented Jan 11, 2022

anonymouse64 Jan 11, 2022

bboozzoo Jan 12, 2022

anonymouse64 Jan 12, 2022

anonymouse64 left a comment

mardy left a comment

mardy Jan 12, 2022

bboozzoo Jan 13, 2022

		# use swapon as it rounds up the numbers nicely
		retry -n 60 --wait 1 bash -c "swapon --show \| MATCH '\s+file\s+200M\s'"

tests: ubuntu-image 2.0 compatibility fixes #11073

tests: ubuntu-image 2.0 compatibility fixes #11073

Conversation

bboozzoo commented Nov 17, 2021

codecov-commenter commented Nov 17, 2021 • edited

Codecov Report

mvo5 left a comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

sergiocazzolato left a comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

mardy left a comment

Choose a reason for hiding this comment

anonymouse64 commented Nov 18, 2021

bboozzoo commented Nov 18, 2021

anonymouse64 commented Nov 18, 2021

anonymouse64 commented Dec 1, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Dec 7, 2021

bboozzoo commented Jan 5, 2022

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

mardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2021 •

edited