Vendor containers/{image,storage} for faster compression and parallel pulls

[vrothberg]

No description provided.

[runcom]

This works in openshift/builder 👍

[giuseppe]

LGTM

[vrothberg]

Very odd errors in Travis: # time="2018-12-19T11:09:53Z" level=debug msg="error parsing image name \"localhost/alpine\" as given, trying with transport \"docker://\": Invalid image name \"localhost/alpine\", expected colon-separated transport:reference"

[rhatdan]

Looks like a different change is not handling localhost correctly.

[vrothberg]

It's a regression in c/image. We're on it :)

[vrothberg]

Looks like a different change is not handling localhost correctly.

I think you're right. I can reproduce the issue on a vanilla master locally.

[vrothberg]

The localhost warnings/errors are red herrings. It actually comes from the progress bars writing to stdout which will then be read by the scripts and interpreted as IDs. Will tackle this issue tomorrow.

[rhatdan]

Can we just tell the output to be quiet?

[vrothberg]

I don’t think that’s user friendly as the progress won’t be shown. I’ll tackle it tomorrow and will add a test to Skopeo catching such regressions.

…

On Wed 19. Dec 2018 at 18:20, Daniel J Walsh ***@***.***> wrote: Can we just tell the output to be quiet? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1244 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALI4g-KCGuJlvz2vD9UC98w1YZooNTZfks5u6nVFgaJpZM4ZZ9BF> .

[rh-atomic-bot]

☔ The latest upstream changes (presumably 4674656) made this pull request unmergeable. Please resolve the merge conflicts.

[vrothberg]

@rhatdan @giuseppe ... CI should pass now. Please, take a look at c46e256. The pgzip library changed a bit the blobcache behaviour and I would like some other pairs of eyes on it.

[rhatdan]

LGTM,
But I think we need @mtrmac and @nalind to agree to your changes on the blob cache.

[vrothberg]

LGTM,
But I think we need @mtrmac and @nalind to agree to your changes on the blob cache.

Sure! Once this PR is merged, we can update Podman as well.

[TomSweeneyRedHat]

LGTM, but would like a head nod from @mtrmac or @nalind

[giuseppe]

same from me, LGTM beside the change in the tests

[vrothberg]

Alright ... this is to be expected and actually @vbatts has warned us about this. The compressed objects are now different on which we're computing the checksums. It still seems to work in some cases but I need to digg deeper in the code/

[rhatdan]

@vrothberg @mtrmac @nalind What is the verdict on this one?

[vrothberg]

Alright. I've manually reproduced the tests before this change and with this change. To me, everything looks fine.

In a simplified way, the tests in question first pull-and-build (with cache) to cache, then push (without cache) to dir:no-cache and then push (with cache) to dir:with-cache. After that, the test compares the contents of cache no-cache and with-cache and searches for matching files.

After the change, the tests were complaining because the base layer (i.e., busybox) was not matching between cache and no-cache which makes sense as the layer in cache was compressed with pgzip while the layer in no-cache is the one we pulled which was compressed differently. In other words, the test was just working before this change because the layers we pulled were compressed with the very same library Buildah was using. In yet other words, we would hit this issue sooner or later once the gzip compression of the stdlib is changed (which is happening). Or in the words of @vbatts: it was a mistake computing digests on compressed objects.

I have a screenshot with 8 tmux tiles that I can use to explain this situation in case you want to verify my above claims.

What I suggest to do is to remove the no-cache comparisons from the tests as those are prone to errors. It was really painful to reproduce 😿

[vrothberg]

I updated the tests (see d88807e).

[runcom]

What I suggest to do is to remove the no-cache comparisons from the tests as those are prone to errors. It was really painful to reproduce

@mtrmac could you ack 👼

[rhatdan]

@vrothberg SGTM

[vrothberg]

Three Travis jobs hit the timeout (now restarted).

[mtrmac]

In a simplified way, the tests in question first pull-and-build (with cache) to cache, then push (without cache) to dir:no-cache and then push (with cache) to dir:with-cache. After that, the test compares the contents of cache no-cache and with-cache and searches for matching files.

Right

After the change, the tests were complaining because the base layer (i.e., busybox) was not matching between cache and no-cache which makes sense as the layer in cache was compressed with pgzip while the layer in no-cache is the one we pulled which was compressed differently.

The other way around AFAICS: The cache contains the original compressed version (and a decompressed version); whereas buildah push without using the cache compresses the data written to dir:no-cache with pgzip.

In other words, the test was just working before this change because the layers we pulled were compressed with the very same library Buildah was using. In yet other words, we would hit this issue sooner or later once the gzip compression of the stdlib is changed (which is happening).

Yes.

What I suggest to do is to remove the no-cache comparisons from the tests as those are prone to errors. It was really painful to reproduce 😿

Yes, for now.

So, LGTM.

---

Ultimately, though, the data flow in buildah would really benefit from a closer [or is that more high-level?] look, quite apart from the blobcache: AFAICS the simplest buildah {commit,bud}, by default:

• In containerImageRef.NewImageSource:
  • Exports the top layer from c/storage as a tar file stream
  • Compresses it on the fly
  • Writes the compressed stream to a temporary file
• In containerImageSource.GetBlob:
  • Opens the file with the compressed data
• In storageImageDestination.PutBlob:
  • Decompresses that data on the fly
  • Writes the decompressed result into another temporary file
• In storageImageDestination.Commit:
  • Opens the file with the decompressed data
  • Creates a c/storage layer from it
• For good measure, a manifest that actually refers to the compressed blobs is created and recorded as well with the newly created image.

I don’t understand c/storage enough to know whether buildah commit can logically be just a “tag” operation that turns the modifiable top layer of a container into an immutable layer in a cheap operation or whether ultimately the export of the container writable layer as tar + import into a new immutable layer is necessary — but the compression+decompression seems clearly unnecessary, and may be very expensive (in my bechmarks, though admittedly on a laptop and not a cloud node, the CPU cost of compression was by fast the slowest part with dealing with large layers).

[rhatdan]

Lets bring up the rest of the discussion with @nalind When he gets back.
@rh-atomic-bot r+

[rh-atomic-bot]

📌 Commit 759f40b has been approved by rhatdan

[rhatdan]

@rh-atomic-bot retry

[vrothberg]

The bot seems to have a cold :^)

[mrunalp]

We want this vendored into openshift/builder next.