Skip to content

plugins: run plugin with new process group ID #4769

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Jan 12, 2024
Merged

plugins: run plugin with new process group ID #4769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli-plugins/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
configureOSSpecificCommand(cmd)

cmd.Env = os.Environ()
cmd.Env = append(cmd.Env, ReexecEnvvar+"="+os.Args[0])
Expand Down
14 changes: 14 additions & 0 deletions cli-plugins/manager/manager_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,21 @@

package manager

import (
"os/exec"
"syscall"
)

var defaultSystemPluginDirs = []string{
"/usr/local/lib/docker/cli-plugins", "/usr/local/libexec/docker/cli-plugins",
"/usr/lib/docker/cli-plugins", "/usr/libexec/docker/cli-plugins",
}

func configureOSSpecificCommand(cmd *exec.Cmd) {
// Spawn the plugin process in a new process group, so that signals are not forwarded by the OS.
// The foreground process group is e.g. sent a SIGINT when Ctrl-C is input to the TTY, but we
// implement our own job control for the plugin.
cmd.SysProcAttr = &syscall.SysProcAttr{
Setpgid: true,
Copy link
Member

@thaJeztah thaJeztah Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping this was abstracted in the syscall package (and that to be the reason to use syscall instead of golang.org/x/sys), but looks like it's not 😢 (otherwise we could've dropped the abstraction)

Well.. it is what it is 🤷‍♂️

Copy link
Member

@thaJeztah thaJeztah Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding a comment why we're doing this (writing down the intent; it looks like you already have a good description in the commit message)? Or do you think it's clear from the code alone?

(A good way to judge: would you understand (without looking in history) 1 Year from now when looking at the code?)

Copy link
Collaborator Author

@laurazard laurazard Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm too used to having to do dig through many years old commits to understand code that I don't even notice anymore 😞 I'll add some more comments!

Copy link
Member

@thaJeztah thaJeztah Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I'm pretty sure we'd be able to trace it back if needed. I started to pay more attention to these things (sorry for being the victim of that), for all those times I had to spend "too much time" finding back the reason.

Having a comment also can help deciding making changes ("oh! thanks for the description; this is soooooo last year and really no longer needed. I can confidently remove this part").

}
}
5 changes: 5 additions & 0 deletions cli-plugins/manager/manager_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@ package manager

import (
"os"
"os/exec"
"path/filepath"
)

var defaultSystemPluginDirs = []string{
filepath.Join(os.Getenv("ProgramData"), "Docker", "cli-plugins"),
filepath.Join(os.Getenv("ProgramFiles"), "Docker", "cli-plugins"),
}

func configureOSSpecificCommand(cmd *exec.Cmd) {
// no-op
}
6 changes: 5 additions & 1 deletion cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,12 +230,16 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string,
// we send a SIGKILL to the plugin process and exit
go func() {
retries := 0
for range signals {
for s := range signals {
if conn != nil {
if err := conn.Close(); err != nil {
_, _ = fmt.Fprintf(dockerCli.Err(), "failed to signal plugin to close: %v\n", err)
}
conn = nil
} else {
// When the plugin is communicating via socket with the host CLI, we perform job control via the socket.
// However, if the plugin is an old version that is not socket-aware, we need to explicitly forward termination signals.
plugincmd.Process.Signal(s)
}
retries++
if retries >= exitLimit {
Expand Down