Add `--feature` to daemon flags by dmcgowan · Pull Request #48167 · moby/moby

dmcgowan · 2024-07-15T22:48:35Z

Adds a --feature flag to the daemon options. This allows quickly toggling features when running the daemon directly or writing daemon tests.

- Description for the changelog

Add a `--feature` flag to the daemon options.

thaJeztah · 2024-07-19T12:46:55Z

opts/opts.go

+func (opts *SetOpts) String() string {
+	return fmt.Sprintf("%v", opts.values)
+}


Slightly wondering if this should produce a string based on the map[string]bool or an output based on the (more human-readable) comma-separated list.

Do you know where the string-format surfaces? Also; is the stringer interface required for FlagOpts, or would this be something we could postpone (add when needed?)

thaJeztah · 2024-07-19T12:54:43Z

cmd/dockerd/config.go

 	flags.StringVar(&conf.ExecRoot, "exec-root", conf.ExecRoot, "Root directory for execution state files")
 	flags.StringVar(&conf.ContainerdAddr, "containerd", "", "containerd grpc address")
 	flags.BoolVar(&conf.CriContainerd, "cri-containerd", false, "start containerd with cri")
+	flags.Var(opts.NewNamedSetOpts("feature", conf.Features), "feature", "Enable feature in the daemon")


Oh (haven't checked yet); also wondering now what we do with other options; if we accept a comma-separated list, or only multiple flags (i.e. --something foo=bar,bar=baz versus --something foo=bar --something bar=baz).

I THINK we may be using the latter, and because of that are using singular for the flag-names (because each flag takes a single value). For example the --storage-opt flag only takes a single option, but the equivalent in the config-file is storage-opts (plural) as it accepts multiple.

We may need to look at consistency there.

That's already how this is implemented, right? Adding a new --feature flag which, when specified multiple times, adds entries to the (existing) features map in the configuration structure.

Ah, you're right, I think I mis-interpreted the type itself to accepting multiple values.

But this one should use plural (features); the name here is used to correlate flags with the config (daemon.json) to detect conflicts.

Currently, this PR doesn't detect those:

echo '{"features": {"containerd-snapshotter": true}}' > /etc/docker/daemon.json hack/make.sh binary && make install dockerd --feature containerd-snapshotter=true --validate configuration OK

Changing this to plural;

diff --git a/cmd/dockerd/config.go b/cmd/dockerd/config.go index 2ea669f0b1..bb6e44ac6e 100644 --- a/cmd/dockerd/config.go +++ b/cmd/dockerd/config.go @@ -28,7 +28,7 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) { flags.StringVar(&conf.ExecRoot, "exec-root", conf.ExecRoot, "Root directory for execution state files") flags.StringVar(&conf.ContainerdAddr, "containerd", "", "containerd grpc address") flags.BoolVar(&conf.CriContainerd, "cri-containerd", false, "start containerd with cri") - flags.Var(opts.NewNamedSetOpts("feature", conf.Features), "feature", "Enable feature in the daemon") + flags.Var(opts.NewNamedSetOpts("features", conf.Features), "feature", "Enable feature in the daemon") flags.Var(opts.NewNamedMapMapOpts("default-network-opts", conf.DefaultNetworkOpts, nil), "default-network-opt", "Default network options") flags.IntVar(&conf.MTU, "mtu", conf.MTU, `Set the MTU for the default "bridge" network`)

Then it correctly handles the conflict;

hack/make.sh binary && make install dockerd --feature containerd-snapshotter=true --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: features: (from flag: map[containerd-snapshotter:true], from file: map[containerd-snapshotter:true])

tianon

This LGTM, but I'd personally feel better about it if we could get eyes from @corhere who's semi-recently really familiar with how all our configuration intersects (🙇), because I know that bit's really complex and we've had some subtle bugs in that area.

vasylenko · 2024-09-06T08:26:52Z

Great initiative! This should simplify using the daemon in CI or other places where working with configuration files is cumbersome, or configuration may vary.

Question: Will this also allow setting the things like enabling containerd image store for the docker engine?
docker-library/docker#477

thaJeztah · 2024-09-06T11:15:17Z

opts/opts.go

+
+var _ NamedOption = &NamedSetOpts{}
+
+// NewNamedMapOpts creates a reference to a new NamedMapOpts struct.


Suggested change

// NewNamedMapOpts creates a reference to a new NamedMapOpts struct.

// NewNamedSetOpts creates a reference to a new NamedSetOpts struct.

thaJeztah · 2024-09-06T11:15:37Z

opts/opts.go

+	}
+}
+
+// Name returns the name of the NamedMapOpts in the configuration.


Suggested change

// Name returns the name of the NamedMapOpts in the configuration.

// Name returns the name of the NamedSetOpts in the configuration.

thaJeztah · 2024-09-06T11:23:14Z

opts/opts.go

+// NewSetOpts creates a new SetOpts with the specified set of values as a map of string to bool.
+func NewSetOpts(values map[string]bool) *SetOpts {
+	if values == nil {
+		values = make(map[string]bool)
+	}
+	return &SetOpts{
+		values: values,
+	}
+}


Looks like this one is only used internally to help NamedSetOpts - should we combine them? (I'm trying to reduce the public API of this opts package - we could even consider making the new one un-exported in daemon/config or somewhere in internal/ or daemon/internal.

thaJeztah · 2024-09-06T11:24:13Z

cmd/dockerd/config.go

 	flags.StringVar(&conf.ExecRoot, "exec-root", conf.ExecRoot, "Root directory for execution state files")
 	flags.StringVar(&conf.ContainerdAddr, "containerd", "", "containerd grpc address")
 	flags.BoolVar(&conf.CriContainerd, "cri-containerd", false, "start containerd with cri")
+	flags.Var(opts.NewNamedSetOpts("feature", conf.Features), "feature", "Enable feature in the daemon")


Ah, you're right, I think I mis-interpreted the type itself to accepting multiple values.

But this one should use plural (features); the name here is used to correlate flags with the config (daemon.json) to detect conflicts.

Currently, this PR doesn't detect those:

echo '{"features": {"containerd-snapshotter": true}}' > /etc/docker/daemon.json hack/make.sh binary && make install dockerd --feature containerd-snapshotter=true --validate configuration OK

Changing this to plural;

diff --git a/cmd/dockerd/config.go b/cmd/dockerd/config.go index 2ea669f0b1..bb6e44ac6e 100644 --- a/cmd/dockerd/config.go +++ b/cmd/dockerd/config.go @@ -28,7 +28,7 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) { flags.StringVar(&conf.ExecRoot, "exec-root", conf.ExecRoot, "Root directory for execution state files") flags.StringVar(&conf.ContainerdAddr, "containerd", "", "containerd grpc address") flags.BoolVar(&conf.CriContainerd, "cri-containerd", false, "start containerd with cri") - flags.Var(opts.NewNamedSetOpts("feature", conf.Features), "feature", "Enable feature in the daemon") + flags.Var(opts.NewNamedSetOpts("features", conf.Features), "feature", "Enable feature in the daemon") flags.Var(opts.NewNamedMapMapOpts("default-network-opts", conf.DefaultNetworkOpts, nil), "default-network-opt", "Default network options") flags.IntVar(&conf.MTU, "mtu", conf.MTU, `Set the MTU for the default "bridge" network`)

Then it correctly handles the conflict;

hack/make.sh binary && make install dockerd --feature containerd-snapshotter=true --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: features: (from flag: map[containerd-snapshotter:true], from file: map[containerd-snapshotter:true])

tianon · 2024-09-06T23:35:16Z

@vasylenko yes, this will allow enabling containerd-snapshotter -- I think that might be the only thing currently using features

Signed-off-by: Derek McGowan <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2024-09-12T13:37:30Z

Updated this PR:

fixed the feature -> features for the named config
moved the new options to internal/opts
added tests

@laurazard PTAL

dismissing, as I made the last updates

thaJeztah · 2024-09-12T13:39:19Z

TODO (but I may do so in a follow-up)

update the man-page (both here and in docker/cli for now)
update the daemon docs in docker/cli

laurazard

LGTM

thaJeztah

"LGTM" as well

I'll do a small follow-up for the man-page, but didn't want to block this one on that.

thaJeztah added area/cli Client status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/changelog docs/revisit area/daemon Core Engine impact/documentation labels Jul 19, 2024

thaJeztah added this to the 28.0.0 milestone Jul 19, 2024

thaJeztah reviewed Jul 19, 2024

View reviewed changes

tianon mentioned this pull request Sep 5, 2024

Allow to easily configure the containerd snapshotter with dind docker-library/docker#477

Closed

tianon approved these changes Sep 5, 2024

View reviewed changes

thaJeztah previously requested changes Sep 6, 2024

View reviewed changes

Add feature to daemon flags

f13c082

Signed-off-by: Derek McGowan <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the add-feature-flags-daemon branch from bbf7ec0 to f13c082 Compare September 12, 2024 13:36

thaJeztah added the process/cherry-pick/27.x label Sep 12, 2024

laurazard approved these changes Sep 12, 2024

View reviewed changes

thaJeztah approved these changes Sep 12, 2024

View reviewed changes

thaJeztah merged commit 164cae5 into moby:master Sep 12, 2024

thaJeztah deleted the add-feature-flags-daemon branch September 12, 2024 17:15

thaJeztah mentioned this pull request Sep 12, 2024

man: update dockerd man-page to include --feature flag #48486

Merged

thaJeztah added the containerd-integration Issues and PRs related to containerd integration label Sep 12, 2024

thaJeztah mentioned this pull request Sep 12, 2024

[27.x backport] Add feature to daemon flags #48487

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick/27.x labels Sep 12, 2024


		var _ NamedOption = &NamedSetOpts{}

		// NewNamedMapOpts creates a reference to a new NamedMapOpts struct.

	// NewNamedMapOpts creates a reference to a new NamedMapOpts struct.
	// NewNamedSetOpts creates a reference to a new NamedSetOpts struct.

	// Name returns the name of the NamedMapOpts in the configuration.
	// Name returns the name of the NamedSetOpts in the configuration.

Conversation

dmcgowan commented Jul 15, 2024 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

vasylenko commented Sep 6, 2024

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!

tianon commented Sep 6, 2024

Uh oh!

thaJeztah commented Sep 12, 2024

Uh oh!

thaJeztah commented Sep 12, 2024

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dmcgowan commented Jul 15, 2024 •

edited by thaJeztah

Loading