Workaround assumption that layers are unique in zstdchunk converter

[apostasie]

Hey,

(Hopefully) fixes #1842 and fixes containerd/nerdctl#3623

zstdchunk convert function assumes that layers are unique and cannot be repeated.

stargz-snapshotter/nativeconverter/zstdchunked/zstdchunked.go

Lines 112 to 116 in a6b9bdb

	defer func() {
	if err := cs.Delete(ctx, uncompressedDesc.Digest); err != nil {
	log.G(ctx).WithError(err).WithField("uncompressedDesc", uncompressedDesc).Warn("failed to remove tmp uncompressed layer")
	}
	}()

When this is not the case, the defer delete of the uncompressed version will remove it while other competing routines may still need it, leading to the conversion failing.

This PR offers one possible solution that is minimally intrusive and does not change the overall design - just use a mutex when uncompressing a specific layer to ensure there cannot be concurrent processing for the same desc.

Note that if the layers are already uncompressed, no locking is enforced, hence the same class of issue may manifest itself elsewhere in the code (this PR does not try to address that - and it is unclear if such a situation is a problem or not).

Also PR-ed on nerdctl - containerd/nerdctl#3628 - should be closed over there if this here can be merged and released.

Finally, there may be more issues involved with large images like the ones in the initial reports - and this PR might not be a silver bullet, though it did successfully convert the nvidia image locally.

Thanks.

cc @AkihiroSuda @ktock

[GrigoryEvko]

Hi @apostasie did you check this PR with ctr-remote command for example? Unfortunately this change works for me in nerdctl and doesn't in ctr-remote image optimize :(

[apostasie]

Hey @GrigoryEvko

Yep.
You have to pass --no-optimize to ctr-remote.

If you do not specify it, because of the extra opts from Analyze, the converter function can no longer acquire a lock and the calls are again parallelized.

Clearly there are deep-seated issues in the codebase making assumptions about images that are not true:
https://github.com/containerd/stargz-snapshotter/blob/main/nativeconverter/zstdchunked/zstdchunked.go#L82-L83
(developer's famous last words: "this should not happen" :-))

Bottom-line:

With this patch, use nerdctl, or use ctr-remote with --no-optimize.

If the maintainers here find this direction acceptable, we can also patch LayerConvertWithLayerOptsFuncWithCompressionLevel, but I feel more and more we have to rethink the entire thing.

None of this appears to be safe to use concurrently (IMO the uncompress/defer delete step on the content store is problematic).

[GrigoryEvko]

You have to pass --no-optimize to ctr-remote.

Oh no, it was the whole point of using optimize command 😄

For today, if I'd like to have optimized zstdchunked image, would ctr-remote image optimize to estargz and then nerdctl image convert to zstdchunked work? I'm not sure about all the metadata, would it be saved and relative after the conversion?

It's interesting that converting to estargz with gzip doesn't have all these issues, I thought it's mostly a compression algorithm change, looks like it's implemented differently.

Thanks again!

[apostasie]

Oh, stop making these puppy eyes sir! 😂

Here.

Latest version works for both (note that I hate the updated version even more than the previous one, because of the global vars).

Enjoy ;-).

[apostasie]

Afk for most of the day - will be slow to answer.

[apostasie]

@AkihiroSuda @ktock gentle nudge

[ktock]

Sorry for slow review. I think we can just remove cs.Delete(ctx, uncompressedDesc.Digest) in this function. containerd garbage collects that blob when nobody refers to that.