chunked: support converting existing images

[giuseppe]

if the "convert_images" option is set in the configuration file, then convert traditional images to the chunked format on the fly.

This is very expensive at the moment since the entire zstd:chunked file is created and then processed.

[rhatdan]

@nalind @mtrmac @vrothberg PTAL

[mtrmac]

What does this do, really? I don’t understand what “convert traditional images” means; GetDiffer is used on the “apply a remote layer to local c/storage” path AFAIK.

Best I can tell this creates a “staging directory” for ApplyDiffFromStagingDirectory from non-chunked data. Why is that useful?

To the extent that this creates a zstd:chunked-formatted tar file from some other tar file, how does this differ from skopeo copy --dest-compress-format=zstd:chunked? In the extreme, even a skopeo copy containers-storage:… containers-storage:… ?

[giuseppe]

What does this do, really? I don’t understand what “convert traditional images” means; GetDiffer is used on the “apply a remote layer to local c/storage” path AFAIK.

the current code in chunked handles only layers that are either in zstd:chunked or eStargz format, so that each file can be retrieved individually.

The current PR instead enables pkg/chunked to handle traditional images that are not in these formats. Of course, in this case, we won't be able to pull only the missing files but we still need to retrieve the entire layer. On the other hand, we can now use these files for host deduplication, and more importantly, use them with composefs that use a different format to store the files (a CAS where the file name is its checksum).

It could be implemented in a more performant way where we pull the layer and handle directly the tarball stream, instead, the current implementation first converts the layer to the zstd:chunked format and then use the local copy with the existing code path.

This is different than using skopeo copy since the conversion is just an internal detail

[mtrmac]

So this is, a very likely completely temporary way to get data in a convenient format for creating composefs layers, until an implementation building the relevant file structure directly is built? And composefs images are going to be built via ApplyDiffFromStagingDirectory or something very similar?

[giuseppe]

And composefs images are going to be built via ApplyDiffFromStagingDirectory or something very similar?

yes, the composefs images are built from ApplyDiffFromStagingDirectory

[mtrmac]

(A fairly brief look I’m afraid), code looks reasonable.

I’m a bit concerned about the code structure / lifetimes (how conversion is not done in GetDiffer, how many fields in chunkedDiffer are set to values that refer to a temporary file that will be gone by the time ApplyDiff returns. Some of that might be inevitable if we don’t want to break external API, and I didn’t really check whether the internal data structures in this file could be restructured.

[mtrmac]

Do I understand correctly that the conversion is deferred to ApplyDiff primarily because the object returned by GetDiffer does not have a Close method that could be used to clean up?

But then… why set this to false if fileSource only lives for the duration of this function? If ApplyDiff is ever called again (which… should not happen?), the conversion will need to be done again, won’t it?

---

I’d prefer sharing more code with makeZstdChunkedDiffer, if this PR is not explicitly intended as a very short-term hack that will be completely reimplemented in a short order.

[giuseppe]

I've dropped the c.convertToZstdChunked = false as it is misleading, ApplyDiff is not called recursively.

The conversion is deferred to ApplyDiff mostly because GetDiffer doesn't have access to the staging directory for the layer yet. In this way, we can write the temporary tar file there

[mtrmac]

Thanks! LGTM, with the caveat that I haven’t kept up with the other changes over the last month, I’m afraid.

[giuseppe]

@rhatdan fine to merge?

[rhatdan]

LGTM