pod infra container is started before a container is run.

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>

cmd/podman/attach.go

@@ -75,7 +75,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 {
+	// TODO FIXME recursive?
+	if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false, false); err != nil {
 		return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
 	}
 

cmd/podman/play_kube.go

@@ -153,7 +153,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

@@ -70,10 +70,17 @@ func runCmd(c *cliconfig.RunValues) error {
 		}
 	}
 
+	var recursive bool
+	if c.IsSet("pod") {
+		recursive = true
+	} else {
+		recursive = false
+	}
+
 	ctx := getContext()
 	// Handle detached start
 	if createConfig.Detach {
-		if err := ctr.Start(ctx); err != nil {
+		if err := ctr.Start(ctx, recursive); err != nil {
 			// This means the command did not exist
 			exitCode = 127
 			if strings.Index(err.Error(), "permission denied") > -1 {
@@ -118,7 +125,7 @@ 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 err := startAttachCtr(ctr, outputStream, errorStream, inputStream, c.String("detach-keys"), c.Bool("sig-proxy"), true, recursive); err != nil {
 		// This means the command did not exist
 		exitCode = 127
 		if strings.Index(err.Error(), "permission denied") > -1 {

cmd/podman/start.go

@@ -108,7 +108,7 @@ 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)
+			err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning, false)
 			if ctrRunning {
 				return err
 			}
@@ -130,7 +130,7 @@ func startCmd(c *cliconfig.StartValues) error {
 			continue
 		}
 		// Handle non-attach start
-		if err := ctr.Start(ctx); err != nil {
+		if err := ctr.Start(ctx, false); 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
 	}

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

@@ -38,24 +38,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
 	}
 
@@ -74,7 +65,8 @@ func (c *Container) Init(ctx context.Context) (err error) {
 // started
 // Stopped containers will be deleted and re-created in runc, undergoing a fresh
 // Init()
-func (c *Container) Start(ctx context.Context) (err error) {
+// if recursive is set, Start will also start all containers this ctr depends on
+func (c *Container) Start(ctx context.Context, recursive bool) (err error) {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -83,7 +75,55 @@ func (c *Container) Start(ctx context.Context) (err error) {
 			return err
 		}
 	}
+	if err := c.prepareToStart(ctx, recursive); 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
+// For configured containers, the container will be initialized first, then
+// started
+// Stopped containers will be deleted and re-created in runc, undergoing a fresh
+// 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
+// if recursive is set, StartAndAttach will also start all containers this ctr 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()
+
+		if err := c.syncContainer(); err != nil {
+			return nil, err
+		}
+	}
+
+	if err := c.prepareToStart(ctx, recursive); err != nil {
+		return nil, err
+	}
+
+	attachChan := make(chan error)
+
+	// Attach to the container before starting it
+	go func() {
+		if err := c.attach(streams, keys, resize, true); err != nil {
+			attachChan <- err
+		}
+		close(attachChan)
+	}()
 
+	return attachChan, nil
+}
+
+// helper function that checks the container is in the right state, then initializes the container
+// in preparation to start the container
+func (c *Container) prepareToStart(ctx context.Context, recursive bool) (err error) {
 	// Container must be created or stopped to be started
 	if !(c.state.State == ContainerStateConfigured ||
 		c.state.State == ContainerStateCreated ||
@@ -92,13 +132,14 @@ func (c *Container) Start(ctx context.Context) (err error) {
 		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)
+	if !recursive {
+		if err := c.checkDependenciesAndHandleError(ctx); err != nil {
+			return err
+		}
+	} else {
+		if err := c.startDependencies(ctx); err != nil {
+			return err
+		}
 	}
 
 	defer func() {
@@ -125,84 +166,80 @@ func (c *Container) Start(ctx context.Context) (err error) {
 			return err
 		}
 	}
-
-	// Start the container
-	return c.start()
+	return nil
 }
 
-// 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
-// Stopped containers will be deleted and re-created in runc, undergoing a fresh
-// 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) {
+// 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 nil, err
+			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 nil, errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID())
+	if err = c.checkDependenciesAndHandleError(ctx); err != nil {
+		return err
 	}
 
+	return c.restartWithTimeout(ctx, timeout)
+}
+
+// helper function that handles dependencies and prints a helpful message
+func (c *Container) checkDependenciesAndHandleError(ctx context.Context) error {
 	notRunning, err := c.checkDependenciesRunning()
 	if err != nil {
-		return nil, errors.Wrapf(err, "error checking dependencies for container %s", c.ID())
+		return 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)
+		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)
-			}
-		}
-	}()
+	return nil
+}
 
-	if err := c.prepare(); err != nil {
-		return nil, err
+func (c *Container) startDependencies(ctx context.Context) error {
+	depCtrIDs := c.Dependencies()
+	if len(depCtrIDs) == 0 {
+		return nil
 	}
 
-	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
+	depCtrs := make([]*Container, 0) //, len(depCtrIDs))
+	for _, ctrID := range depCtrIDs {
+		ctr, err := c.runtime.state.LookupContainer(ctrID)
+		if err != nil {
+			return errors.Wrapf(err, "error starting dependency for container %s. container %s not found", c.ID(), ctrID)
 		}
+		depCtrs = append(depCtrs, ctr)
 	}
 
-	attachChan := make(chan error)
+	// Build a dependency graph of containers
+	graph, err := buildContainerGraph(depCtrs)
+	if err != nil {
+		return errors.Wrapf(err, "error generating dependency graph for container %s", c.ID())
+	}
 
-	// Attach to the container before starting it
-	go func() {
-		if err := c.attach(streams, keys, resize, true); err != nil {
-			attachChan <- err
-		}
-		close(attachChan)
-	}()
+	ctrErrors := make(map[string]error)
+	ctrsVisited := make(map[string]bool)
 
-	return attachChan, nil
+	// If there are no containers without dependencies, we can't start
+	// Error out
+	if len(graph.noDepNodes) == 0 {
+		return errors.Wrapf(ErrNoSuchCtr, "All dependencies have dependencies of %s", c.ID())
+	}
+
+	// Traverse the graph beginning at nodes with no dependencies
+	for _, node := range graph.noDepNodes {
+		startNode(ctx, node, false, ctrErrors, ctrsVisited, true)
+	}
+
+	if len(ctrErrors) > 0 {
+		return errors.Wrapf(ErrCtrExists, "error stopping some containers")
+	}
+	return nil
 }
 
 // Stop uses the container's stop signal (or SIGTERM if no signal was specified)
@@ -730,28 +767,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 {