windows: libcontainer: prefer bundled containerd.exe#43951
windows: libcontainer: prefer bundled containerd.exe#43951thaJeztah wants to merge 2 commits intomoby:masterfrom
Conversation
| binPath = filepath.Join(filepath.Dir(dockerdPath), binaryName) | ||
| } |
There was a problem hiding this comment.
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.
That approach is Raymond Chen approved, too! https://devblogs.microsoft.com/oldnewthing/20110620-00/?p=10393
thaJeztah
force-pushed
the
windows_containerd_as_proces_rebase_and_suggestions
branch
from
b268fe6 to
8341bb8
Compare
thaJeztah
changed the title
thaJeztah
force-pushed
the
windows_containerd_as_proces_rebase_and_suggestions
branch
from
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.
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.
There was a problem hiding this comment.
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.
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.
tianon
left a comment
tianon
left a comment
There was a problem hiding this comment.
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 <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
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 <[email protected]>
thaJeztah
force-pushed
the
windows_containerd_as_proces_rebase_and_suggestions
branch
from
fcbfe99 to
2ea0657
Compare
4 participants
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)