pod infra container is started before a container in a pod is run, st…

…arted or attached.

Prior, a pod would have to be started immediately when created, leading to confusion about what a pod state should be immediately after creation. The problem was podman run --pod ... would error out if the infra container wasn't started (as it is a dependency). Fix this by allowing for recursive start, where each of the container's dependencies are started prior to the new container. This is only applied to the case where a new container is attached to a pod.

Also rework container_api Start, StartAndAttach, and Init functions, as there was some duplicated code, which made addressing the problem easier to fix.

Signed-off-by: Peter Hunt <pehunt@redhat.com>

.gitignore

@@ -17,3 +17,4 @@ __pycache__
 /cmd/podman/varlink/ioprojectatomicpodman.go
 /cmd/podman/varlink/iopodman.go
 .gopathok
+test/e2e/e2e.coverprofile

cmd/podman/attach.go

@@ -74,7 +74,8 @@ func attachCmd(c *cliconfig.AttachValues) error {
 		inputStream = nil
 	}
 
-	if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false); err != nil && errors.Cause(err) != libpod.ErrDetach {
+	// If the container is in a pod, also set to recursively start dependencies
+	if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false, ctr.PodID() != ""); err != nil && errors.Cause(err) != libpod.ErrDetach {
 		return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
 	}
 

cmd/podman/play_kube.go

@@ -154,7 +154,7 @@ func playKubeYAMLCmd(c *cliconfig.KubePlayValues) error {
 
 	// start the containers
 	for _, ctr := range containers {
-		if err := ctr.Start(ctx); err != nil {
+		if err := ctr.Start(ctx, false); err != nil {
 			// Making this a hard failure here to avoid a mess
 			// the other containers are in created status
 			return err

cmd/podman/run.go

@@ -72,7 +72,8 @@ func runCmd(c *cliconfig.RunValues) error {
 	ctx := getContext()
 	// Handle detached start
 	if createConfig.Detach {
-		if err := ctr.Start(ctx); err != nil {
+		// if the container was created as part of a pod, also start its dependencies, if any.
+		if err := ctr.Start(ctx, c.IsSet("pod")); err != nil {
 			// This means the command did not exist
 			exitCode = 127
 			if strings.Index(err.Error(), "permission denied") > -1 {
@@ -117,15 +118,15 @@ func runCmd(c *cliconfig.RunValues) error {
 			}
 		}
 	}
-	if err := startAttachCtr(ctr, outputStream, errorStream, inputStream, c.String("detach-keys"), c.Bool("sig-proxy"), true); err != nil {
+	// if the container was created as part of a pod, also start its dependencies, if any.
+	if err := startAttachCtr(ctr, outputStream, errorStream, inputStream, c.String("detach-keys"), c.Bool("sig-proxy"), true, c.IsSet("pod")); err != nil {
 		// We've manually detached from the container
 		// Do not perform cleanup, or wait for container exit code
 		// Just exit immediately
 		if errors.Cause(err) == libpod.ErrDetach {
 			exitCode = 0
 			return nil
 		}
-
 		// This means the command did not exist
 		exitCode = 127
 		if strings.Index(err.Error(), "permission denied") > -1 {

cmd/podman/start.go

@@ -107,7 +107,8 @@ func startCmd(c *cliconfig.StartValues) error {
 			}
 
 			// attach to the container and also start it not already running
-			err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning)
+			// If the container is in a pod, also set to recursively start dependencies
+			err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning, ctr.PodID() != "")
 			if errors.Cause(err) == libpod.ErrDetach {
 				// User manually detached
 				// Exit cleanly immediately
@@ -136,7 +137,8 @@ func startCmd(c *cliconfig.StartValues) error {
 			continue
 		}
 		// Handle non-attach start
-		if err := ctr.Start(ctx); err != nil {
+		// If the container is in a pod, also set to recursively start dependencies
+		if err := ctr.Start(ctx, ctr.PodID() != ""); err != nil {
 			var createArtifact cc.CreateConfig
 			artifact, artifactErr := ctr.GetArtifact("create-config")
 			if artifactErr == nil {

cmd/podman/utils.go

@@ -20,7 +20,7 @@ type RawTtyFormatter struct {
 }
 
 // Start (if required) and attach to a container
-func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool, startContainer bool) error {
+func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool, startContainer bool, recursive bool) error {
 	ctx := context.Background()
 	resize := make(chan remotecommand.TerminalSize)
 
@@ -76,7 +76,7 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac
 		return ctr.Attach(streams, detachKeys, resize)
 	}
 
-	attachChan, err := ctr.StartAndAttach(getContext(), streams, detachKeys, resize)
+	attachChan, err := ctr.StartAndAttach(getContext(), streams, detachKeys, resize, recursive)
 	if err != nil {
 		return err
 	}

contrib/perftest/main.go

@@ -201,7 +201,7 @@ func runSingleThreadedStressTest(ctx context.Context, client *libpod.Runtime, im
 
 		// Start container
 		startStartTime := time.Now()
-		err = ctr.Start(ctx)
+		err = ctr.Start(ctx, false)
 		if err != nil {
 			return nil, err
 		}

docs/podman-pod-create.1.md

@@ -10,8 +10,8 @@ podman\-pod\-create - Create a new pod
 
 Creates an empty pod, or unit of multiple containers, and prepares it to have
 containers added to it. The pod id is printed to STDOUT. You can then use
-**podman create --pod <pod_id|pod_name> ...** to add containers to the pod, and
-**podman pod start <pod_id|pod_name>** to start the pod.
+**podman create --pod \<pod_id|pod_name\> ...** to add containers to the pod, and
+**podman pod start \<pod_id|pod_name\>** to start the pod.
 
 ## OPTIONS
 

docs/podman-run.1.md

@@ -449,6 +449,7 @@ Tune the container's pids limit. Set `-1` to have unlimited pids for the contain
 
 Run container in an existing pod. If you want podman to make the pod for you, preference the pod name with `new:`.
 To make a pod with more granular options, use the `podman pod create` command before creating a container.
+If a container is run with a pod, and the pod has an infra-container, the infra-container will be started before the container is.
 
 **--privileged**=*true*|*false*
 

docs/podman-start.1.md

@@ -8,7 +8,7 @@ podman\-start - Start one or more containers
 
 ## DESCRIPTION
 Start one or more containers.  You may use container IDs or names as input.  The *attach* and *interactive*
-options cannot be used to override the *--tty** and *--interactive* options from when the container
+options cannot be used to override the *--tty* and *--interactive* options from when the container
 was created. If you attempt to start a running container with the *--attach* option, podman will simply
 attach to the container.
 
@@ -33,6 +33,7 @@ Attach container's STDIN. The default is false.
 Instead of providing the container name or ID, use the last created container. If you use methods other than Podman
 to run containers such as CRI-O, the last started container could be from either of those methods.
 
+
 **--sig-proxy**=*true*|*false*
 
 Proxy received signals to the process (non-TTY mode only). SIGCHLD, SIGSTOP, and SIGKILL are not proxied. The default is *true* when attaching, *false* otherwise.

libpod/container_api.go

@@ -7,7 +7,6 @@ import (
 	"io/ioutil"
 	"os"
 	"strconv"
-	"strings"
 	"time"
 
 	"github.com/containers/libpod/libpod/driver"
@@ -38,24 +37,15 @@ func (c *Container) Init(ctx context.Context) (err error) {
 		return errors.Wrapf(ErrCtrExists, "container %s has already been created in runtime", c.ID())
 	}
 
-	notRunning, err := c.checkDependenciesRunning()
-	if err != nil {
-		return errors.Wrapf(err, "error checking dependencies for container %s", c.ID())
-	}
-	if len(notRunning) > 0 {
-		depString := strings.Join(notRunning, ",")
-		return errors.Wrapf(ErrCtrStateInvalid, "some dependencies of container %s are not started: %s", c.ID(), depString)
+	// don't recursively start
+	if err := c.checkDependenciesAndHandleError(ctx); err != nil {
+		return err
 	}
 
-	defer func() {
-		if err != nil {
-			if err2 := c.cleanup(ctx); err2 != nil {
-				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
-			}
-		}
-	}()
-
 	if err := c.prepare(); err != nil {
+		if err2 := c.cleanup(ctx); err2 != nil {
+			logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
+		}
 		return err
 	}
 
@@ -68,13 +58,14 @@ func (c *Container) Init(ctx context.Context) (err error) {
 	return c.init(ctx)
 }
 
-// Start starts a container
-// Start can start configured, created or stopped containers
+// Start starts a container.
+// Start can start configured, created or stopped containers.
 // For configured containers, the container will be initialized first, then
-// started
+// started.
 // Stopped containers will be deleted and re-created in runc, undergoing a fresh
-// Init()
-func (c *Container) Start(ctx context.Context) (err error) {
+// Init().
+// If recursive is set, Start will also start all containers this container depends on.
+func (c *Container) Start(ctx context.Context, recursive bool) (err error) {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -83,64 +74,26 @@ func (c *Container) Start(ctx context.Context) (err error) {
 			return err
 		}
 	}
-
-	// Container must be created or stopped to be started
-	if !(c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateCreated ||
-		c.state.State == ContainerStateStopped ||
-		c.state.State == ContainerStateExited) {
-		return errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID())
-	}
-
-	notRunning, err := c.checkDependenciesRunning()
-	if err != nil {
-		return errors.Wrapf(err, "error checking dependencies for container %s", c.ID())
-	}
-	if len(notRunning) > 0 {
-		depString := strings.Join(notRunning, ",")
-		return errors.Wrapf(ErrCtrStateInvalid, "some dependencies of container %s are not started: %s", c.ID(), depString)
-	}
-
-	defer func() {
-		if err != nil {
-			if err2 := c.cleanup(ctx); err2 != nil {
-				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
-			}
-		}
-	}()
-
-	if err := c.prepare(); err != nil {
+	if err := c.prepareToStart(ctx, recursive); err != nil {
 		return err
 	}
 
-	if c.state.State == ContainerStateStopped {
-		// Reinitialize the container if we need to
-		if err := c.reinit(ctx); err != nil {
-			return err
-		}
-	} else if c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateExited {
-		// Or initialize it if necessary
-		if err := c.init(ctx); err != nil {
-			return err
-		}
-	}
-
 	// Start the container
 	return c.start()
 }
 
-// StartAndAttach starts a container and attaches to it
-// StartAndAttach can start configured, created or stopped containers
+// StartAndAttach starts a container and attaches to it.
+// StartAndAttach can start configured, created or stopped containers.
 // For configured containers, the container will be initialized first, then
-// started
+// started.
 // Stopped containers will be deleted and re-created in runc, undergoing a fresh
-// Init()
+// Init().
 // If successful, an error channel will be returned containing the result of the
 // attach call.
 // The channel will be closed automatically after the result of attach has been
-// sent
-func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize) (attachResChan <-chan error, err error) {
+// sent.
+// If recursive is set, StartAndAttach will also start all containers this container depends on.
+func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, recursive bool) (attachResChan <-chan error, err error) {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -150,48 +103,10 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 		}
 	}
 
-	// Container must be created or stopped to be started
-	if !(c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateCreated ||
-		c.state.State == ContainerStateStopped ||
-		c.state.State == ContainerStateExited) {
-		return nil, errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID())
-	}
-
-	notRunning, err := c.checkDependenciesRunning()
-	if err != nil {
-		return nil, errors.Wrapf(err, "error checking dependencies for container %s", c.ID())
-	}
-	if len(notRunning) > 0 {
-		depString := strings.Join(notRunning, ",")
-		return nil, errors.Wrapf(ErrCtrStateInvalid, "some dependencies of container %s are not started: %s", c.ID(), depString)
-	}
-
-	defer func() {
-		if err != nil {
-			if err2 := c.cleanup(ctx); err2 != nil {
-				logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2)
-			}
-		}
-	}()
-
-	if err := c.prepare(); err != nil {
+	if err := c.prepareToStart(ctx, recursive); err != nil {
 		return nil, err
 	}
 
-	if c.state.State == ContainerStateStopped {
-		// Reinitialize the container if we need to
-		if err := c.reinit(ctx); err != nil {
-			return nil, err
-		}
-	} else if c.state.State == ContainerStateConfigured ||
-		c.state.State == ContainerStateExited {
-		// Or initialize it if necessary
-		if err := c.init(ctx); err != nil {
-			return nil, err
-		}
-	}
-
 	attachChan := make(chan error)
 
 	// Attach to the container before starting it
@@ -205,6 +120,24 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 	return attachChan, nil
 }
 
+// RestartWithTimeout restarts a running container and takes a given timeout in uint
+func (c *Container) RestartWithTimeout(ctx context.Context, timeout uint) (err error) {
+	if !c.batched {
+		c.lock.Lock()
+		defer c.lock.Unlock()
+
+		if err := c.syncContainer(); err != nil {
+			return err
+		}
+	}
+
+	if err = c.checkDependenciesAndHandleError(ctx); err != nil {
+		return err
+	}
+
+	return c.restartWithTimeout(ctx, timeout)
+}
+
 // Stop uses the container's stop signal (or SIGTERM if no signal was specified)
 // to stop the container, and if it has not stopped after container's stop
 // timeout, SIGKILL is used to attempt to forcibly stop the container
@@ -730,28 +663,6 @@ func (c *Container) Sync() error {
 	return nil
 }
 
-// RestartWithTimeout restarts a running container and takes a given timeout in uint
-func (c *Container) RestartWithTimeout(ctx context.Context, timeout uint) (err error) {
-	if !c.batched {
-		c.lock.Lock()
-		defer c.lock.Unlock()
-
-		if err := c.syncContainer(); err != nil {
-			return err
-		}
-	}
-
-	notRunning, err := c.checkDependenciesRunning()
-	if err != nil {
-		return errors.Wrapf(err, "error checking dependencies for container %s", c.ID())
-	}
-	if len(notRunning) > 0 {
-		depString := strings.Join(notRunning, ",")
-		return errors.Wrapf(ErrCtrStateInvalid, "some dependencies of container %s are not started: %s", c.ID(), depString)
-	}
-	return c.restartWithTimeout(ctx, timeout)
-}
-
 // Refresh refreshes a container's state in the database, restarting the
 // container if it is running
 func (c *Container) Refresh(ctx context.Context) error {