Change handling for pods in libpod state

Add new functions to update pods and add/remove containers from them
Use these new functions in place of manually modifying pods

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

Closes: #229
Approved by: rhatdan

libpod/container.go

@@ -114,7 +114,6 @@ func (ns LinuxNS) String() string {
 type Container struct {
 	config *ContainerConfig
 
-	pod         *Pod
 	runningSpec *spec.Spec
 
 	state *containerRuntimeInfo

libpod/in_memory_state.go

@@ -104,8 +104,7 @@ func (s *InMemoryState) HasContainer(id string) (bool, error) {
 }
 
 // AddContainer adds a container to the state
-// If the container belongs to a pod, the pod must already be present when the
-// container is added, and the container must be present in the pod
+// Containers in a pod cannot be added to the state
 func (s *InMemoryState) AddContainer(ctr *Container) error {
 	if !ctr.valid {
 		return errors.Wrapf(ErrCtrRemoved, "container with ID %s is not valid", ctr.ID())
@@ -116,17 +115,8 @@ func (s *InMemoryState) AddContainer(ctr *Container) error {
 		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in state", ctr.ID())
 	}
 
-	if ctr.pod != nil {
-		if _, ok := s.pods[ctr.pod.ID()]; !ok {
-			return errors.Wrapf(ErrNoSuchPod, "pod %s does not exist, cannot add container %s", ctr.pod.ID(), ctr.ID())
-		}
-
-		hasCtr, err := ctr.pod.HasContainer(ctr.ID())
-		if err != nil {
-			return errors.Wrapf(err, "error checking if container %s is present in pod %s", ctr.ID(), ctr.pod.ID())
-		} else if !hasCtr {
-			return errors.Wrapf(ErrNoSuchCtr, "container %s is not present in pod %s", ctr.ID(), ctr.pod.ID())
-		}
+	if ctr.config.Pod != "" {
+		return errors.Wrapf(ErrInvalidArg, "cannot add a container that is in a pod with AddContainer, use AddContainerToPod")
 	}
 
 	if err := s.ctrNameIndex.Reserve(ctr.Name(), ctr.ID()); err != nil {
@@ -346,6 +336,86 @@ func (s *InMemoryState) RemovePod(pod *Pod) error {
 	return nil
 }
 
+// UpdatePod updates a pod's state from the backing database
+// As in-memory states have no database this is a no-op
+func (s *InMemoryState) UpdatePod(pod *Pod) error {
+	return nil
+}
+
+// AddContainerToPod adds a container to the given pod, also adding it to the
+// state
+func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
+	if !pod.valid {
+		return errors.Wrapf(ErrPodRemoved, "pod %s is not valid and cannot be added to", pod.ID())
+	} else if !ctr.valid {
+		return errors.Wrapf(ErrCtrRemoved, "container %s is not valid and cannot be added to the pod", ctr.ID())
+	}
+
+	if ctr.config.Pod != pod.ID() {
+		return errors.Wrapf(ErrInvalidArg, "container %s is not in pod %s", ctr.ID(), pod.ID())
+	}
+
+	// Add container to pod
+	if err := pod.addContainer(ctr); err != nil {
+		return err
+	}
+
+	// Add container to state
+	_, ok := s.containers[ctr.ID()]
+	if ok {
+		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in state", ctr.ID())
+	}
+
+	if err := s.ctrNameIndex.Reserve(ctr.Name(), ctr.ID()); err != nil {
+		return errors.Wrapf(err, "error registering container name %s", ctr.Name())
+	}
+
+	if err := s.ctrIDIndex.Add(ctr.ID()); err != nil {
+		s.ctrNameIndex.Release(ctr.Name())
+		return errors.Wrapf(err, "error registering container ID %s", ctr.ID())
+	}
+
+	s.containers[ctr.ID()] = ctr
+
+	return nil
+}
+
+// RemoveContainerFromPod removes the given container from the given pod
+// The container is also removed from the state
+func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
+	if !pod.valid {
+		return errors.Wrapf(ErrPodRemoved, "pod %s is not valid and containers cannot be removed", pod.ID())
+	} else if !ctr.valid {
+		return errors.Wrapf(ErrCtrRemoved, "container %s is not valid and cannot be removed from the pod", ctr.ID())
+	}
+
+	// Is the container in the pod?
+	exists, err := pod.HasContainer(ctr.ID())
+	if err != nil {
+		return errors.Wrapf(err, "error checking for container %s in pod %s", ctr.ID(), pod.ID())
+	} else if !exists {
+		return errors.Wrapf(ErrNoSuchCtr, "no container %s in pod %s", ctr.ID(), pod.ID())
+	}
+
+	// Remove container from pod
+	if err := pod.removeContainer(ctr); err != nil {
+		return err
+	}
+
+	// Remove container from state
+	if _, ok := s.containers[ctr.ID()]; !ok {
+		return errors.Wrapf(ErrNoSuchCtr, "no container exists in state with ID %s", ctr.ID())
+	}
+
+	if err := s.ctrIDIndex.Delete(ctr.ID()); err != nil {
+		return errors.Wrapf(err, "error removing container ID from index")
+	}
+	delete(s.containers, ctr.ID())
+	s.ctrNameIndex.Release(ctr.Name())
+
+	return nil
+}
+
 // AllPods retrieves all pods currently in the state
 func (s *InMemoryState) AllPods() ([]*Pod, error) {
 	pods := make([]*Pod, 0, len(s.pods))

libpod/options.go

@@ -334,7 +334,6 @@ func (r *Runtime) WithPod(pod *Pod) CtrCreateOption {
 		}
 
 		ctr.config.Pod = pod.ID()
-		ctr.pod = pod
 
 		return nil
 	}

libpod/runtime_ctr.go

@@ -72,26 +72,23 @@ func (r *Runtime) NewContainer(rSpec *spec.Spec, options ...CtrCreateOption) (c
 		ctr.config.Mounts = append(ctr.config.Mounts, ctr.config.ShmDir)
 	}
 
-	// If the container is in a pod, add it to the pod
-	if ctr.pod != nil {
-		if err := ctr.pod.addContainer(ctr); err != nil {
-			return nil, errors.Wrapf(err, "error adding new container to pod %s", ctr.pod.ID())
+	// Add the container to the state
+	// TODO: May be worth looking into recovering from name/ID collisions here
+	if ctr.config.Pod != "" {
+		// Get the pod from state
+		pod, err := r.state.Pod(ctr.config.Pod)
+		if err != nil {
+			return nil, errors.Wrapf(err, "cannot add container %s to pod %s", ctr.ID(), ctr.config.Pod)
 		}
-	}
-	defer func() {
-		if err != nil && ctr.pod != nil {
-			if err2 := ctr.pod.removeContainer(ctr); err2 != nil {
-				logrus.Errorf("Error removing partially-created container from pod %s: %s", ctr.pod.ID(), err2)
-			}
-		}
-	}()
 
-	if err := r.state.AddContainer(ctr); err != nil {
-		// TODO: Might be worth making an effort to detect duplicate IDs and names
-		// We can recover from that by generating a new ID for the
-		// container
+		if err := r.state.AddContainerToPod(pod, ctr); err != nil {
 			return nil, errors.Wrapf(err, "error adding new container to state")
 		}
+	} else {
+		if err := r.state.AddContainer(ctr); err != nil {
+			return nil, errors.Wrapf(err, "error adding new container to state")
+		}
+	}
 
 	return ctr, nil
 }
@@ -161,25 +158,34 @@ func (r *Runtime) removeContainer(c *Container, force bool) error {
 		return err
 	}
 
+	// Remove the container from the state
+	if c.config.Pod != "" {
+		pod, err := r.state.Pod(c.config.Pod)
+		if err != nil {
+			return errors.Wrapf(err, "container %s is in pod %s, but pod cannot be retrieved", c.ID(), pod.ID())
+		}
+
+		if err := r.state.RemoveContainerFromPod(pod, c); err != nil {
+			return errors.Wrapf(err, "error removing container %s from state", c.ID())
+		}
+	} else {
 		if err := r.state.RemoveContainer(c); err != nil {
 			return errors.Wrapf(err, "error removing container from state")
 		}
+	}
 
 	// Delete the container
+	// Only do this if we're not ContainerStateConfigured - if we are,
+	// we haven't been created in runc yet
+	if c.state.State == ContainerStateConfigured {
 		if err := r.ociRuntime.deleteContainer(c); err != nil {
 			return errors.Wrapf(err, "error removing container %s from runc", c.ID())
 		}
+	}
 
 	// Set container as invalid so it can no longer be used
 	c.valid = false
 
-	// Remove container from pod, if it joined one
-	if c.pod != nil {
-		if err := c.pod.removeContainer(c); err != nil {
-			return errors.Wrapf(err, "error removing container from pod %s", c.pod.ID())
-		}
-	}
-
 	return nil
 }
 

libpod/sql_state.go

@@ -853,6 +853,21 @@ func (s *SQLState) RemovePod(pod *Pod) error {
 	return ErrNotImplemented
 }
 
+// UpdatePod updates a pod from the database
+func (s *SQLState) UpdatePod(pod *Pod) error {
+	return ErrNotImplemented
+}
+
+// AddContainerToPod adds a container to the given pod
+func (s *SQLState) AddContainerToPod(pod *Pod, ctr *Container) error {
+	return ErrNotImplemented
+}
+
+// RemoveContainerFromPods removes a container from the given pod
+func (s *SQLState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
+	return ErrNotImplemented
+}
+
 // AllPods retrieves all pods presently in the state
 func (s *SQLState) AllPods() ([]*Pod, error) {
 	return nil, ErrNotImplemented

libpod/state.go

@@ -16,9 +16,7 @@ type State interface {
 	// Checks if a container with the given ID is present in the state
 	HasContainer(id string) (bool, error)
 	// Adds container to state
-	// If the container belongs to a pod, that pod must already be present
-	// in the state when the container is added, and the container must be
-	// present in the pod
+	// The container cannot be part of a pod
 	AddContainer(ctr *Container) error
 	// Removes container from state
 	// The container will only be removed from the state, not from the pod
@@ -53,6 +51,14 @@ type State interface {
 	// Containers within a pod will not be removed from the state, and will
 	// not be changed to remove them from the now-removed pod
 	RemovePod(pod *Pod) error
+	// UpdatePod updates a pod's state from the backing store
+	UpdatePod(pod *Pod) error
+	// AddContainerToPod adds a container to an existing pod
+	// The container given will be added to the state and the pod
+	AddContainerToPod(pod *Pod, ctr *Container) error
+	// RemoveContainerFromPod removes a container from an existing pod
+	// The container will also be removed from the state
+	RemoveContainerFromPod(pod *Pod, ctr *Container) error
 	// Retrieves all pods presently in state
 	AllPods() ([]*Pod, error)
 }