Make the snapshotter configurable #40

rumpl · 2022-08-04T08:39:05Z

No description provided.

daemon/config/config.go

daemon/oci_linux.go

thaJeztah · 2022-08-04T15:18:54Z

cmd/dockerd/config.go


 	flags.StringVar(&conf.ContainerdNamespace, "containerd-namespace", conf.ContainerdNamespace, "Containerd namespace to use")
 	flags.StringVar(&conf.ContainerdPluginNamespace, "containerd-plugins-namespace", conf.ContainerdPluginNamespace, "Containerd namespace to use for plugins")
+	flags.StringVar(&conf.ContainerdSnapshotter, "containerd-snapshotter", conf.ContainerdSnapshotter, "Containerd snapshotter to use")


Wondering if this should take the place of --storage-driver and/or --storage-opt;

moby/cmd/dockerd/config_unix.go

Line 39 in 2279803

flags.StringVarP(&conf.GraphDriver, "storage-driver", "s", "", "Storage driver to use")

moby/cmd/dockerd/config.go

Line 38 in 2279803

flags.Var(opts.NewNamedListOptsRef("storage-opts", &conf.GraphOptions, nil), "storage-opt", "Storage driver options")

As effectively, they're mutually exclusive (either us containerd-snapshotter, or graph drivers). Default snapshotter could be an option for that 🤔

"s/take the place/if we could use/"

What feels weird is that snapshotters are not storage drivers, I'm not sure why the GraphDriver flag is storage-driver, I would prefer having a flag for containerd rather than using the existing. This would also make it clearer in the future that graph drivers don't exist any more since the daemon won't start (I guess?) if you give it a flag it doesn't know

I'm not sure why the GraphDriver flag is storage-driver,

Right, so originally they were called "graph-driver", which was quite specific to the implementation, so later on it was renamed to storage-driver (which is more descriptive for the purpose). However, when the "visible" terminology was renamed, the internal types and packages weren't; there were some PRs to do so (moby#9758, moby#22228 -> moby#23237), but abandoned, and I guess nobody picked it up again to finish 😞.

What feels weird is that snapshotters are not storage drivers

Not sure; overall, they seem quite the equivalent, with the difference being that "storage drivers" had the problem that the concept of "layers" leaked into them; "snapshotters" made that more generic, and changed it into a more generic concept of "snapshots" (which could be implemented using layers, but also through other ways).

From a user-perspective, I'm inclined to see them as "same purpose", only the snapshotters may be "multi-arch" aware (and other features, but that also applies to storage-drivers; some come with additional features that can be used).

I can bring this up in the maintainers meeting to see what others think

We discussed this in the maintainers meeting;

concensus was that the preference is to re-use the --storage-driver flag, as the term is generic enough, and was originally chosen for that reason (apparently there had been some internal discussion at that time to get rid of the "layers" concept, and early implementations of "snapshotter-like approaches" were already being designed)

for now, we would switch between "containerd snapshotters" and "storage drivers" based on the feature flag that was added (but snapshotter would of course at some point become the default)

if that feature is enabled, we must have some mapping for backward compatibility on (already familiar) names that have an equivalent in containerd, so;

overlay -> io.containerd.snapshotter.v1.overlayfs

overlay2 -> io.containerd.snapshotter.v1.overlayfs (same)

zfs -> io.containerd.snapshotter.v1.zfs

etc.

besides the "shorthand" (overlay2) names, also accept the "fully qualified" names (e.g. io.containerd.snapshotter.v1.overlayfs can be used in the daemon config)

if no explicit snapshotter is defined in the daemon config we can use containerd's default

but, we should consider having a better automatic selection (similar to how we currently select the most suitable snapshotter) instead of containerd's (currently hardcoded) default; this could be by querying (if possible) the list of available snapshotter-plugins that were loaded, and pick the first available one (based on a priority list).

perhaps this could use a contribution to containerd, to make containerd itself also make a similar selection (TBD)

./cc @tonistiigi @cpuguy83 @tianon @neersighted (pls check if I captured the discussion correctly or if I forgot something)

thaJeztah · 2022-08-08T10:11:59Z

Still a bit flaky, this one;

=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIRunSuite/TestRunContainerWithRmFlagCannotStartContainer (12.69s)
    docker_cli_run_test.go:2772: timeout hit after 10s: waiting for container 'sparkles' to be removed
    --- FAIL: TestDockerCLIRunSuite/TestRunContainerWithRmFlagCannotStartContainer (12.69s)

rumpl

Approved :D

thaJeztah · 2022-08-08T14:58:49Z

daemon/containerd/snapshotters.go

+	graphDriver = strings.TrimPrefix(graphDriver, "io.containerd.snapshotter.v1.")
+	return graphDriver


Silly question; does containerd always expect the snapshotter name without the prefix? I was somewhat expecting it to use the "fully qualified" name, but indeed see that containerd.DefaultSnapshotter is just "overlayfs".

Mostly wondering if it would expect a fully-qualified name for non-built-in snapshotters (e.g.)

Yes, it only works with shortname. I tested the stargz snapshotter with the FQN and it fails:
snapshotter not loaded: io.containerd.snapshotter.v1.stargz: invalid argument

Giving this some thought; if containerd itself doesn't accept the fully-qualified name (io.containerd.snapshotter.v1.<foo>), perhaps we should not either (at least for now); we can consider contributing this in upstream, and if that's accepted implement it on our side as well.

That said; if we DO want to support it, perhaps we should use containerd's const for this; https://github.com/containerd/containerd/blob/902212651b12e4d81b7bc779632b2e4da0a8e673/plugin/plugin.go#L65-L66

graphDriver = strings.TrimPrefix(graphDriver, plugin.SnapshotPlugin+".")

daemon/daemon.go

thaJeztah · 2022-08-08T15:16:44Z

daemon/containerd/snapshotters.go

+	if graphDriver == "" {
+		graphDriver = containerd.DefaultSnapshotter
+	}


As we were discussing; we should (for a follow-up) look at;

detecting what snapshotter to use as default (which may depend on the underlying filesystem (zfs, btrfs))

check if we should be storing the default in the containerd namespaces and/or check what default was set in the namespace and take that as default if no custom default was set.

see if we can present the list of available snapshotters (although I'm not sure if we currently present that list in docker info or elsewhere 🤔)

daemon/daemon.go

daemon/containerd/snapshotters.go

daemon/containerd/snapshotters_test.go

thaJeztah · 2022-08-09T08:39:10Z

daemon/containerd/snapshotters.go

+	graphDriver = strings.TrimPrefix(graphDriver, "io.containerd.snapshotter.v1.")
+	return graphDriver


Giving this some thought; if containerd itself doesn't accept the fully-qualified name (io.containerd.snapshotter.v1.<foo>), perhaps we should not either (at least for now); we can consider contributing this in upstream, and if that's accepted implement it on our side as well.

That said; if we DO want to support it, perhaps we should use containerd's const for this; https://github.com/containerd/containerd/blob/902212651b12e4d81b7bc779632b2e4da0a8e673/plugin/plugin.go#L65-L66

graphDriver = strings.TrimPrefix(graphDriver, plugin.SnapshotPlugin+".")

daemon/containerd/snapshotters.go

Signed-off-by: Djordje Lukic <[email protected]>

Also moved some layerStore related initialization to the non-c8d case because otherwise they get treated as a graphdriver plugins. Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Paweł Gronowski <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

vvoland · 2022-08-09T12:54:04Z

Squashed suggestions from #48

thaJeztah

LGTM, thanks!

thaJeztah · 2022-08-09T14:21:41Z

CI is happy; let's get this one in 👍

This was added as part of rumpl#40, but wasn't used. Signed-off-by: Sebastiaan van Stijn <[email protected]>

rumpl commented Aug 4, 2022

View reviewed changes

daemon/config/config.go Show resolved Hide resolved

rumpl commented Aug 4, 2022

View reviewed changes

daemon/oci_linux.go Show resolved Hide resolved

rumpl requested review from ndeloof, thaJeztah and vvoland August 4, 2022 08:43

vvoland approved these changes Aug 4, 2022

View reviewed changes

daemon/oci_linux.go Outdated Show resolved Hide resolved

This was referenced Aug 4, 2022

vendor: re-run hack/vendor.sh #41

Merged

c8d/pull: Add options for stargz/nydus snapshotters #42

Merged

rumpl force-pushed the snapshotter-configuration branch from bea880d to 3e3c4bf Compare August 4, 2022 09:27

vvoland mentioned this pull request Aug 4, 2022

c8d/daemon: Add remote snapshotters to config #44

Open

rumpl force-pushed the snapshotter-configuration branch from 3e3c4bf to 8ed407d Compare August 4, 2022 14:54

thaJeztah reviewed Aug 4, 2022

View reviewed changes

rumpl force-pushed the snapshotter-configuration branch 4 times, most recently from 4eaca06 to d15224e Compare August 5, 2022 09:09

vvoland force-pushed the snapshotter-configuration branch from 4edbac0 to 53c9291 Compare August 8, 2022 08:43

rumpl commented Aug 8, 2022

View reviewed changes

rumpl requested a review from thaJeztah August 8, 2022 14:08

thaJeztah reviewed Aug 8, 2022

View reviewed changes

vvoland force-pushed the snapshotter-configuration branch from 53c9291 to f076116 Compare August 8, 2022 15:58

thaJeztah reviewed Aug 8, 2022

View reviewed changes

daemon/daemon.go Outdated Show resolved Hide resolved

vvoland force-pushed the snapshotter-configuration branch from f076116 to b6770ed Compare August 9, 2022 07:12

rumpl requested a review from thaJeztah August 9, 2022 08:33

thaJeztah reviewed Aug 9, 2022

View reviewed changes

thaJeztah mentioned this pull request Aug 9, 2022

Make the snapshotter configurable - review suggestions / ideas #48

Closed

vvoland force-pushed the snapshotter-configuration branch from b6770ed to f060068 Compare August 9, 2022 12:51

rumpl and others added 2 commits August 9, 2022 14:52

Make the snapshotter configurable

99655cc

Signed-off-by: Djordje Lukic <[email protected]>

vvoland force-pushed the snapshotter-configuration branch from f060068 to e645f73 Compare August 9, 2022 12:52

rumpl requested a review from thaJeztah August 9, 2022 12:59

thaJeztah approved these changes Aug 9, 2022

View reviewed changes

vvoland mentioned this pull request Aug 9, 2022

let buildx know we support containerd snapshotter #50

Merged

thaJeztah merged commit 4b035c9 into c8d Aug 9, 2022

thaJeztah deleted the snapshotter-configuration branch August 9, 2022 14:21

thaJeztah mentioned this pull request Aug 12, 2022

[RFC] libcontainerd/supervisor: use containerd's defaults moby/moby#43950

Draft

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Aug 18, 2022

daemon/config: remove unused DefaultContainerdSnapshotter

71c723b

This was added as part of rumpl#40, but wasn't used. Signed-off-by: Sebastiaan van Stijn <[email protected]>

This was referenced Aug 18, 2022

daemon/config: remove unused DefaultContainerdSnapshotter #66

Merged

ImageService: rename GraphDriverName to StorageDriver, remove daemon.graphdriver moby/moby#43982

Merged

daemon: make the snapshotter configurable moby/moby#43983

Merged

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Aug 22, 2022

daemon/config: remove unused DefaultContainerdSnapshotter

641415b

This was added as part of rumpl#40, but wasn't used. Signed-off-by: Sebastiaan van Stijn <[email protected]>

This was referenced Aug 24, 2022

image commit: use configured snapshotter instead of default #76

Merged

daemon: remove SnapshotterFromGraphDriver mapping #77

Merged

rumpl added the to-be-upstreamed label Aug 30, 2022

crazy-max pushed a commit to crazy-max/moby that referenced this pull request Sep 29, 2022

daemon/config: remove unused DefaultContainerdSnapshotter

8652947

This was added as part of rumpl#40, but wasn't used. Signed-off-by: Sebastiaan van Stijn <[email protected]>

vvoland added upstreamed and removed to-be-upstreamed labels Jan 13, 2023

		graphDriver = strings.TrimPrefix(graphDriver, "io.containerd.snapshotter.v1.")
		return graphDriver

Make the snapshotter configurable #40

Make the snapshotter configurable #40

Uh oh!

Conversation

rumpl commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

thaJeztah commented Aug 8, 2022

Uh oh!

rumpl 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland commented Aug 9, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah Aug 4, 2022 •

edited

Loading