cmd/dockerd: assorted changes to improve context-passing, config loading#47412
Merged
thaJeztah merged 9 commits intomoby:masterfrom Sep 17, 2024
Merged
cmd/dockerd: assorted changes to improve context-passing, config loading#47412thaJeztah merged 9 commits intomoby:masterfrom
thaJeztah merged 9 commits intomoby:masterfrom
Conversation
thaJeztah
added
area/cli
thaJeztah
force-pushed
the
cleanup_daemon_start
branch
from
f6a69e6 to
1cb0cef
Compare
thaJeztah
changed the title
corhere
requested changes
Feb 23, 2024
Contributor
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
The premise of this PR is invalid: dockerd on Windows is not ignoring custom data-root paths
thaJeztah
force-pushed
the
cleanup_daemon_start
branch
from
1cb0cef to
6d111e7
Compare
thaJeztah
force-pushed
the
cleanup_daemon_start
branch
from
6d111e7 to
f228a4d
Compare
corhere
approved these changes
Sep 16, 2024
Contributor
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
LGTM! My only complaint is with the PR title: this refactor does not "fix" anything w.r.t. Windows config loading.
Comment on lines
+14
to
+22
| cli, err := newDaemonCLI(opts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if opts.Validate { | ||
| // If config wasn't OK we wouldn't have made it this far. | ||
| _, _ = fmt.Fprintln(os.Stderr, "configuration OK") | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
This block of code is now line-for-line identical between both platforms. I smell a refactoring opportunity! Looks like it would be very straightforward to lift it into the RunE closure, and turning the runDaemon platform-hook function as a decorator for (*daemonCLI).start().
func runDaemon(ctx context.Context, cli *daemonCLI)
Member
Author
There was a problem hiding this comment.
Ah, yes, that's an option! I'll have a look at that in a follow-up 👍
Member
Author
There was a problem hiding this comment.
thaJeztah
changed the title
thaJeztah
force-pushed
the
cleanup_daemon_start
branch
from
f228a4d to
1b24a61
Compare
This patch moves Windows-specific config for the config-file location to config- related code to help discoverability. Unlike Linux, which uses fixed locations as default, the Windows daemon uses paths relative to the data-root as defaults for storing both the PIDFile, and the daemon configuration file (daemon.json). For the PIDfile, additional changes will be needed, as using a PIDfile depends on whether the daemon is run as a service or not. Signed-off-by: Sebastiaan van Stijn <[email protected]>
getDefaultDaemonConfigDir would never return an error and because of that, neither would getDefaultDaemonConfigFile, so we can remove these error returns. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…entation Document why we cannot return a default on Windows. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Make creating the options slightly more atomic, and set the defaults when instancing the options. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Make it more explicit that this function is mutating the passed configuration. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Validate and apply options when creating the CLI, so that starting the CLI does not have to mutate the config, and to have a clearer separation between "creating", "validating", and starting the daemon. This also allows skipping the service-registration code in situations where we only want to validate the config. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…ions Unlike Linux, which uses fixed locations as default, the Windows daemon uses paths relative to the data-root as defaults for storing both the PIDFile, and the daemon configuration file (daemon.json). The data-root is configurable both through command-line options (`--data-root`), and through the daemon configuration file (daemon.json). This patch moves Windows- specific config handling to config-related code. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Construct the context we use in the main function, and set it as context for the root-command. Signed-off-by: Sebastiaan van Stijn <[email protected]>
They're only used within this package, and are not expected to be used externally. Some exported functions also take non-exported types as argument, so would not be usable outside of this package either way. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
cleanup_daemon_start
branch
from
1b24a61 to
b186261
Compare
robmry
approved these changes
Sep 17, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cmd/dockerd: move Windows code for data-root
This patch moves Windows-specific config for the config-file location to config-
related code to help discoverability.
Unlike Linux, which uses fixed locations as default, the Windows daemon uses
paths relative to the data-root as defaults for storing both the PIDFile, and
the daemon configuration file (daemon.json).
For the PIDfile, additional changes will be needed, as using a PIDfile depends
on whether the daemon is run as a service or not.
cmd/dockerd: remove unused error-returns
getDefaultDaemonConfigDir would never return an error and because of that,
neither would getDefaultDaemonConfigFile, so we can remove these error returns.
cmd/dockerd: getDefaultDaemonConfigFile: add GoDoc for Windows implementation
Document why we cannot return a default on Windows.
cmd/dockerd: set default configfile location as part of newDaemonOptions
Make creating the options slightly more atomic, and set the defaults when
instancing the options.
cmd/dockerd: rename loadCLIPlatformConfig to setPlatformOptions
Make it more explicit that this function is mutating the passed
configuration.
cmd/dockerd: apply options when creating daemonCLI, not when starting
Validate and apply options when creating the CLI, so that starting the
CLI does not have to mutate the config, and to have a clearer separation
between "creating", "validating", and starting the daemon.
This also allows skipping the service-registration code in situations
where we only want to validate the config.
cmd/dockerd: windows: move setting PIDFile location to setPlatformOptions
Unlike Linux, which uses fixed locations as default, the Windows daemon uses
paths relative to the data-root as defaults for storing both the PIDFile, and
the daemon configuration file (daemon.json).
The data-root is configurable both through command-line options (
--data-root),and through the daemon configuration file (daemon.json). This patch moves Windows-
specific config handling to config-related code.
cmd/dockerd: construct context in main
Construct the context we use in the main function, and set it as context
for the root-command.
cmd/dockerd: un-export DaemonCli, NewDaemonCli
They're only used within this package, and are not expected to be used
externally. Some exported functions also take non-exported types as
argument, so would not be usable outside of this package either way.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)