Refactor dependency checks from init() into public API

Instead of checking during init(), which could result in major
locking issues when used with pods, make our dependency checks in
the public API instead. This avoids doing them when we start pods
(where, because of the dependency graph, we can reasonably say
all dependencies are up before we start a container).

Signed-off-by: Matthew Heon <matthew.heon@gmail.com>

Closes: #577
Approved by: rhatdan

1 files changed, 56 insertions, 22 deletions

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index 523d088d5..f75df8c28 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -350,40 +350,72 @@ func (c *Container) save() error {
 	return nil
 }
 
-// Initialize a container, creating it in the runtime
-func (c *Container) init() error {
+// Check if a container's dependencies are running
+// Returns a []string containing the IDs of dependencies that are not running
+func (c *Container) checkDependenciesRunning() ([]string, error) {
 	deps := c.Dependencies()
+	notRunning := []string{}
+
+	// We were not passed a set of dependency containers
+	// Make it ourselves
 	depCtrs := make(map[string]*Container, len(deps))
-	// Ensure that dependencies are up before we begin
 	for _, dep := range deps {
 		// Get the dependency container
 		depCtr, err := c.runtime.state.Container(dep)
 		if err != nil {
-			return errors.Wrapf(err, "error retrieving dependency %s of container %s from state", dep, c.ID())
+			return nil, errors.Wrapf(err, "error retrieving dependency %s of container %s from state", dep, c.ID())
 		}
 
 		// Check the status
 		state, err := depCtr.State()
 		if err != nil {
-			return errors.Wrapf(err, "error retrieving state of dependency %s of container %s", dep, c.ID())
+			return nil, errors.Wrapf(err, "error retrieving state of dependency %s of container %s", dep, c.ID())
 		}
 		if state != ContainerStateRunning {
-			return errors.Wrapf(ErrCtrStateInvalid, "dependency %s of container %s is not running, cannot initialize container", dep, c.ID())
+			notRunning = append(notRunning, dep)
 		}
 		depCtrs[dep] = depCtr
+	}
+
+	return notRunning, nil
+}
+
+// Check if a container's dependencies are running
+// Returns a []string containing the IDs of dependencies that are not running
+// Assumes depencies are already locked, and will be passed in
+// Accepts a map[string]*Container containing, at a minimum, the locked
+// dependency containers
+// (This must be a map from container ID to container)
+func (c *Container) checkDependenciesRunningLocked(depCtrs map[string]*Container) ([]string, error) {
+	deps := c.Dependencies()
+	notRunning := []string{}
+
+	for _, dep := range deps {
+		depCtr, ok := depCtrs[dep]
+		if !ok {
+			return nil, errors.Wrapf(ErrNoSuchCtr, "container %s depends on container %s but it is not on containers passed to checkDependenciesRunning", c.ID(), dep)
+		}
 
-		// There is potential race here
-		// A dep container may stop before this container starts
-		// We can't really control this, but it probably can't cause
-		// anything insidious.
+		if err := c.syncContainer(); err != nil {
+			return nil, err
+		}
+
+		if depCtr.state.State != ContainerStateRunning {
+			notRunning = append(notRunning, dep)
+		}
 	}
 
+	return notRunning, nil
+}
+
+// Initialize a container, creating it in the runtime
+func (c *Container) init() error {
 	if err := c.makeBindMounts(); err != nil {
 		return err
 	}
 
 	// Generate the OCI spec
-	spec, err := c.generateSpec(depCtrs)
+	spec, err := c.generateSpec()
 	if err != nil {
 		return err
 	}
@@ -875,7 +907,7 @@ func (c *Container) generateHosts() (string, error) {
 
 // Generate spec for a container
 // Accepts a map of the container's dependencies
-func (c *Container) generateSpec(deps map[string]*Container) (*spec.Spec, error) {
+func (c *Container) generateSpec() (*spec.Spec, error) {
 	g := generate.NewFromSpec(c.config.Spec)
 
 	// If network namespace was requested, add it now
@@ -921,37 +953,37 @@ func (c *Container) generateSpec(deps map[string]*Container) (*spec.Spec, error)
 
 	// Add shared namespaces from other containers
 	if c.config.IPCNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, IPCNS, deps[c.config.IPCNsCtr], spec.IPCNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, IPCNS, c.config.IPCNsCtr, spec.IPCNamespace); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.MountNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, MountNS, deps[c.config.MountNsCtr], spec.MountNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, MountNS, c.config.MountNsCtr, spec.MountNamespace); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.NetNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, NetNS, deps[c.config.NetNsCtr], spec.NetworkNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, NetNS, c.config.NetNsCtr, spec.NetworkNamespace); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.PIDNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, PIDNS, deps[c.config.PIDNsCtr], string(spec.PIDNamespace)); err != nil {
+		if err := c.addNamespaceContainer(&g, PIDNS, c.config.PIDNsCtr, string(spec.PIDNamespace)); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.UserNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, UserNS, deps[c.config.UserNsCtr], spec.UserNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, UserNS, c.config.UserNsCtr, spec.UserNamespace); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.UTSNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, UTSNS, deps[c.config.UTSNsCtr], spec.UTSNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, UTSNS, c.config.UTSNsCtr, spec.UTSNamespace); err != nil {
 			return nil, err
 		}
 	}
 	if c.config.CgroupNsCtr != "" {
-		if err := c.addNamespaceContainer(&g, CgroupNS, deps[c.config.CgroupNsCtr], spec.CgroupNamespace); err != nil {
+		if err := c.addNamespaceContainer(&g, CgroupNS, c.config.CgroupNsCtr, spec.CgroupNamespace); err != nil {
 			return nil, err
 		}
 	}
@@ -979,11 +1011,13 @@ func (c *Container) generateSpec(deps map[string]*Container) (*spec.Spec, error)
 }
 
 // Add an existing container's namespace to the spec
-func (c *Container) addNamespaceContainer(g *generate.Generator, ns LinuxNS, nsCtr *Container, specNS string) error {
-	if nsCtr == nil {
-		return errors.Wrapf(ErrNoSuchCtr, "nil container passed to addNamespaceContainer")
+func (c *Container) addNamespaceContainer(g *generate.Generator, ns LinuxNS, ctr string, specNS string) error {
+	nsCtr, err := c.runtime.state.Container(ctr)
+	if err != nil {
+		return errors.Wrapf(err, "error retrieving dependency %s of container %s from state", ctr, c.ID())
 	}
 
+	// TODO need unlocked version of this for use in pods
 	nsPath, err := nsCtr.NamespacePath(ns)
 	if err != nil {
 		return err