introduce Containerd-snapshotter feature flag

[ndeloof]

- What I did
introduce Containerd-snapshotter feature flag

- How I did it
added support for Containerd-snapshotterfeature flag
introduced usesSnapshotter for usability in code
illustration in oci_linux for some obvious places where moby enforce a mounted rootFS, while migration to containerd will rely on an active snapshot

- How to verify it
no impact as this is just basic skaffolding to bootstrap containerd image store migration work. Enabling this new feature flag is expected to just break until some concrete development takes place.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

I'd prefer using all lowercase for this one; containerd-snapshotter

Should the list of features be returned in the /info API response (so that it can be shown in docker info output?)

[thaJeztah]

As to using the Features for this; note that the features are in the list of daemon options that are "live" reloadable, which means that the option can be changed without restarting the daemon;

moby/daemon/reload.go

Lines 270 to 278 in 20d6b5c

	// reloadFeatures updates configuration with enabled/disabled features
	func (daemon *Daemon) reloadFeatures(conf *config.Config, attributes map[string]string) {
	// update corresponding configuration
	// note that we allow features option to be entirely unset
	daemon.configStore.Features = conf.Features
	// prepare reload event attributes with updatable configurations
	attributes["features"] = fmt.Sprintf("%v", daemon.configStore.Features)
	}

What happens if the options is set, the daemon.json updated (or deleted), and the daemon-config is reloaded?

[ndeloof]

good point. Doing so would be unpredictable as existing containers will be either set with a snapshot or moby-managed rootfs, and won't be manageable by the code once flag has changed value. Same applies anyway to any flags mechanism that will require daemon to be restarted.

Maybe a feature flag is not the best option then. I'd be fine we decide to rely on a build flag if this makes more sense.

[thaJeztah]

Thinking what's the best option as well. Basically, we have a (somewhat) similar situation if you switch to a different storage-driver, but for that case, the daemon is able to detect the previous selection; perhaps we need something similar for this (detect what the daemon was configured with?) 🤔

[thaJeztah]

I see there's various locations where s.Root is referenced; should this be initialised with an empty specs.Root if daemon.usesSnapshotter() (to prevent panics), e.g. this may panic?

moby/libcontainerd/remote/client.go

Line 131 in 9ccedde

c.logger.WithField("bundle", bdir).WithField("root", ociSpec.Root.Path).Debug("bundle dir created")

Or should the field be propagated in some other way? There's also the ReadOnly option that is discarded now; is that passed on through some other ways?

[ndeloof]

enabling this flag in the current codebase will break, this PR just introduces the feature flag and utility method, with an initial illustration example where it is obviously needed, but it is not expected enabling this flag won't trigger a panic.
Longer terms, as this flag is enabled, s.Root should not be used anymore, as we will fully rely on snapshotter

[AkihiroSuda]

Thanks for working on this, but not sure this is ready to merge until it actually works (for docker pull at least)

[ndeloof]

@AkihiroSuda our goal is to avoid a log term branch with massive changes, and doing so we will propose small incremental changes which are by nature incomplete, and won't be able to offer a working daemon. So this feature flag is a way for us to get maintainer reviews, but enforce this won't impact Moby stability on master
See https://github.com/rumpl/moby/tree/containerd-store for initial exploratory work that will drive this effort

[thaJeztah]

This test is still flaky on windows; was discussing yesterday with Kevin; looks like Windows machines in GHA are slow, and in some cases need more than 10 seconds (which is already super long) to delete a container 😞

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

[thaJeztah]

Kicked it again

[ndeloof]

@AkihiroSuda are you ok with the approach? (see https://hackmd.io/@TAyMovgqSdezts48gqqkdQ/B1eYq-p_vq)

[tianon]

LGTM, but IMO we should cut a 22.06 branch before we merge it so it doesn't sneak into 22.06 yet 😇