From d1b8bcb2c785159b44d5a8e16bbd74665c2a0bba Mon Sep 17 00:00:00 2001 From: aiordache Date: Tue, 4 Aug 2020 17:41:49 +0200 Subject: [PATCH 1/4] fix signal passing to mobycli Signed-off-by: aiordache --- cli/cmd/context/create.go | 3 ++- cli/cmd/context/inspect.go | 3 ++- cli/cmd/context/ls.go | 3 ++- cli/cmd/login/login.go | 3 ++- cli/cmd/logout/logout.go | 3 ++- cli/cmd/version.go | 3 ++- cli/main.go | 2 +- cli/mobycli/exec.go | 17 ++++------------- 8 files changed, 17 insertions(+), 20 deletions(-) diff --git a/cli/cmd/context/create.go b/cli/cmd/context/create.go index 802a8a73..6c9b4eab 100644 --- a/cli/cmd/context/create.go +++ b/cli/cmd/context/create.go @@ -70,7 +70,8 @@ $ docker context create my-context --description "some description" --docker "ho Use: "create CONTEXT", Short: "Create new context", RunE: func(cmd *cobra.Command, args []string) error { - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil }, Long: longHelp, } diff --git a/cli/cmd/context/inspect.go b/cli/cmd/context/inspect.go index cbc98ab0..bc8af24b 100644 --- a/cli/cmd/context/inspect.go +++ b/cli/cmd/context/inspect.go @@ -27,7 +27,8 @@ func inspectCommand() *cobra.Command { Use: "inspect", Short: "Display detailed information on one or more contexts", RunE: func(cmd *cobra.Command, args []string) error { - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil }, } // flags matching delegated command in moby cli diff --git a/cli/cmd/context/ls.go b/cli/cmd/context/ls.go index 20c82ba7..910c371a 100644 --- a/cli/cmd/context/ls.go +++ b/cli/cmd/context/ls.go @@ -69,7 +69,8 @@ func runList(cmd *cobra.Command, opts lsOpts) error { return err } if opts.format != "" { - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil } ctx := cmd.Context() diff --git a/cli/cmd/login/login.go b/cli/cmd/login/login.go index 0fee7222..f24ad27f 100644 --- a/cli/cmd/login/login.go +++ b/cli/cmd/login/login.go @@ -56,7 +56,8 @@ func runLogin(cmd *cobra.Command, args []string) error { backend := args[0] return errors.New("unknown backend type for cloud login: " + backend) } - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil } func cloudLogin(cmd *cobra.Command, backendType string, params interface{}) error { diff --git a/cli/cmd/logout/logout.go b/cli/cmd/logout/logout.go index 0ea967a4..cfc63ed3 100644 --- a/cli/cmd/logout/logout.go +++ b/cli/cmd/logout/logout.go @@ -21,5 +21,6 @@ func Command() *cobra.Command { } func runLogout(cmd *cobra.Command, args []string) error { - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil } diff --git a/cli/cmd/version.go b/cli/cmd/version.go index 3bea2b12..d3cd088d 100644 --- a/cli/cmd/version.go +++ b/cli/cmd/version.go @@ -51,7 +51,8 @@ func runVersion(cmd *cobra.Command, version string) error { // we don't want to fail on error, there is an error if the engine is not available but it displays client version info // Still, technically the [] byte versionResult could be nil, just let the original command display what it has to display if versionResult == nil { - return mobycli.ExecCmd(cmd) + mobycli.Exec() + return nil } var s string = string(versionResult) fmt.Print(strings.Replace(s, "\n Version:", "\n Azure integration "+displayedVersion+"\n Version:", 1)) diff --git a/cli/main.go b/cli/main.go index 0645a7c8..9ef5fd25 100644 --- a/cli/main.go +++ b/cli/main.go @@ -151,7 +151,7 @@ func main() { // --host and --version should immediately be forwarded to the original cli if opts.Host != "" || opts.Version { - mobycli.Exec(ctx) + mobycli.Exec() } if opts.Config == "" { diff --git a/cli/mobycli/exec.go b/cli/mobycli/exec.go index bbb6c4ea..179eb4a1 100644 --- a/cli/mobycli/exec.go +++ b/cli/mobycli/exec.go @@ -23,8 +23,6 @@ import ( "os/exec" "strings" - "github.com/spf13/cobra" - apicontext "github.com/docker/api/context" "github.com/docker/api/context/store" ) @@ -43,7 +41,7 @@ func ExecIfDefaultCtxType(ctx context.Context) { currentCtx, err := s.Get(currentContext) // Only run original docker command if the current context is not ours. if err != nil || mustDelegateToMoby(currentCtx.Type()) { - Exec(ctx) + Exec() } } @@ -57,11 +55,12 @@ func mustDelegateToMoby(ctxType string) bool { } // Exec delegates to com.docker.cli if on moby context -func Exec(ctx context.Context) { - cmd := exec.CommandContext(ctx, ComDockerCli, os.Args[1:]...) +func Exec() { + cmd := exec.Command(ComDockerCli, os.Args[1:]...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { if exiterr, ok := err.(*exec.ExitError); ok { os.Exit(exiterr.ExitCode()) @@ -72,14 +71,6 @@ func Exec(ctx context.Context) { os.Exit(0) } -// ExecCmd delegates the cli command to com.docker.cli. The error is never -// returned (process will exit with docker classic exit code), the return type -// is to make it easier to use with cobra commands -func ExecCmd(command *cobra.Command) error { - Exec(command.Context()) - return nil -} - // IsDefaultContextCommand checks if the command exists in the classic cli (issues a shellout --help) func IsDefaultContextCommand(dockerCommand string) bool { cmd := exec.Command(ComDockerCli, dockerCommand, "--help") From 4dd1918ac2203b0756dbf236e46459b42a0188aa Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Wed, 5 Aug 2020 13:59:03 +0200 Subject: [PATCH 2/4] Improve test that was sometimes passing too quickly where it should fail --- tests/skip-win-ci-e2e/skip_win_ci_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/skip-win-ci-e2e/skip_win_ci_test.go b/tests/skip-win-ci-e2e/skip_win_ci_test.go index b8de4437..0bb5ce31 100644 --- a/tests/skip-win-ci-e2e/skip_win_ci_test.go +++ b/tests/skip-win-ci-e2e/skip_win_ci_test.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "github.com/docker/api/cli/mobycli" + . "github.com/onsi/gomega" "github.com/stretchr/testify/suite" @@ -50,9 +52,10 @@ RUN sleep 100`), 0644)).To(Succeed()) _, err := ctx.Exec() errs <- err }() + mobyBuild := mobycli.ComDockerCli + " build --no-cache -t " + imageName err := WaitFor(time.Second, 10*time.Second, errs, func() bool { out := s.ListProcessesCommand().ExecOrDie() - return strings.Contains(out, imageName) + return strings.Contains(out, mobyBuild) }) Expect(err).NotTo(HaveOccurred()) log.Println("Killing docker process") @@ -60,7 +63,7 @@ RUN sleep 100`), 0644)).To(Succeed()) close(shutdown) err = WaitFor(time.Second, 12*time.Second, nil, func() bool { out := s.ListProcessesCommand().ExecOrDie() - return !strings.Contains(out, imageName) + return !strings.Contains(out, mobyBuild) }) Expect(err).NotTo(HaveOccurred()) }) From 05a8582126280a0117d5f03a0106292ec43c1aef Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Wed, 5 Aug 2020 13:59:52 +0200 Subject: [PATCH 3/4] Forward all signals to child process --- cli/mobycli/exec.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/cli/mobycli/exec.go b/cli/mobycli/exec.go index 179eb4a1..14a5f8b9 100644 --- a/cli/mobycli/exec.go +++ b/cli/mobycli/exec.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "os/exec" + "os/signal" "strings" apicontext "github.com/docker/api/context" @@ -61,6 +62,17 @@ func Exec() { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + s := make(chan os.Signal) + signal.Notify(s) // catch all signals + go func() { + for sig := range s { + err := cmd.Process.Signal(sig) + if err != nil { + fmt.Printf("WARNING could not forward signal %s to %s : %s\n", sig.String(), ComDockerCli, err.Error()) + } + } + }() + if err := cmd.Run(); err != nil { if exiterr, ok := err.(*exec.ExitError); ok { os.Exit(exiterr.ExitCode()) From 29fe26620ab2ee8fca74a5b5ee9a1c4db4bdd52e Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Wed, 5 Aug 2020 14:42:21 +0200 Subject: [PATCH 4/4] Stop forwarding signals to child process when the child terminates --- cli/mobycli/exec.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/cli/mobycli/exec.go b/cli/mobycli/exec.go index 14a5f8b9..c9cdfb15 100644 --- a/cli/mobycli/exec.go +++ b/cli/mobycli/exec.go @@ -62,18 +62,26 @@ func Exec() { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - s := make(chan os.Signal) - signal.Notify(s) // catch all signals + signals := make(chan os.Signal) + childExit := make(chan bool) + signal.Notify(signals) // catch all signals go func() { - for sig := range s { - err := cmd.Process.Signal(sig) - if err != nil { - fmt.Printf("WARNING could not forward signal %s to %s : %s\n", sig.String(), ComDockerCli, err.Error()) + for { + select { + case sig := <-signals: + err := cmd.Process.Signal(sig) + if err != nil { + fmt.Printf("WARNING could not forward signal %s to %s : %s\n", sig.String(), ComDockerCli, err.Error()) + } + case <-childExit: + return } } }() - if err := cmd.Run(); err != nil { + err := cmd.Run() + childExit <- true + if err != nil { if exiterr, ok := err.(*exec.ExitError); ok { os.Exit(exiterr.ExitCode()) }