cc @ktock

Thanks for this work!
LGTM

Thanks for the work! nydus-snapshotter can be upgraded to v0.5.0, others LGTM.

If we were to instead support a user-specified containerd snapshotter, wouldn't we get similar benefits but without pulling in additional dependencies / code surface we have to maintain over time?

In other words, what specific benefits do we gain by explicitly supporting (and thus maintaining over time) these directly instead of supporting them implicitly via containerd's existing support?

In other words, what specific benefits do we gain by explicitly supporting (and thus maintaining over time) these directly instead of supporting them implicitly via containerd's existing support?

Also curious about that part; if possible it would be good that we wouldn't have to make changes for every possible snapshotter out there. Not sure what exactly is needed on our side for that. Otherwise we'd continue to play catch-up every time a new snapshotter comes to existence/

In other words, what specific benefits do we gain by explicitly supporting (and thus maintaining over time) these directly instead of supporting them implicitly via containerd's existing support?

Also curious about that part; if possible it would be good that we wouldn't have to make changes for every possible snapshotter out there. Not sure what exactly is needed on our side for that. Otherwise we'd continue to play catch-up every time a new snapshotter comes to existence/

The lazy snapshotters need the image handlers, they add metadata so that the snapshotter knows later what’s what and be able to pull the layers on demand.

A user-defined snapshotter will only work if that snapshotter doesn’t need any custom image handlers on pull

I only added the snapshotters that are subprojects of the containerd project, I don’t think there will be new ones popping up every week.

In other words, what specific benefits do we gain by explicitly supporting (and thus maintaining over time) these directly instead of supporting them implicitly via containerd's existing support?

To rephrase my other comment: without this code these two snapshotters don’t work at all, containerd’s client doesn’t support them by default. Maybe this will change in the future with the Transport service they are making but I’m not sure

I only added the snapshotters that are subprojects of the containerd project, I don’t think there will be new ones popping up every week.

Right, but the bar for becoming a non-core project is fairly low, so even if it doesn't happen often, it also doesn't really tell us anything about the support or longevity of the project (and thus whether it makes sense for us to commit to supporting; see the saga around several of our log drivers for a painful example of this 😔).

The snapshotter-specific code just appends labels, so it is easier to maintain compared to logging drivers.

If this is still a burden to get the PR merged, I guess we can move the snapshot-specific labeling part to daemon.json values.

The snapshotter-specific code just appends labels, so it is easier to maintain compared to logging drivers.

If this is still a burden to get the PR merged, I guess we can move the snapshot-specific labeling part to daemon.json values.

If these are well-known (fixed) labels, perhaps they can be set without having to introduce the dependencies (see docker/buildx#1549 (comment) for a similar discussion). Configurable through daemon.json (or other ways) could be an option yes (worth exploring).

FWIW; if we only need the dependency for the labels, I'm not so much concerned about the dependency itself (which would then be mostly "dead code" (still not ideal)), but also the ripple effect those dependencies may cause (indirect dependencies, versions of those), which becomes more problematic if we're an actual go module.

The snapshotter-specific code just appends labels, so it is easier to maintain compared to logging drivers.
If this is still a burden to get the PR merged, I guess we can move the snapshot-specific labeling part to daemon.json values.

If these are well-known (fixed) labels, perhaps they can be set without having to introduce the dependencies (see docker/buildx#1549 (comment) for a similar discussion). Configurable through daemon.json (or other ways) could be an option yes (worth exploring).

The labels these two snapshotters set don't seem to be well-known, I'm not sure we want to implement a whole logic around adding different labels to different media-types with specific values. This would really mean we are maintaining lazy snapshotters but in a completely different way the original snapshotter does.

FWIW; if we only need the dependency for the labels, I'm not so much concerned about the dependency itself (which would then be mostly "dead code" (still not ideal)), but also the ripple effect those dependencies may cause (indirect dependencies, versions of those), which becomes more problematic if we're an actual go module.

FYI we already had stargz-snapshotter as a dependency because buildkit handles it and we will also depend on nydus once we update buildkit

It was observed today in the maintainer's call that containerd itself does not import these snapshotters, and supports them just fine. It appears that more-or-less the same code being imported and called here is built into the CRI:

https://github.com/containerd/containerd/blob/dadd203c255756d2208879088f7c61e1bee7d1f8/pkg/cri/sbserver/image_pull.go#L577-L602

Can we see if we can incorporate this code directly, and raise a PR against containerd that hoists it out of the CRI and to an exported package? Then we can add a FIXME to use the newly exported method when we eventually pick up the change in containerd.

@neersighted Thank you for the suggestion! Yes, using appendInfoHandlerWrapper with WithImageHandlerWrapper is enough to integrate remote snapshotters aware of these labels (e.g. stargz-snapshotter and maybe nydus as well cc: @imeoer), without importing each project's handler.

As of now, appendInfoHandlerWrapper isn't an exported function so we can work on it in the following steps:

1. This PR (cc @rumpl): Copy appendInfoHandlerWrapper to moby's codebase and use it. Add FIXME comment to use the function exported by containerd in the following-up patch.
2. Following-up PR at containerd project: Fix containerd to export that function (Under discussion on Merged via Export remote snapshotter label handler containerd/containerd#8036) Now this is available in containerd v1.6.17
3. Following-up PR at moby project: Fix moby's codebase to use the newly exported function when the version of containerd including the above fix is imported to moby.

I'm willing to working on following-up PRs.

cc @tianon @thaJeztah @AkihiroSuda

1. This PR (cc @rumpl): Copy appendInfoHandlerWrapper to moby's codebase and use it. Add FIXME comment to use the function exported by containerd in the following-up patch.

I can do that 👍

Thanks @ktock for the work, we will update nydus snapshotter once containerd/containerd#8036 is merged and released.

nydus snapshotter has integrated containerd/containerd#8036
cc @rumpl Any progress we can push forward together?

I've gone ahead and rebased this in terms of containerd 1.6.18 and the newly exported code; PTAL @vvoland @rumpl

Can't approve my own PR but 👍 LGTM

Bringing this in; and I'm really happy to see how we came together across projects to improve this for the ecosystem 🎉