image: write resolved-content from snap prepare-image #9914

mvo5 · 2021-02-08T13:55:24Z

This commit adds code to create a new "resolved-content" directory
under opts.PrepareDir that can be picked up by ubuntu-image. This
dir will contain the resolved content for each volume structure
that should be written to the image.

The corresponding ubuntu-image code would be something like https://github.com/CanonicalLtd/ubuntu-image/compare/master...mvo5:snap-prepare-image-resolved-content?expand=1

This commit adds code to create a new "resolved-content" directory under opts.PrepareDir that can be picked up by ubuntu-image. This dir will contain the resolved content for each volume structure that should be written to the image.

pedronis · 2021-02-08T15:38:40Z

image/helpers.go

+	if err != nil {
+		return err
+	}
+	for _, ps := range lv.LaidOutStructure {


for UC20 this code will write files that are not needed, because we really need to take care only of system-seed, no? OTOH maybe it's ok because is ready if we do some of install work already at ubuntu-image time later.

ATM the layouts are already different:

UC16/18

<prepare-dir>/image

UC20

<prepare-dir>/system-seed

Yeah, this will produce too much data on uc20 currently (it. should stop after it found the system-seed).

It maybe okay for now that prepare-image just outputs all the resolved content and ubuntu-image picks what it needs. Longer term ubuntu-image will be in go and it will call better and richer APIs than a (very limited) CLI itnerface.

…t (slightly) less ugly

Using relative directories falls apart when used with ubuntu-image because ubuntu-image provides a relative dir as prepareDir here, and thus the symlink created is broken. Making it an absolute link fixes the problem. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64

this looks good to me, I pushed one change to fix the symlink issue I mention below, but otherwise I think it's good with an additional unit test

This also fully enables my uc20 pi-config work too so +2 from me 😄

image/helpers.go

image/image_linux.go

image/helpers.go

image/helpers_test.go

…explicitely

At this point we know already we are not in classic

pedronis

looks good, some comments mostly around the signature of writeResolvedContent

pedronis · 2021-02-26T10:54:40Z

image/helpers.go

+var writeResolvedContent = writeResolvedContentImpl
+
+// XXX: move to gadget?
+func writeResolvedContentImpl(targetDir, prepareDir, gadgetUnpackDir, kernelUnpackDir string, model *asserts.Model) error {


maybe s/targetDir/resolvedContentDIr/ ?

given the logic about the symlink and the fact it takes a prepareDir it should probably live here, it could be made a gadget function but it would need a more complicated interface.

It probably merits a doc comment though about what it does and the main params as it has so many

do we need to pass both targetDir and prepareDir? given where this lives it could compute one from the other:

prepareDir = targetDir/..

pedronis · 2021-02-26T10:56:41Z

image/image_linux.go

@@ -130,6 +130,13 @@ func unpackGadget(gadgetFname, gadgetUnpackDir string) error {
 	return snap.Unpack("*", gadgetUnpackDir)
 }

+func unpackKernel(kernelFname, kernelUnpackDir string) error {


bit unclear why the 2nd function copy given that they do the same thing, maybe just rename it to unpackEssentialSnap

Indeed, silly me. Removed the duplicated helper.

pedronis · 2021-02-26T11:04:25Z

image/helpers.go

+		return err
+	}
+
+	info, err := gadget.ReadInfoAndValidate(gadgetUnpackDir, model, nil)


there's an argument to be made that this function should receive the gadget.Info directly instead of the model.

the arguments would be:

targetDir, info, gadgetUnpackDir, kernelUnpackDir

Indeed, I like that. I updated the code and had to tweak some tests. Note that the signature is slightly different (not targetDir) as I applied the earlier feedback about just using gadgetUnpackDir. Let me know if you feel it should be done differently.

…s to Samuele)

…ted code)

This now take gadget.Info instead of the asserts.Model. Thanks to Samuele for the suggestion.

pedronis

thank you

anonymouse64

lgtm, thanks for the additional unit tests here

…cible Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2021-03-03T16:16:27Z

(I pushed a fix for the failing unit test which was racy I think since the output wasn't sorted)

bboozzoo

LGTM

image/helpers.go

mvo5 · 2021-03-05T17:29:20Z

After spending some quality time with ubuntu-image it looks like this needs a little bit of tweaking because volume structures often do not have names and ubuntu-image uses the structure number instead. So prepare-image should layout as <volume-name>/<structure-number> by default

Not all volume structures have names so to avoid issues do what ubuntu-image is doing and use the partition number whe writing the resolved content dir structure, e.g.: ``` resolved-content/vol1/part2/foo ```

mvo5 · 2021-03-08T15:27:09Z

Fwiw, I tested this with the coresponding ubuntu-image changes in https://github.com/CanonicalLtd/ubuntu-image/compare/master...mvo5:snap-prepare-image-resolved-content?expand=1 and got a working image that had the DTBs, seeded and subsequent kernel/gadget updates cause no observable issues.

Using canonical/ubuntu-image-legacy#198 and the snaps from https://people.canonical.com/~mvo/tmp/kernel-assets/ that include the changes from https://bugs.launchpad.net/snapd/+bug/1907056 (wihtout the assumes part) an image was created with:

$ UBUNTU_IMAGE_SNAP_CMD=/tmp/snap PYTHONPATH=. ./ubuntu-image snap \
  ubuntu-core-20-pi-arm64-dangerous.model \
  --snap ~/tmp/kernel-dtb/pi/pi_20-1_arm64.snap \
  --snap ~/tmp/kernel-dtb/pi-kernel/pi-kernel_5.4.0-1030.33_arm64.snap \
  --snap ~/tmp/kernel-dtb/snapd_r1140_arm64.snap \
  --snap ~/tmp/kernel-dtb/core20_r961_arm64.snap

Note that snapd/core20 are not modified for this.

And a similar successful install with UC18:

$ UBUNTU_IMAGE_SNAP_CMD=/tmp/snap PYTHONPATH=. ./ubuntu-image snap  \
 ubuntu-core-18-pi-arm64.model  \
 --snap ~/tmp/kernel-dtb/uc18/pi/pi_18-1_arm64.snap   \
 --snap ~/tmp/kernel-dtb/uc18/pi-kernel/pi-kernel_5.3.0-1037.39_arm64.snap  \
 --snap ~/tmp/kernel-dtb/snapd_r1140_arm64.snap  \
 --snap ~/tmp/kernel-dtb/uc18/core18_r1996_arm64.snap

Note that snapd/core18 are not modified for this.

mvo5 · 2021-03-08T17:37:50Z

@pedronis I did a small tweak to the dir structure in my latest commit, it uses resolved-content/<vol-name>/part<NR>/ now (e.g. resolved-content/pi/part1) because a lot of the volume structures do not have names and ubuntu-image is already doing the "part" bit internal. Would love to get your okay on this particular chagne before I merge this PR.

pedronis

looks ok as we discussed, some nitpicks about the comment

pedronis · 2021-03-09T10:28:00Z

image/helpers.go

@@ -412,10 +412,10 @@ var writeResolvedContent = writeResolvedContentImpl
 // writeResolvedContent takes gadget.Info and the unpacked
 // gadget/kernel snaps and outputs the resolved content from the
 // {gadget,kernel}.yaml into a filesystem tree with the structure:
-// <prepareImageDir>/resolved-content/<volume-name>/<structure-name>/...
+// <prepareImageDir>/resolved-content/<volume-name>/<structure-nr>/...


this is missing "part" ?

pedronis · 2021-03-09T10:28:36Z

image/helpers.go

 //
 // E.g.
-// /tmp/prep-img/resolved-content/pi/ubuntu-seed/{config.txt,bootcode.bin,...}
+// /tmp/prep-img/resolved-content/pi/part1/{config.txt,bootcode.bin,...}


is ubuntu-seed the first structure? then it should be part0 ?

anonymouse64

post merge +1 from me again

pedronis self-requested a review February 8, 2021 14:34

pedronis reviewed Feb 8, 2021

View reviewed changes

mvo5 added 3 commits February 25, 2021 16:52

fix writeResolvedContent to go over all volumes

a49b00b

image: use symlink in resolved-content for the system-seed

a03a7fe

image: pass prepareImageDir dir to writeResolvedContentImpl to make i…

099d50e

…t (slightly) less ugly

pedronis self-requested a review February 25, 2021 17:47

anonymouse64 self-requested a review February 25, 2021 22:42

anonymouse64 mentioned this pull request Feb 26, 2021

sysconfig, o/configstate/configcore: expose helper to apply pi-config at image build time #9965

Closed

2 tasks

anonymouse64 reviewed Feb 26, 2021

View reviewed changes

image/helpers.go Show resolved Hide resolved

image/image_linux.go Outdated Show resolved Hide resolved

image/helpers.go Outdated Show resolved Hide resolved

image/helpers_test.go Outdated Show resolved Hide resolved

image: tweak signature of WriteResolvedContent and include targetDir …

7cab875

…explicitely

mvo5 marked this pull request as ready for review February 26, 2021 08:48

mvo5 added the Skip spread Indicate that spread job should not run label Feb 26, 2021

mvo5 added 2 commits February 26, 2021 09:53

image: no need to check for !opts.Classic in setupSeed()

61c72ed

At this point we know already we are not in classic

image: add test for WriteResolvedContent from a relative dir

8b94daa

pedronis reviewed Feb 26, 2021

View reviewed changes

mvo5 added 3 commits March 1, 2021 10:43

image: improve signature/docstring of writeResolvedContentImpl (thank…

b0855d0

…s to Samuele)

image: rename unpack{Gadget,Kernel} -> unpackSnap (and remove duplica…

6b0b4a1

…ted code)

image: update signature of writeResolvedContentImpl()

acb726a

This now take gadget.Info instead of the asserts.Model. Thanks to Samuele for the suggestion.

pedronis approved these changes Mar 1, 2021

View reviewed changes

mvo5 removed the Skip spread Indicate that spread job should not run label Mar 1, 2021

mvo5 closed this Mar 1, 2021

mvo5 reopened this Mar 1, 2021

mvo5 added 2 commits March 1, 2021 16:58

Merge remote-tracking branch 'upstream/master' into kernel-dtb-refs-6

de179c1

image: drop SectorSize in writeResolvedContentImpl()

e754cf6

anonymouse64 self-requested a review March 1, 2021 17:44

bboozzoo self-requested a review March 3, 2021 14:34

anonymouse64 approved these changes Mar 3, 2021

View reviewed changes

image/helpers_test.go: sort find output to ensure the test is reprodu…

59dcc9b

…cible Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bboozzoo approved these changes Mar 4, 2021

View reviewed changes

image/helpers.go Outdated Show resolved Hide resolved

gadget: add new gadget.DefaultConstraints (thanks to Maciej)

6e976b1

mvo5 added 2 commits March 5, 2021 21:45

image: write resolved volume-content to <volname>/part<structure-nr>

0b7d742

Not all volume structures have names so to avoid issues do what ubuntu-image is doing and use the partition number whe writing the resolved content dir structure, e.g.: ``` resolved-content/vol1/part2/foo ```

Merge remote-tracking branch 'upstream/master' into kernel-dtb-refs-6

8750384

mvo5 mentioned this pull request Mar 8, 2021

ubuntu_image: use resolved content provided by snap prepare-image canonical/ubuntu-image-legacy#198

Merged

pedronis requested review from pedronis and anonymouse64 and removed request for anonymouse64 March 9, 2021 09:10

pedronis approved these changes Mar 9, 2021

View reviewed changes

mvo5 added the Skip spread Indicate that spread job should not run label Mar 9, 2021

image: tweak comments about resolved-content layout (thanks to Samuele)

1ca8eda

mvo5 merged commit 4a12a9f into snapcore:master Mar 9, 2021

anonymouse64 reviewed Mar 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image: write resolved-content from snap prepare-image #9914

image: write resolved-content from snap prepare-image #9914

mvo5 commented Feb 8, 2021

pedronis Feb 8, 2021

mvo5 Feb 26, 2021

mvo5 Feb 26, 2021

anonymouse64 left a comment

pedronis left a comment

pedronis Feb 26, 2021

pedronis Feb 26, 2021

mvo5 Mar 1, 2021

pedronis Feb 26, 2021

mvo5 Mar 1, 2021

pedronis left a comment

anonymouse64 left a comment

anonymouse64 commented Mar 3, 2021

bboozzoo left a comment

mvo5 commented Mar 5, 2021 •

edited by pedronis

mvo5 commented Mar 8, 2021 •

edited

mvo5 commented Mar 8, 2021

pedronis left a comment

pedronis Mar 9, 2021

pedronis Mar 9, 2021

anonymouse64 left a comment

image: write resolved-content from snap prepare-image #9914

image: write resolved-content from snap prepare-image #9914

Conversation

mvo5 commented Feb 8, 2021

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

pedronis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

anonymouse64 commented Mar 3, 2021

bboozzoo left a comment

Choose a reason for hiding this comment

mvo5 commented Mar 5, 2021 • edited by pedronis

mvo5 commented Mar 8, 2021 • edited

mvo5 commented Mar 8, 2021

pedronis left a comment

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

mvo5 commented Mar 5, 2021 •

edited by pedronis

mvo5 commented Mar 8, 2021 •

edited