torcx/perform: support squashfs archives #96

euank · 2018-02-16T06:36:04Z

Fixes #62 (see discussion there)

This change was largely for my own edification, though I think it does work (tested with the overlay and scripts changes in the 'torcx-gourd' branches on my forks).

Note: This change adds squashfs support for stores, irrespective of any changes in profiles or other version bumps.
Documentation-wise, I've called it out in the v1 profile only, but in reality the code operates after versions have been normalized already and so happily will handle a v0 profile with a squashfs addon.
I'm fine accepting this behavior, but I would like to call it out.

If both tgz and squashfs addons are available, it will take the first one lexicographically, but it's documented as being arbitrary in case we wis hto change that.
In practice, users should avoid having multiple different formats available for the same addon reference.

squashfs addons do support a loopback device, and those are a limited resource. I think the typical user will have few enough addons that it won't be an issue, but it's worth noting.

euank · 2018-03-09T23:22:32Z

Rebased, it's now at a much more manageable diff size with #103 already being merged

euank · 2018-03-09T23:23:13Z

scripts/build.sh

@@ -35,7 +35,7 @@ if [ -n "${BUILDTAGS}" ]; then
 fi

 export GOARCH="${ARCH}"
-export CGO_ENABLED=0
+export CGO_ENABLED=1


This is not strictly necessary, however it's nice to be able to share the cgo consts in the loopback code, which is what this allows

If it is only for constants sharing, I'd pretty much prefer sticking to the current pure-go binary. However I fear we may need the C part for some of the ioctl() wrappers, but I didn't dig deeper.

I did manage to implement it without cgo; I can switch back and just be a little unhappy about the consts.

I removed the cgo dependency. I'm happy to squash idown those commits, but they're kept separate for now.

euank · 2018-03-09T23:25:42Z

ftests/rkt.go

+
+// binLibs is a helper that prints libraries a given binary is dynamically
+// linked against, including recursive dependencies of those libraries.
+func binLibs(binPath string) ([]string, error) {


This bit is a quite icky hack. It may be more desirable to rework the ftest code to use an environment similar to CL (and probably something other than rkt) to run dynamically linked binaries rather than take this.

Are you thinking about something like downloading a CL image and running tests in there via systemd-nspawn?

Something like that would be nice. If we wanted to use docker, there's also a friendly user who packaged the dev container up that way

lucab · 2018-03-26T14:22:39Z

Sorry for the radio silence. I took a deep dive into squashfs and I see the merit of using it instead of the current tmpfs+tgz approach. I'm introducing a new profile manifest revision in #105, so I'm bundling a new format field in there for non-tgz addon images.

lucab · 2018-03-26T14:35:31Z

Documentation/schemas/profile-manifest-v0.md

-  Referenced image must be available in the storepath, as a file name `${image}:${reference}.torcx.tgz`.
+  Referenced image must be available in the storepath, as a file name `${image}:${reference}.torcx.${format}`
+- value/images/#/format : string, either "tgz" or "squashfs" to indicate the image is stored as either a gzipped tarball or squashfs archive.
+  For backwards compatibility reasons, format will default to "tgz", but "squashfs" should be preferred for new images.


squashfs supports had a few (deprecated) major versions and it also supports multiple compression versions, which we should probably specifiy here. I'm mostly concerned with the latter.

Would you prefer to pick a specific compression algorithm, or to have multiple formats with compression identifiers (i.e. sqsh.gz)? I'd lean toward the former.

I also lean towards the former. I can rebase on #105 and indicate the single supported version+compression algorithm.

squeed · 2018-03-27T10:17:01Z

I don't think the image format should be part of the profile. Rather, profiles are intended to be a list of name + version pairs. Then, the install resolution logic is responsible for turning that in to a path on disk to install.

So, Format shouldn't be part of the Image type, but rather the Archive type.

lucab · 2018-03-27T12:32:25Z

I think both approaches are feasible, it's just a matter of being on the explicit or on the implicit side.

If we make this implicit, detection will happen when populating the store metadata cache (via the Archive above). We'll need to tie-break if there are multiple images with different formats in stores. At the same time it will hide away some complexity from the user.

Making this explicit will allow remotes to provide multiple format via a single manifest, so that client can fetch the relevant one for their profile.

I'm generally in favor of the explicit approach, and I think it will make transition and future tooling a bit simpler, but I don't have a strong opinion. @squeed except for code architecture, do you have any specific concern with sourcing the format information from user-provided metadata?

squeed · 2018-03-27T14:26:30Z

From a more abstract perspective, I think it's a cleaner architecture for the profile to be divorced from the store as much as possible. Not doing so means that tooling like tectonic-torcx now needs to be rewritten to update profiles between OS versions.

Perhaps more unfriendly from a user perspective, is that an administrator who wishes to use a profile that includes one or more vendor-provided packages (i.e. reference com.coreos.cl) now needs to check each OS store to see which packaging format is in use. This makes me feel like we haven't quite got the right level of abstraction.

lucab · 2018-05-07T09:07:58Z

@euank ping, I'd like to hear your feedback on this.

As stated, I don't have strong opinions here but I see that @squeed has a point wrt. the admin having to know in advance about the whole versions/images/formats matrix.
As such, perhaps we can handle this all internally in torcx without exposing it in the profile?

euank · 2018-05-08T20:05:25Z

After thinking about it more, I agree with @squeed that it's a little nicer to tie it to Archive rather than Image in torcx's internal representation.

I still don't quite like that it's now implicitly determined from what files do exist rather than explicitly.

That being said, I think it's still a better option given what we have now.

I've started refactoring the code in that direction, but that's still WIP for the moment.
I do think the documentation portion has been updated appropriately already, so review of that is welcome.

euank · 2018-05-09T00:05:24Z

I've noticed that the proposed torcx-remote-contents-v1 manifest also includes a format field. I think that's an argument in favour of dropping it from the profile manifest since the distributor of the file will have already specified the format explicitly once.

I'm also marking this as blocked on #105 since I wish to build this on the v1 profile rather than modify the v0 one as it currently does.

lucab · 2018-05-09T13:56:39Z

@euank I was actually wondering if the descriptors there should expose the format field, and it seems that indeed it would make sense.

euank · 2018-05-09T23:01:45Z

I think this is ready for review now!

I've added a functional test for mounting a squashfs addon with torcx-generator, and it seems to work as I'd expect. It does require a loopback device to be available, but it also frees it afterwards (when rkt unmount's the container's rootfs it gets the loopback mount too conveniently) and seems like it should work on most systems.

This implementation doesn't tie squashfs support to any particular profile version, other than only documenting it for the newer version. I'm okay with that, but it differs from what I commented above so I'd like to call it out.
If it's desirable that it be more explicit, it could be, though I think the code would be a bit more complicated and I don't see all that much benefit.

lucab · 2018-05-11T12:45:52Z

Documentation/design/principles.md

@@ -18,7 +18,7 @@ These are the non-goals and out-of-scope topics that `torcx` wants to avoid:
 1. full packaging system: it should not be involved into complex upgrade paths
 1. upgrading and downgrading packages at runtime: profile activation is an atomic operation performed at-most-once at boot-time
 1. pre-removal and post-installation custom logic: changes performed by torcx are (mainly) volatile and are meant to only last for a single boot
-1. defining a custom package format: torcx just handles tgz rootfs archives
+1. defining a custom package format: torcx just handles squashfs rootfs archives


I think we can simply drop this line. Initially we wanted to just re-use all of OCI specs/toolings, but in the end it was not feasible.

lucab · 2018-05-11T12:47:05Z

Documentation/design/ux.md

@@ -3,7 +3,7 @@
 ## Concepts

 * *torcx*: a CLI tool organized in subcommand
- * *archive*: an torcx.tgz archive containing addon binaries and assets
+ * *archive*: an torcx.squashfs archive containing addon binaries and assets


I would just say "a compressed archive ...".

lucab · 2018-05-11T12:53:28Z

internal/torcx/types.go

+
+// FileSuffix returns the file extension this archive format must have.
+func (arf ArchiveFormat) FileSuffix() string {
+	return fmt.Sprintf(".torcx.%s", arf)


Should we default the zero value to tgz in here? Or take care of it in some other ways?

The intent is that the zero value is never used, so I'd rather either ignore it here or panic here.

Defaulting to tgz should happen a layer higher. Are you okay with it just being the caller's responsibility, or would you like me to panic / return an error / handle it here?

I guess it's a matter of taste then, so I won't insist. I slightly prefer returning a string+error tuple, but I'm also fine with leaving it to the caller (as is).

lucab · 2018-05-11T12:54:51Z

Documentation/design/images.md

 Such archives are typically custom-built and tailored for torcx.

+A torcx squashfs archive *MUST* be a [version 4.0](https://github.com/torvalds/linux/blob/v4.16/Documentation/filesystems/squashfs.txt) squashfs filesystem archive. It *MUST* be compressed using either gzip or lz4.


Just for reference, are gzip+lz4 chosen because of current ContainerLinux kconfig or for any other specific reason?

I arbitrarily picked from those supported by the current squashfs tools we have.
We must choose to only support a subset of them to be forwards compatible with newer squashfs tools which may add more formats, and for simplicity I thought picking those two options would cover common needs (the default of gzip and one which has very fast performance but lower compression).

lucab · 2018-05-11T12:58:37Z

Documentation/schemas/profile-manifest-v1.md

-  Referenced image will be locally looked up as a file named `${name}:${reference}.torcx.tgz`.
+  Referenced image will be locally looked up as a file named
+  `${name}:${reference}.torcx.${format}` where `format` may be either `tgz` or
+  `squashfs`. If both exist, either may arbitrarily be chosen.


IMHO the last sentence is somehow problematic. Shouldn't we prefer "deterministically" and "squashfs"?
This would also be reflected in ArchiveFor logic.

lucab · 2018-05-11T13:00:51Z

Thanks, I think it is pretty much in great shape.

I only have one real concern about deterministic ordering and preferences, rest of comments are mostly cosmetic.

squeed · 2018-05-11T15:57:32Z

ftests/rkt.go

@@ -87,3 +89,24 @@ func RunTestInContainer(t *testing.T, cfg RktConfig) {
 		t.Error(string(bytes))
 	}
 }
+
+func loopbackMonuts(t *testing.T) []string {


typo (monuts)

squeed · 2018-05-11T15:58:11Z

internal/third_party/docker/README.txt

@@ -0,0 +1,4 @@
+Code in this directory was imported from the docker projcet, under the Apache


typo (projcet)

squeed · 2018-05-11T15:59:22Z

internal/torcx/perform.go

+	}
+	defer loopDev.Close()
+
+	if err := unix.Mount(loopDev.Name(), topDir, "squashfs", unix.MS_RDONLY, ""); err != nil {


We should make sure that /run is mounted shared-recursive, in case anyone wants to bind-mount it in to a container.

Nevermind - this runs so early on, it doesn't really matter.

squeed · 2018-05-11T16:02:49Z

internal/torcx/perform.go

+		case ArchiveFormatSquashfs:
+			imageRoot, err = mountSquashfs(applyCfg, archive.Filepath, im.Name)
+		default:
+			logrus.WithFields(logFields).Error("unrecognized format for archive: ", archive)


does it make sense to set err here so we track failed images? (this is very hypothetical)

I think it does make sense, though realistically this code is unreachable so could also panic (since ArchiveFor several lines up can only return a tgz or squashfs).

updated to add to failed images so it will fail more noisily if the code changes or I'm missing something.

squeed · 2018-05-11T16:04:09Z

A few tiny things, but LGTM otherwise.

euank · 2018-05-15T00:21:09Z

I believe I addressed all present comments.Note, the torcx/perform: err on unknown image formats more and torcx/store: deterministically prefer squashfs are behavioural changes, but the others changes since last time are a few typo fixes and the like.

euank · 2018-05-15T01:07:49Z

I went ahead and made sure an image with the current set of torcx changes + squashfs docker addon still boots correctly. I uplaoded the qemu image here if you'd like to poke around at anything.

I built it with current master + these scripts changes, overlay changes, and a cros_workon for torcx.

Stale

lucab · 2018-05-15T08:39:14Z

LGTM (I haven't tested your image at this point).

And improve naming for our intended use of it

These values should be constant across all modern linuxes, so it should be fine to simply hardcode them without going through cgo.

This tests the generator mounting a tgz and a squashfs addon. It verifies the contents of the 'unpack' directory matches the contents used to create the addons. Propogation can also be tested fairly easily, but to keep this a bit smaller I'm punting on that for the moment.

This defines behavior that was previously omitted to try and simplify the implementation. In reality, the simplication is minor to none and it's much nicer to define these semantics.

The code already printed out an error if an image format was unrecognized that late (which shouldn't in practice happen). This ensures it also later fails the operation by considering that image failed too.

euank · 2018-05-15T18:53:22Z

Did a trivial rebase and tests are still green. Merging..

euank force-pushed the gourd branch 2 times, most recently from 6e1614f to 6c54f40 Compare February 16, 2018 07:55

euank changed the title ~~[WIP] torcx/perform: support squashfs archives~~ torcx/perform: support squashfs archives Feb 16, 2018

euank force-pushed the gourd branch from 6c54f40 to 212bf21 Compare February 16, 2018 08:29

euank mentioned this pull request Mar 6, 2018

Update logrus+sys/unix packages, use unix over syscall #103

Merged

euank force-pushed the gourd branch from 212bf21 to 9d03975 Compare March 9, 2018 23:21

euank commented Mar 9, 2018

View reviewed changes

euank mentioned this pull request Mar 9, 2018

[WIP] torcx/perform: don't store unpacked data in /run #95

Closed

lucab reviewed Mar 26, 2018

View reviewed changes

lucab mentioned this pull request May 7, 2018

schemas: add profile-manifest-v1 #105

Merged

euank force-pushed the gourd branch from da3712f to bf14efe Compare May 8, 2018 20:02

euank force-pushed the gourd branch from bf14efe to 81b7ae4 Compare May 8, 2018 23:49

euank force-pushed the gourd branch 2 times, most recently from 5bc38a8 to 1a7c9c2 Compare May 9, 2018 22:51

lucab previously requested changes May 11, 2018

View reviewed changes

squeed reviewed May 11, 2018

View reviewed changes

euank force-pushed the gourd branch from 0da85d6 to ac11ad5 Compare May 15, 2018 00:13

lucab approved these changes May 15, 2018

View reviewed changes

euank and others added 12 commits May 15, 2018 10:30

docs/design: support squashfs formatted images

3f25c1d

third_party: import loopback code from moby

cfe87ad

third_party/loopback: remove import comments

5d546db

third_party/loopback: open source file readonly

dafe4a9

And improve naming for our intended use of it

third_party/loopback: remove cgo consts

65c80d3

These values should be constant across all modern linuxes, so it should be fine to simply hardcode them without going through cgo.

torcx/perform: support squashfs archives

4473943

ftests: add test torcx addon images

40ec1a2

torcx/paths: remove unused ArchiveFilename helper

09561f4

torcx/store: deterministically prefer squashfs

ee19f76

This defines behavior that was previously omitted to try and simplify the implementation. In reality, the simplication is minor to none and it's much nicer to define these semantics.

torcx/perform: err on unknown image formats more

55d1db4

The code already printed out an error if an image format was unrecognized that late (which shouldn't in practice happen). This ensures it also later fails the operation by considering that image failed too.

docs: update some squashfs wording

5c9c576

euank force-pushed the gourd branch from ac11ad5 to 5c9c576 Compare May 15, 2018 18:21

euank merged commit 71756e9 into coreos:master May 15, 2018

This was referenced May 23, 2018

[WIP] build_torcx_store: support squashfs for ondisk addons coreos/scripts#818

Closed

Container Linux memory usage with docker 17.xx unreasonably high coreos/bugs#2237

Closed

		Such archives are typically custom-built and tailored for torcx.

		A torcx squashfs archive MUST be a [version 4.0](https://github.com/torvalds/linux/blob/v4.16/Documentation/filesystems/squashfs.txt) squashfs filesystem archive. It MUST be compressed using either gzip or lz4.

		@@ -0,0 +1,4 @@
		Code in this directory was imported from the docker projcet, under the Apache

torcx/perform: support squashfs archives #96

torcx/perform: support squashfs archives #96

Conversation

euank commented Feb 16, 2018 • edited by lucab Loading

euank commented Mar 9, 2018

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

euank Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed commented Mar 27, 2018

lucab commented Mar 27, 2018

squeed commented Mar 27, 2018

lucab commented May 7, 2018

euank commented May 8, 2018

euank commented May 9, 2018

lucab commented May 9, 2018

euank commented May 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented May 11, 2018

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

squeed May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed commented May 11, 2018

euank commented May 15, 2018

euank commented May 15, 2018

lucab commented May 15, 2018

euank commented May 15, 2018

euank commented Feb 16, 2018 •

edited by lucab

Loading

euank Mar 9, 2018 •

edited

Loading

squeed May 11, 2018 •

edited

Loading