ostree: support it as an image source and drop any usage of the ostree CLI tool #360

giuseppe · 2017-10-13T15:19:42Z

No description provided.

mtrmac · 2017-10-13T16:27:12Z

ostree/ostree_src.go

+		return 0, err
+	}
+
+	size, err := strconv.ParseInt(string(out[1:len(out)-2]), 10, 64)


The 1:len(out)-2 is pretty magic, and fragile. Could this be expressed in safer terms, perhaps splitting trailing '\n' and white space?

(Applies to all instances of that.)

(I guess this would go away with using the bindings directly.)

yes that would go away with the bindings. Anyway, I agree it is quite hard to follow so I've pushed a new version with this part cleaned up. The out string is the output of g_variant_print from OSTree plus a \n. With this code I drop the quotes around the JSON file plus the final \n.

In the new version, I use g_variant_parse so that we don't have to deal how it is quoted.

mtrmac · 2017-10-13T16:29:45Z

ostree/ostree_src.go

+	if err != nil {
+		return nil, "", err
+	}
+	m := []byte(strings.Replace(string(out[1:len(out)-2]), "\\n", "\n", -1))


Is the strings.Replace safe? This seems ambiguous. What if the manifest contains "ignoredKey": "Foo\nBar", will it be corrupted?

(Again, perhaps will go away with the golfing bindings.)

this is also fixed with g_variant_parse.

mtrmac · 2017-10-13T16:30:29Z

ostree/ostree_src.go

+}
+
+func (s *ostreeImageSource) GetTargetManifest(digest digest.Digest) ([]byte, string, error) {
+	return nil, imgspecv1.MediaTypeImageManifest, nil


This should probably fail with a readable error message instead.

(Still outstanding.)

mtrmac · 2017-10-13T16:31:58Z

ostree/ostree_src.go

+
+// GetBlob returns a stream for the specified blob, and the blob's size.
+func (s *ostreeImageSource) GetBlob(info types.BlobInfo) (io.ReadCloser, int64, error) {
+	blob := strings.Replace(string(info.Digest), "sha256:", "", 1)


Should this be info.Digest.Hex()? Or is any non-sha256 digest supposed to be preserved including the prefix?

yes, fixed now.

mtrmac · 2017-10-13T16:33:54Z

ostree/ostree_src.go

+			return nil, 0, err
+		}
+		return ioutils.NewReadCloserWrapper(stream, func() error {
+			return os.RemoveAll(dir)


All the error paths also need to remove dir.

(This would need less repetition with a defer func() {if !succeeded { os.RemoveAll(dir) } }() or so. This still works I guess.)

giuseppe · 2017-10-23T08:39:59Z

I've dropped the WIP and addressed the comments

mtrmac · 2017-10-24T17:27:00Z

ostree/ostree_dest.go

 	"github.com/pkg/errors"

 	"github.com/ostreedev/ostree-go/pkg/otbuiltin"
+


(Nit: Please keep the imports organized in two sections, imports from the default Golang library, and all external imports.)

mtrmac · 2017-10-24T17:33:46Z

ostree/ostree_src.go

+	cstring := C.CString(in)
+	defer C.free(unsafe.Pointer(cstring))
+
+	ptr := (*C.char)(C.g_variant_get_string(C.g_variant_parse(nil, (*C.gchar)(cstring), nil, nil, nil), nil))


Doesn’t g_variant_unref need to be called?

mtrmac · 2017-10-24T17:36:27Z

ostree/ostree_src.go

+}
+
+func (s *ostreeImageSource) GetTargetManifest(digest digest.Digest) ([]byte, string, error) {
+	return nil, imgspecv1.MediaTypeImageManifest, nil


(Still outstanding.)

mtrmac · 2017-10-24T17:38:26Z

ostree/ostree_src.go

+			return nil, 0, err
+		}
+		return ioutils.NewReadCloserWrapper(stream, func() error {
+			return os.RemoveAll(dir)


(This would need less repetition with a defer func() {if !succeeded { os.RemoveAll(dir) } }() or so. This still works I guess.)

mtrmac

Apart from a few nits, the attempt to re-create compressed blobs is very problematic. I’m sorry I haven’t noticed that earlier.

mtrmac · 2017-10-24T18:07:32Z

ostree/ostree_src.go

+	go func() {
+		io.Copy(gzipWriter, ots)
+		gzipWriter.Close()
+		pipeWriter.Close()


Hum, trying to re-create a compressed blob by re-creating the uncompressed tarstream and compressing it doesn’t work long-term and is not supportable. (We did try to do this earlier, for containers-storage, IIRC; it failed after a few months.) The gzip bitstream is not stable over Golang versions, meaning that this will not be able to reproduce the compressed blobs of images compressed by earlier Golang versions, and after any future Golang upgrade a previously working GetBlob can start returning different data; in either case the blob won’t match the digest recorded in the manifest.

For me, this inability to handle compressed data is by no means a blocker for this PR; containers-storage has the same problem, and that will need to be solved by editing the manifest. See #305 and the conversations in there.

What to do for now, though? I would be fine with this PR just skipping the compression step, meaning that the ostree: source produces usable images only for originally uncompressed images, and dealing with the compression as a follow-up.

That follow-up can probably be built around the tentative c/i/manifest subpackage being explored in #305.

Cc: @nalind

it is something that is biting us also when we import layers from the local Docker engine, as we compute the checksum using whatever gzip implementation is used by the golang version used to build https://github.com/projectatomic/atomic/blob/master/dockertar-sha256-helper.go. In anycase, we should probably use skopeo copy in atomic for accessing the local Docker engine.

mtrmac · 2017-10-24T18:11:34Z

ostree/ostree_src.go

+
+// newImageSource returns an ImageSource for reading from an existing directory.
+func newImageSource(ctx *types.SystemContext, tmpDir string, ref ostreeReference) (types.ImageSource, error) {
+	return &ostreeImageSource{ref: ref, tmpDir: tmpDir}, nil


(Does the tmpDir need to be user-specifiable? In #228, OSTreeTmpDirPath was introduced to affect the SELinux labels of the committed files; here the directory is used only to temporarily store a tarball, which AFAICT does not need any particular SELinux label, as long as this process can read and write the file.

Non-blocking, this does work fine.)

yes true. As you say, the tmpDir used should not affect the result. Also, in the long term, I'd like that the golang bindings offer some nice ways to browse/request files from the ostree repository without extracting them to a temporary directory first

giuseppe · 2017-11-07T16:44:08Z

@mtrmac I've just rebased the PR

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Use the tar-split information to recreate the original tarball. It is not the ideal implementation yet, we will need to implement more stuff in the OSTree golang bindings before we can improve it. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2017-11-21T11:03:11Z

@mtrmac thanks for the detailed review. I've addressed the comments in the version here ⬆️

mtrmac

Please drop the gzipWriter and return the output of tar-split unmodified.

The GError memory handling, while now consistent, seems to be designed to be used differently.

mtrmac · 2017-11-21T15:36:34Z

ostree/ostree_src.go

+}
+
+func (o ostreePathFileGetter) Close() {
+	C.g_object_ref(C.gpointer(o.repo))


This should be g_object_unref

mtrmac · 2017-11-21T15:38:07Z

ostree/ostree_src.go

+}
+
+// newImageSource returns an ImageSource for reading from an existing directory.
+func newImageSource(ctx *types.SystemContext, tmpDir string, ref ostreeReference) (types.ImageSource, error) {


(ctx is not used and could be dropped. Non-blocking.)

giuseppe · 2017-11-21T16:51:09Z

@mtrmac if I simply drop the gzipWriter then the digest doesn't match:

FATA[0011] Error writing blob: Digest did not match, expected sha256:76b0264c130a031b05e695bf175147575b93271c9c838408ba63daaa7de934e9, got sha256:f4c74f205954fed1127474d1d58614639a0d560fe3bab9c9c1abd51ad7426fe7

is there anything more to do to get it working with the output from tar-split?

mtrmac · 2017-11-21T16:53:45Z

@mtrmac if I simply drop the gzipWriter then the digest doesn't match:

It “in principle” does not match with the gzipWriter either; it just happens to work for your particular combination of golang version and source image:

It is likely enough to work for a month or so, enough for users to build features on top and assume everything is fine, and break in a few golang RPM upgrades.

I think I’d rather have a source which transparently always fails to reproduce compressed blobs than one that works long enough for users to rely on it when we know we can’t support this long-term. (@runcom WDYT?)

The work in #305 should make this possible one way or another; until then, I’d prefer not even trying here (but @runcom can persuade me otherwise).

giuseppe · 2017-11-21T17:25:22Z

can this be changed once #305 is merged? It is really unfortunate that the checksum is calculated on the compressed file, we are already using this assumption in atomic

mtrmac · 2017-11-21T17:27:47Z

It doesn’t work. Whatever you are building that relies on this recompression doesn’t work reliably (and after #305 happens still will neither return compressed data nor return the original checksum).

giuseppe · 2017-11-21T18:00:49Z

it doesn't depend on the compressed data, as long as we will be able to provide a modified manifest file that will match the uncompressed data.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

It could be missing in the manifest Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Take rid of any usage of the OSTree CLI tool and use directly the ostree C library. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

system containers used to support this format. Let's include it, since the required changes are minimal. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

the previous implementation relied on the golang gzip implementation to generate always the same stream and thus checksum. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2017-11-22T07:03:06Z

I've added another patch to handle only uncompressed layers for now. When #305 will be merged, then I'll work on supporting compressed layers as well.

runcom · 2017-11-23T10:59:11Z

The work in #305 should make this possible one way or another; until then, I’d prefer not even trying here (but @runcom can persuade me otherwise).

well, #305 is needed by CRI-O as well, so it won't be that long till we merge that hopefully. That PR is actually fixing 2 bugs in CRI-O, meanwhile, I don't have any issue with this PR as long as when #305 is merged, this is reworked to take into account what's missing.

@mtrmac sounds good?

giuseppe · 2017-11-23T12:57:12Z

@mtrmac are you still fine if I drop the "ostree: support only uncompressed images for now" patch or is it necessary to get this PR approved?

mtrmac · 2017-11-23T18:29:01Z

I’d still prefer the compression not to be included; I can’t see that making an incorrect impression that this works is valuable at all, and the post-#305 work will not benefit from the compression either. But, *shrug* if @runcom is fine with having that in, I won’t block on that.

However you decide, please let me know when this is ready for merging.

giuseppe · 2017-11-23T19:28:17Z

ok then, let's merge without the support for compressed images for now. I will take another look if and how it can be supported after #305 is merged. Thanks again for the reviews

mtrmac · 2017-11-23T19:30:07Z

👍

Thanks!

giuseppe · 2017-11-23T20:48:10Z

do I need to re-vendor Skopeo or is it going to happen anyway before a new release?

mtrmac · 2017-11-23T21:23:04Z

It is very likely to happen, but we don’t have a process that absolutely guarantees it. If you want to be sure and submit a skopeo PR (just run vndr and commit), go ahead.

giuseppe force-pushed the ostree-store-tarsplit-info branch 2 times, most recently from 7a7aeca to c65fed6 Compare October 13, 2017 16:07

mtrmac reviewed Oct 13, 2017

View reviewed changes

giuseppe force-pushed the ostree-store-tarsplit-info branch 6 times, most recently from f3a5ff9 to 3d85d7e Compare October 16, 2017 08:45

giuseppe changed the title ~~[WIP] ostree: support it as an image source~~ ostree: support it as an image source Oct 16, 2017

mtrmac reviewed Oct 24, 2017

View reviewed changes

giuseppe force-pushed the ostree-store-tarsplit-info branch 2 times, most recently from 2f78255 to 23e6ccd Compare October 28, 2017 11:01

giuseppe force-pushed the ostree-store-tarsplit-info branch 5 times, most recently from 86c852f to 37e6869 Compare November 7, 2017 16:18

giuseppe force-pushed the ostree-store-tarsplit-info branch 3 times, most recently from 8e15431 to 3db3651 Compare November 8, 2017 12:30

giuseppe changed the title ~~ostree: support it as an image source~~ [WIP DONOTMERGE] ostree: support it as an image source Nov 9, 2017

giuseppe force-pushed the ostree-store-tarsplit-info branch from 6e3e7ab to 86aa36f Compare November 9, 2017 11:34

giuseppe changed the title ~~[WIP DONOTMERGE] ostree: support it as an image source~~ ostree: support it as an image source and drop any usage of the ostree CLI tool Nov 9, 2017

giuseppe force-pushed the ostree-store-tarsplit-info branch 3 times, most recently from a40100e to e7b1b04 Compare November 9, 2017 16:51

giuseppe added 3 commits November 21, 2017 11:32

ostree: use golang binding for committing the config file

274296a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: store the tar-split output in the metadata

20f2ac6

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: support ostree as a source

86bada7

Use the tar-split information to recreate the original tarball. It is not the ideal implementation yet, we will need to implement more stuff in the OSTree golang bindings before we can improve it. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the ostree-store-tarsplit-info branch from 1796a08 to afa60a0 Compare November 21, 2017 10:40

mtrmac reviewed Nov 21, 2017

View reviewed changes

giuseppe force-pushed the ostree-store-tarsplit-info branch from afa60a0 to 5363311 Compare November 21, 2017 23:48

giuseppe added 8 commits November 22, 2017 07:57

ostree, source: copy files directly from the storage

97f94aa

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: support copy of signatures

ac74201

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: fix typo

30e2804

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: do not access the config checksum

8c2fb37

It could be missing in the manifest Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: remove any usage of the ostree CLI tool

48c1877

Take rid of any usage of the OSTree CLI tool and use directly the ostree C library. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: support v1 manifests

555790e

system containers used to support this format. Let's include it, since the required changes are minimal. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: fix imports style

71c7fde

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ostree: support only uncompressed images for now

c809f3d

the previous implementation relied on the golang gzip implementation to generate always the same stream and thus checksum. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the ostree-store-tarsplit-info branch from b8b40e1 to c809f3d Compare November 22, 2017 06:59

mtrmac merged commit 717f39c into containers:master Nov 23, 2017

		"github.com/pkg/errors"

		"github.com/ostreedev/ostree-go/pkg/otbuiltin"

ostree: support it as an image source and drop any usage of the ostree CLI tool #360

ostree: support it as an image source and drop any usage of the ostree CLI tool #360

Uh oh!

Conversation

giuseppe commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Nov 7, 2017

Uh oh!

giuseppe commented Nov 21, 2017

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Nov 21, 2017

Uh oh!

mtrmac commented Nov 21, 2017

Uh oh!

giuseppe commented Nov 21, 2017

Uh oh!

mtrmac commented Nov 21, 2017

Uh oh!

giuseppe commented Nov 21, 2017

Uh oh!

giuseppe commented Nov 22, 2017

Uh oh!

runcom commented Nov 23, 2017

Uh oh!

giuseppe commented Nov 23, 2017

Uh oh!

mtrmac commented Nov 23, 2017

Uh oh!

giuseppe commented Nov 23, 2017

Uh oh!

mtrmac commented Nov 23, 2017 •

edited by runcom

Loading