New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
windows: libcontainer: prefer bundled containerd.exe #43951
base: master
Are you sure you want to change the base?
windows: libcontainer: prefer bundled containerd.exe #43951
Conversation
binPath = filepath.Join(filepath.Dir(dockerdPath), binaryName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change this to make this a "preferred" binary;
- look for containerd.exe in the same path as dockerd.exe; if found, use that
- otherwise use containerd.exe from
$PATH
(as before this patch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1; I like that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach is Raymond Chen approved, too! https://devblogs.microsoft.com/oldnewthing/20110620-00/?p=10393
b268fe6
to
8341bb8
Compare
8341bb8
to
fcbfe99
Compare
// On Windows, it first checks if a containerd binary is found in the same | ||
// directory as the dockerd binary. If found, this binary takes precedence | ||
// over containerd binaries installed in $PATH. | ||
supervisor.WithDetectLocalBinary(), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the detection to the initialisation as a DaemonOpt
; this approach made it slightly more flexible (on Windows we use this option, on Linux not (yet?)); performing the lookup during initialisation stores the result in the supervisor's config.
The previous iteration performed the lookup during startContainerd()
, which is called in monitorDaemon()
, which re-starts containerd if it would fail, and (theoretically) would mean that deleting the containerd.exe
binary would cause dockerd
to lookup the binary again, and switch from a bundled containerd to a system containerd. I think that is undesirable, so it's better to fail in that case than to switch to a different binary.
if err := r.startContainerd(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably shouldn't use this on Linux unless we also disable it for the case of dockerd
in a common path like /usr/bin
, /usr/sbin
, etc. 😅
(This seems like a sane approach regardless though, to be clear 👍)
// binary to start if found. If no binary is found, no changes are made. | ||
func WithDetectLocalBinary() DaemonOpt { | ||
return func(r *remote) error { | ||
dockerdPath, err := os.Executable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that os.Executable()
does not resolve symlinks. I think that's what we expect, but thought I'd leave a comment just in case there's concerns about that. https://github.com/golang/go/blob/8cb350d69a1b0765c1c81301583d6fd99fb9d74b/src/os/executable.go#L7-L14
// Executable returns the path name for the executable that started
// the current process. There is no guarantee that the path is still
// pointing to the correct executable. If a symlink was used to start
// the process, depending on the operating system, the result might
// be the symlink or the path it pointed to. If a stable result is
// needed, path/filepath.EvalSymlinks might help.
//
// Executable returns an absolute path unless an error occurred.
//
// The main use case is finding resources located relative to an
// executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't totally understand the intent of the .github/workflows/windows.yml
changes, but the rest seems like a good approach IMO 👍
Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Prefer a containerd binary that's installed next to the dockerd binary. This prevents running a containerd executable that was installed through other means instead of the bundled one, in cases where the docker is installed using the static binaries. Also see this MicroSoft blog-post about this approach; https://devblogs.microsoft.com/oldnewthing/20110620-00/?p=10393 This patch impements a WithDetectLocalBinary() DaemonOpt, which is set by default on Windows (we can consider using the same on other platforms). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
fcbfe99
to
2ea0657
Compare
This removes the bit I commented on in #43893 (comment)
Prefer a containerd binary that's installed next to the dockerd
binary. This prevents running a containerd executable that was installed through
other means instead of the bundled one, in cases where the docker is installed
using the static binaries.
Also see this MicroSoft blog-post about this approach;
https://devblogs.microsoft.com/oldnewthing/20110620-00/?p=10393
This patch impements a WithDetectLocalBinary() DaemonOpt, which is set by
default on Windows (we can consider using the same on other platforms).
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)