Merge pull request #6468 from mheon/remote_detached_exec

Enable detached exec for remote

12 files changed, 122 insertions, 33 deletions

diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh
index 945b33909..25b7ff941 100755
--- a/contrib/cirrus/setup_environment.sh
+++ b/contrib/cirrus/setup_environment.sh
@@ -46,6 +46,9 @@ case "${OS_RELEASE_ID}" in
 
         workaround_bfq_bug
 
+	# HACK: Need Conmon 2.0.17, currently in updates-testing on F31.
+	dnf update -y --enablerepo=updates-testing conmon
+
         if [[ "$ADD_SECOND_PARTITION" == "true" ]]; then
             bash "$SCRIPT_BASE/add_second_partition.sh"
         fi
diff --git a/contrib/spec/podman.spec.in b/contrib/spec/podman.spec.in
index 701b5f0b1..9ab0db3ae 100644
--- a/contrib/spec/podman.spec.in
+++ b/contrib/spec/podman.spec.in
@@ -33,7 +33,7 @@
 # People want conmon packaged with the copr rpm
 %global import_path_conmon      github.com/containers/conmon
 %global git_conmon      https://%{import_path_conmon}
-%global commit_conmon   d532caebc788fafdd2a305b68cd1983b4039bea4
+%global commit_conmon   41877362fc4685d55e0473d2e4a1cbe5e1debee0
 %global shortcommit_conmon %(c=%{commit_conmon}; echo ${c:0:7})
 
 Name: podman
diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index f2943b73c..a0e8904dc 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -69,6 +69,10 @@ type ExecConfig struct {
 	// The ID of the exec session, and the ID of the container the exec
 	// session is a part of (in that order).
 	ExitCommand []string `json:"exitCommand,omitempty"`
+	// ExitCommandDelay is a delay (in seconds) between the container
+	// exiting, and the exit command being executed. If set to 0, there is
+	// no delay. If set, ExitCommand must also be set.
+	ExitCommandDelay uint `json:"exitCommandDelay,omitempty"`
 }
 
 // ExecSession contains information on a single exec session attached to a given
@@ -165,6 +169,9 @@ func (c *Container) ExecCreate(config *ExecConfig) (string, error) {
 	if len(config.Command) == 0 {
 		return "", errors.Wrapf(define.ErrInvalidArg, "must provide a non-empty command to start an exec session")
 	}
+	if config.ExitCommandDelay > 0 && len(config.ExitCommand) == 0 {
+		return "", errors.Wrapf(define.ErrInvalidArg, "must provide a non-empty exit command if giving an exit command delay")
+	}
 
 	// Verify that we are in a good state to continue
 	if !c.ensureState(define.ContainerStateRunning) {
@@ -984,6 +991,7 @@ func prepareForExec(c *Container, session *ExecSession) (*ExecOptions, error) {
 	opts.PreserveFDs = session.Config.PreserveFDs
 	opts.DetachKeys = session.Config.DetachKeys
 	opts.ExitCommand = session.Config.ExitCommand
+	opts.ExitCommandDelay = session.Config.ExitCommandDelay
 
 	return opts, nil
 }
diff --git a/libpod/oci.go b/libpod/oci.go
index 7c5218319..684a7ba42 100644
--- a/libpod/oci.go
+++ b/libpod/oci.go
@@ -172,6 +172,9 @@ type ExecOptions struct {
 	// ExitCommand is a command that will be run after the exec session
 	// exits.
 	ExitCommand []string
+	// ExitCommandDelay is a delay (in seconds) between the exec session
+	// exiting, and the exit command being invoked.
+	ExitCommandDelay uint
 }
 
 // HTTPAttachStreams informs the HTTPAttach endpoint which of the container's
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index 51819f90a..bc39100f8 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -421,12 +421,14 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 		for _, arg := range options.ExitCommand[1:] {
 			args = append(args, []string{"--exit-command-arg", arg}...)
 		}
+		if options.ExitCommandDelay > 0 {
+			args = append(args, []string{"--exit-delay", fmt.Sprintf("%d", options.ExitCommandDelay)}...)
+		}
 	}
 
 	logrus.WithFields(logrus.Fields{
 		"args": args,
 	}).Debugf("running conmon: %s", r.conmonPath)
-	// TODO: Need to pass this back so we can wait on it.
 	execCmd := exec.Command(r.conmonPath, args...)
 
 	// TODO: This is commented because it doesn't make much sense in HTTP
diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
index 6865a3319..8f7016903 100644
--- a/pkg/api/handlers/compat/exec.go
+++ b/pkg/api/handlers/compat/exec.go
@@ -10,6 +10,7 @@ import (
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/api/handlers"
 	"github.com/containers/libpod/pkg/api/handlers/utils"
+	"github.com/containers/libpod/pkg/specgen/generate"
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -54,6 +55,24 @@ func ExecCreateHandler(w http.ResponseWriter, r *http.Request) {
 	libpodConfig.Privileged = input.Privileged
 	libpodConfig.User = input.User
 
+	// Make our exit command
+	storageConfig := runtime.StorageConfig()
+	runtimeConfig, err := runtime.GetConfig()
+	if err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+	exitCommandArgs, err := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, false, true, true)
+	if err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+	libpodConfig.ExitCommand = exitCommandArgs
+
+	// Run the exit command after 5 minutes, to mimic Docker's exec cleanup
+	// behavior.
+	libpodConfig.ExitCommandDelay = 5 * 60
+
 	sessID, err := ctr.ExecCreate(libpodConfig)
 	if err != nil {
 		if errors.Cause(err) == define.ErrCtrStateInvalid {
@@ -104,15 +123,6 @@ func ExecInspectHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	utils.WriteResponse(w, http.StatusOK, inspectOut)
-
-	// Only for the Compat API: we want to remove sessions that were
-	// stopped. This is very hacky, but should suffice for now.
-	if !utils.IsLibpodRequest(r) && inspectOut.CanRemove {
-		logrus.Infof("Pruning stale exec session %s from container %s", sessionID, sessionCtr.ID())
-		if err := sessionCtr.ExecRemove(sessionID, false); err != nil && errors.Cause(err) != define.ErrNoSuchExecSession {
-			logrus.Errorf("Error removing stale exec session %s from container %s: %v", sessionID, sessionCtr.ID(), err)
-		}
-	}
 }
 
 // ExecStartHandler runs a given exec session.
@@ -121,7 +131,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 
 	sessionID := mux.Vars(r)["id"]
 
-	// TODO: We should read/support Tty and Detach from here.
+	// TODO: We should read/support Tty from here.
 	bodyParams := new(handlers.ExecStartConfig)
 
 	if err := json.NewDecoder(r.Body).Decode(&bodyParams); err != nil {
@@ -129,11 +139,6 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 			errors.Wrapf(err, "failed to decode parameters for %s", r.URL.String()))
 		return
 	}
-	if bodyParams.Detach {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Errorf("Detached exec is not yet supported"))
-		return
-	}
 	// TODO: Verify TTY setting against what inspect session was made with
 
 	sessionCtr, err := runtime.GetExecSessionContainer(sessionID)
@@ -154,6 +159,19 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if bodyParams.Detach {
+		// If we are detaching, we do NOT want to hijack.
+		// Instead, we perform a detached start, and return 200 if
+		// successful.
+		if err := sessionCtr.ExecStart(sessionID); err != nil {
+			utils.InternalServerError(w, err)
+			return
+		}
+		// This is a 200 despite having no content
+		utils.WriteResponse(w, http.StatusOK, "")
+		return
+	}
+
 	// Hijack the connection
 	hijacker, ok := w.(http.Hijacker)
 	if !ok {
diff --git a/pkg/api/server/register_exec.go b/pkg/api/server/register_exec.go
index 17181d286..af9a83496 100644
--- a/pkg/api/server/register_exec.go
+++ b/pkg/api/server/register_exec.go
@@ -13,7 +13,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec (compat)
 	// summary: Create an exec instance
-	// description: Run a command inside a running container.
+	// description: Create an exec session to run a command inside a running container. Exec sessions will be automatically removed 5 minutes after they exit.
 	// parameters:
 	//  - in: path
 	//    name: name
@@ -153,7 +153,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec (compat)
 	// summary: Inspect an exec instance
-	// description: Return low-level information about an exec instance. Stale (stopped) exec sessions will be auto-removed after inspect runs.
+	// description: Return low-level information about an exec instance.
 	// parameters:
 	//  - in: path
 	//    name: id
@@ -182,7 +182,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec
 	// summary: Create an exec instance
-	// description: Run a command inside a running container.
+	// description: Create an exec session to run a command inside a running container. Exec sessions will be automatically removed 5 minutes after they exit.
 	// parameters:
 	//  - in: path
 	//    name: name
diff --git a/pkg/bindings/containers/exec.go b/pkg/bindings/containers/exec.go
index 2aeeae1f8..73cfb5079 100644
--- a/pkg/bindings/containers/exec.go
+++ b/pkg/bindings/containers/exec.go
@@ -1,6 +1,7 @@
 package containers
 
 import (
+	"bytes"
 	"context"
 	"net/http"
 	"strings"
@@ -69,3 +70,31 @@ func ExecInspect(ctx context.Context, sessionID string) (*define.InspectExecSess
 
 	return respStruct, nil
 }
+
+// ExecStart starts (but does not attach to) a given exec session.
+func ExecStart(ctx context.Context, sessionID string) error {
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return err
+	}
+
+	logrus.Debugf("Starting exec session ID %q", sessionID)
+
+	// We force Detach to true
+	body := struct {
+		Detach bool `json:"Detach"`
+	}{
+		Detach: true,
+	}
+	bodyJSON, err := json.Marshal(body)
+	if err != nil {
+		return err
+	}
+
+	resp, err := conn.DoRequest(bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/start", nil, nil, sessionID)
+	if err != nil {
+		return err
+	}
+
+	return resp.Process(nil)
+}
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
index e982c7c11..706fcec47 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -613,12 +613,11 @@ func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrId s
 	if err != nil {
 		return "", errors.Wrapf(err, "error retrieving Libpod configuration to build exec exit command")
 	}
-	podmanPath, err := os.Executable()
+	// TODO: Add some ability to toggle syslog
+	exitCommandArgs, err := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, false, true, true)
 	if err != nil {
-		return "", errors.Wrapf(err, "error retrieving executable to build exec exit command")
+		return "", errors.Wrapf(err, "error constructing exit command for exec session")
 	}
-	// TODO: Add some ability to toggle syslog
-	exitCommandArgs := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, podmanPath, false, true, true)
 	execConfig.ExitCommand = exitCommandArgs
 
 	// Create and start the exec session
diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index e1c859e7c..af9d94293 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -376,7 +376,7 @@ func (ic *ContainerEngine) ContainerAttach(ctx context.Context, nameOrId string,
 	return containers.Attach(ic.ClientCxt, nameOrId, &options.DetachKeys, nil, bindings.PTrue, options.Stdin, options.Stdout, options.Stderr, nil)
 }
 
-func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, options entities.ExecOptions, streams define.AttachStreams) (int, error) {
+func makeExecConfig(options entities.ExecOptions) *handlers.ExecCreateConfig {
 	env := []string{}
 	for k, v := range options.Envs {
 		env = append(env, fmt.Sprintf("%s=%s", k, v))
@@ -395,6 +395,12 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, o
 	createConfig.WorkingDir = options.WorkDir
 	createConfig.Cmd = options.Cmd
 
+	return createConfig
+}
+
+func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, options entities.ExecOptions, streams define.AttachStreams) (int, error) {
+	createConfig := makeExecConfig(options)
+
 	sessionID, err := containers.ExecCreate(ic.ClientCxt, nameOrId, createConfig)
 	if err != nil {
 		return 125, err
@@ -412,8 +418,19 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, o
 	return inspectOut.ExitCode, nil
 }
 
-func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (string, error) {
-	return "", errors.New("not implemented")
+func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrId string, options entities.ExecOptions) (string, error) {
+	createConfig := makeExecConfig(options)
+
+	sessionID, err := containers.ExecCreate(ic.ClientCxt, nameOrId, createConfig)
+	if err != nil {
+		return "", err
+	}
+
+	if err := containers.ExecStart(ic.ClientCxt, sessionID); err != nil {
+		return "", err
+	}
+
+	return sessionID, nil
 }
 
 func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, input, output, errput *os.File) error { //nolint
diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
index ffd7fd4dd..7ddfed339 100644
--- a/pkg/specgen/generate/container_create.go
+++ b/pkg/specgen/generate/container_create.go
@@ -107,12 +107,12 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 	}
 	options = append(options, opts...)
 
-	podmanPath, err := os.Executable()
+	// TODO: Enable syslog support - we'll need to put this in SpecGen.
+	exitCommandArgs, err := CreateExitCommandArgs(rt.StorageConfig(), rtc, false, s.Remove, false)
 	if err != nil {
 		return nil, err
 	}
-	// TODO: Enable syslog support - we'll need to put this in SpecGen.
-	options = append(options, libpod.WithExitCommand(CreateExitCommandArgs(rt.StorageConfig(), rtc, podmanPath, false, s.Remove, false)))
+	options = append(options, libpod.WithExitCommand(exitCommandArgs))
 
 	runtimeSpec, err := SpecGenToOCI(ctx, s, rt, rtc, newImage, finalMounts)
 	if err != nil {
@@ -229,13 +229,18 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen.
 	return options, nil
 }
 
-func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Config, podmanPath string, syslog, rm bool, exec bool) []string {
+func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Config, syslog, rm, exec bool) ([]string, error) {
 	// We need a cleanup process for containers in the current model.
 	// But we can't assume that the caller is Podman - it could be another
 	// user of the API.
 	// As such, provide a way to specify a path to Podman, so we can
 	// still invoke a cleanup process.
 
+	podmanPath, err := os.Executable()
+	if err != nil {
+		return nil, err
+	}
+
 	command := []string{podmanPath,
 		"--root", storageConfig.GraphRoot,
 		"--runroot", storageConfig.RunRoot,
@@ -265,9 +270,11 @@ func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Co
 		command = append(command, "--rm")
 	}
 
+	// This has to be absolutely last, to ensure that the exec session ID
+	// will be added after it by Libpod.
 	if exec {
 		command = append(command, "--exec")
 	}
 
-	return command
+	return command, nil
 }
diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go
index 8ec666c2b..b152da9e6 100644
--- a/test/e2e/exec_test.go
+++ b/test/e2e/exec_test.go
@@ -25,6 +25,10 @@ var _ = Describe("Podman exec", func() {
 		podmanTest = PodmanTestCreate(tempdir)
 		podmanTest.Setup()
 		podmanTest.SeedImages()
+		// HACK: Remove this once we get Conmon 2.0.17 on Ubuntu
+		if podmanTest.Host.Distribution == "ubuntu" {
+			Skip("Unable to perform test on Ubuntu distributions due to too-old Conmon (need 2.0.17)")
+		}
 	})
 
 	AfterEach(func() {
@@ -284,7 +288,6 @@ var _ = Describe("Podman exec", func() {
 	})
 
 	It("podman exec --detach", func() {
-		Skip(v2remotefail)
 		ctrName := "testctr"
 		ctr := podmanTest.Podman([]string{"run", "-t", "-i", "-d", "--name", ctrName, ALPINE, "top"})
 		ctr.WaitWithDefaultTimeout()