Add create-layer sub command to create filesystem changeset #8

Open
wants to merge 9 commits into
from

Projects

None yet

6 participants

@coolljt0725
Contributor

This PR use a well tested pkg from github.com/containers/storage(which is forked from docker/docker)
from opencontainers/image-spec#280
here is an example:

$ oci-image-tool create-layer
One or two filesystems are required
Usage:
  oci-image-tool create-layer [child] [parent] [flags]

Flags:
  -h, --help   help for create-layer
$ tree rootfs-1
rootfs-1
├── bin
│   ├── my-app
│   └── my-tool
└── etc
    └── my-tool-cfg

2 directories, 3 files
[lei@fedora test]$ oci-image-tool create-layer rootfs-1
[lei@fedora test]$ ls
rootfs-1  rootfs-1.tar
[lei@fedora test]$ vim rootfs-1.tar
" tar.vim version v29
" Browsing tarfile /home/lei/test/rootfs-1.tar
" Select a file with cursor and press ENTER

bin/
bin/my-app
bin/my-tool
etc/
etc/my-tool-cfg
~

[lei@fedora test]$ cp -par rootfs-1 rootfs-1-s
[lei@fedora test]$ rm rootfs-1-s/bin/my-tool
[lei@fedora test]$ touch rootfs-1-s/bin/my-tool-1
[lei@fedora test]$ mkdir rootfs-1-s/etc/my-app-cfg.d
[lei@fedora test]$ touch rootfs-1-s/etc/my-app-cfg.d/cfg
[lei@fedora test]$ tree rootfs-1-s
rootfs-1-s
├── bin
│   ├── my-app
│   └── my-tool-1
└── etc
    ├── my-app-cfg.d
    │   └── cfg
    └── my-tool-cfg

3 directories, 4 files

[lei@fedora test]$ oci-image-tool create-layer rootfs-1-s/ rootfs-1
[lei@fedora test]$ ls
rootfs-1  rootfs-1-s  rootfs-1-s.tar  rootfs-1.tar
[lei@fedora test]$ vim rootfs-1-s.tar
" tar.vim version v29
" Browsing tarfile /home/lei/test/rootfs-1-s.tar
" Select a file with cursor and press ENTER

bin/
bin/.wh.my-tool
bin/my-tool-1
etc/
etc/my-app-cfg.d/
etc/my-app-cfg.d/cfg

@wking
Contributor
wking commented Sep 18, 2016

On Sat, Sep 17, 2016 at 09:05:40PM -0700, Lei Jitang wrote:

  • Run make update-deps to update dependency

    M vendor/github.com/opencontainers/runtime-spec/specs-go/config.go (44)
    M vendor/github.com/opencontainers/runtime-spec/specs-go/state.go (2)
    M vendor/github.com/opencontainers/runtime-spec/specs-go/version.go (2)

This pulls in the Linux → *Linux change from
opencontainers/runtime-spec#502. I think we want to make that bump an
explicit step, so we can catch up (e.g. updating runtimeSpec, which
currently uses s.Linux without initializing it because it couldn't
have been nil before).

@wking
Contributor
wking commented Sep 18, 2016

On Sat, Sep 17, 2016 at 09:05:40PM -0700, Lei Jitang wrote:

This PR use a well tested pkg from
github.com/containers/storage(which is forked from docker/docker)

Pulling in containers/storage is much better than pulling in all of
Docker. But it's still doing a lot more than we need (just
pkg/archive). Maybe the containers folks would be interested in
moving that out into a containers/archive repository?

here is an example:

$ oci-image-tool create-layer

Now just oci-create-layer. And you should add an
oci-create-layer.1.md to go with your new command.

cmd/oci-create-layer/main.go
+ }
+
+ cmd := &cobra.Command{
+ Use: "create-layer [child] [parent]",
@wking
wking Sep 18, 2016 Contributor

This should probably write the layer tar to stdout unless the user sets an option to write to a particular filename.

@coolljt0725
coolljt0725 Sep 18, 2016 Contributor

This will write the layer tar to the pwd.

Add an option to write to a particular filename is under working

image/layer.go
+// layer and its parent layer which may be "".
+func Diff(child, parent string) (arch archive.Archive, err error) {
+ if parent == "" {
+ archive, err := archive.Tar(child, archive.Uncompressed)
@wking
wking Sep 18, 2016 Contributor

Current image-spec requires gzipped layers, although there is some talk of weakening that in opencontainers/image-spec#316 (I'm in favor of making compression optional).

@coolljt0725
Contributor

@wking

This pulls in the Linux → *Linux change from
opencontainers/runtime-spec#502. I think we want to make that bump an
explicit step, so we can catch up (e.g. updating runtimeSpec, which
currently uses s.Linux without initializing it because it couldn't
have been nil before).

I just want to avoid updating other packages when run make update-deps to vendor the github.com/containers/storage, I want the commit cb84b39 only contains the packages it relay on, so I run make update-deps first, then develop my commit. Maybe there is a way to update a specified package without updating all the packages?

Now just oci-create-layer. And you should add an
oci-create-layer.1.md to go with your new command.

This is under working

@wking
Contributor
wking commented Sep 18, 2016 edited

On Sat, Sep 17, 2016 at 11:20:18PM -0700, Lei Jitang wrote:

@wking

This pulls in the Linux → *Linux change from
opencontainers/runtime-spec#502. I think we want to make that
bump an explicit step, so we can catch up (e.g. updating
runtimeSpec, which currently uses s.Linux without initializing it
because it couldn't have been nil before).

I just want to avoid updating other packages when run make update-deps to vendor the github.com/containers/storage, I want
the commit cb84b39 only contains
the packages it relay on, so I run make update-deps first, then
develop my commit. Maybe there is a way to update a specified
package without updating all the packages?

I think you can just roll back the packages you don't want to bump
with ‘git checkout HEAD -- vendor/path/to/revert’ and manually resetting
portions of glide.lock. See 2b694a6 for an example of me doing that.

@coolljt0725
Contributor

@wking thanks :)

@runcom
Contributor
runcom commented Sep 18, 2016

@coolljt0725 can't you just require a specific version of a library in glide.yaml? What's the point if having a field named version if we have to git reset and checkout what we don't need..

@wking
Contributor
wking commented Sep 18, 2016

On Sun, Sep 18, 2016 at 12:23:48AM -0700, Antonio Murdaca wrote:

@coolljt0725 can't you just require a specific version of a library
in glide.yaml?

Ah, there's a config file for this thing? ;). That's definitely a
better approach than what I was doing.

@glestaris
Contributor

I am very up for sharing a single unpacking implementation! I am not sure if this implementation should live outside of an opencontainers repository though. I would hope to see the image-tools becoming more of community maintained reference implementation in unpacking single layers.

Also, having to pull the whole containers/storage and its dependencies in image-tools makes me a bit sad as it's way more than what we need.

@vbatts
Member
vbatts commented Sep 20, 2016

On 20/09/16 05:53 -0700, George Lestaris wrote:

I am very up for sharing a single unpacking implementation! I am not sure if this implementation should live outside of an opencontainers repository though. I would hope to see the image-tools becoming more of community maintained reference implementation in unpacking single layers.

This is the intention of having this repo in the opencontainers org.

Also, having to pull the whole containers/storage and its dependencies in image-tools makes me a bit sad as it's way more than what we need.

perhaps there is a way to trim down that vendoring, such that only the
package used is pulled in?

@glestaris
Contributor

@vbatts let me try to become more specific. Once this PR is accepted we will be using github.com/containers/storage (or a subset of that) to make a changeset (layer). This will lead us in a situation where building a layer is done through a github.com/containers/storage implementation and unpacking a layer is done through the unpack method that currently lives in image/manifest.go.

This is unfortunately asymmetric. So I am worried that the natural next step would be to use github.com/containers/storage to deal with unpacking as well.

I may be overthinking this but it feels strange to see the layer creation be implemented in a different place than the layer unpacking.

@wking
Contributor
wking commented Sep 20, 2016

On Tue, Sep 20, 2016 at 09:53:40AM -0700, George Lestaris wrote:

This is unfortunately asymmetric. So I am worried that the natural
next step would be to use github.com/containers/storage to deal
with unpacking as well.

I don't think it really matters where the reference implementation
lives, as long as we're not pulling in lots of unrelated stuff. The
test suites on the other hand, should be defined locally, so we can
distinguish between “the tested (un)packer is buggy” and
“containers/storage is buggy” when certifying third-party (un)packers.

I may be overthinking this but it feels strange to see the layer
creation be implemented in a different place than the layer
unpacking.

I agree.

@coolljt0725
Contributor

@glestaris I'm agree what you said, I working on implementing the packing without vendoring third-party package

@coolljt0725
Contributor

update without vendorring third-party package except logrus, most code are from docker/docker/pkg/archive, it is well tested.

image/change.go
@@ -0,0 +1,283 @@
+// Copyright 2016 The Linux Foundation
@wking
wking Sep 30, 2016 Contributor

We probably want to revisit this header, especially if we're going to be copying large chunks of files from places like docker/docker's pkg/archive/changes.go where the Linux Foundation is clearly not the primary copyright holder. How about:

Copyright 2016 The Linux Foundation and contributors

with the emphasis for this file being on “and contributors” ;).

And I'm still not wild about either forking these packages (like you're doing here) or vendoring huge external dependencies (like you were doing before). Is there really no interest inside Docker for moving this package out into a stand-alone docker/archive repository? Or in handing that off to us to develop as opencontainers/archive? I don't see docker/docker including github.com/opencontainers/image-tools/image to get this functionality back, but the other two solutions sound like they have a chance.

@vbatts
Member
vbatts commented Oct 6, 2016

please rebase and address the golint reportings

coolljt0725 added some commits Sep 30, 2016
@coolljt0725 coolljt0725 Vendor Sirupsen
Signed-off-by: Lei Jitang <leijitang@huawei.com>
eb10149
@coolljt0725 coolljt0725 Add create layer cmd to create filesystem changeset
Signed-off-by: Lei Jitang <leijitang@huawei.com>
1c001af
@coolljt0725 coolljt0725 Fix golint warnings
Signed-off-by: Lei Jitang <leijitang@huawei.com>
953699f
@coolljt0725 coolljt0725 Use pointer to avoid golint warning of copy lock
Signed-off-by: Lei Jitang <leijitang@huawei.com>
593085b
@coolljt0725 coolljt0725 Add create layer test
Signed-off-by: Lei Jitang <leijitang@huawei.com>
f95665c
@coolljt0725 coolljt0725 Fix golint warnings
Signed-off-by: Lei Jitang <leijitang@huawei.com>
66e6cc3
@coolljt0725 coolljt0725 Move changes.go and changes_unix.go to utils
Signed-off-by: Lei Jitang <leijitang@huawei.com>
c3071ab
@coolljt0725 coolljt0725 Add oci-create-layer man pages
Signed-off-by: Lei Jitang <leijitang@huawei.com>
a694c4e
+**oci-create-layer** [child] [parent] [flags]
+
+# DESCRIPTION
+`oci-create-layer` creates a filesystem changeset from two layers. It compares child with parent and generates a filsystem diff, pack the diff into a uncompressed tar archive. The output tar archive name is the child name with .tar suffix by default, use `--dest` to specify a custom one.
@wking
wking Oct 13, 2016 Contributor

Can we write to stdout by default? Then with #5 you could:

$ oci-create-layer child-dir parent-dir | gzip | oci-cas put image-layout-dir

to push a layer into CAS without landing the uncompressed layer tarball on the disk.

@coolljt0725
coolljt0725 Oct 14, 2016 Contributor

@wking How about write to the stdout by default and use --dest to specify a custom destination?

@wking
wking Oct 14, 2016 Contributor

Yeah, that sounds good to me. Although I think --output is a more popular name for this sort of thing than --dest.

@wking wking referenced this pull request in opencontainers/image-spec Oct 19, 2016
Open

Build story: producing an OCI image #396

image/layer.go
+
+// CreateLayer cretes filesystem changset from child and parent
+func CreateLayer(child, parent, dest string) error {
+ arch, err := Diff(child, parent)
@cyphar
cyphar Oct 19, 2016 Member

I would somewhat like to use @vbatts' go-mtree here. To be fair, I've been slacking on the required PRs to implement it, but would you be open to switching to go-mtree once it's up to snuff? The main benefit is that it has far more explicit design constraints about reproducibility and manifest verifiability than pkg/archive.

@coolljt0725
coolljt0725 Oct 19, 2016 Contributor

@cyphar The main purpose to use pkg/archive is it's well tested and have been used for a long time on docker. I'm personally ok to try to switch to go-mtree once it's up to snuff, but others may have different thoughts.

@cyphar
cyphar Oct 24, 2016 Member

Another benefit of go-mtree that I forgot to mention is that you could make it possible to create layers without needing to keep two copies of the tree -- since mtree generates a manifest that you can compare against. So you could do something like:

% go-mtree -c -K sha256sum -p rootfs/ > before.mtree
% # modify the rootfs
% oci-image-tool --base before.mtree rootfs > layer.tar
@coolljt0725 coolljt0725 Output to stdout by default
Signed-off-by: Lei Jitang <leijitang@huawei.com>
8f43e7a
@cyphar
Member
cyphar commented Oct 25, 2016

I've been playing with this for a while, it works pretty nice (especially with the CAS PR). I'm going to write a PR after this is merged to use go-mtree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment