Tear out pod containers map. Instead rely on state

This ensures that there is only one canonical place where
containers in a pod are stored, in the state itself.

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

Closes: #268
Approved by: rhatdan

5 files changed, 154 insertions, 117 deletions

diff --git a/libpod/in_memory_state.go b/libpod/in_memory_state.go
index 55162e6c8..2f0d2ec70 100644
--- a/libpod/in_memory_state.go
+++ b/libpod/in_memory_state.go
@@ -8,15 +8,20 @@ import (
 	"github.com/projectatomic/libpod/pkg/registrar"
 )
 
+// TODO: unified name/ID registry to ensure no name and ID conflicts between
+// containers and pods
+// This can probably be used to replace the existing trunc index and registrars
+
 // An InMemoryState is a purely in-memory state store
 type InMemoryState struct {
-	pods         map[string]*Pod
-	containers   map[string]*Container
-	ctrDepends   map[string][]string
-	podNameIndex *registrar.Registrar
-	podIDIndex   *truncindex.TruncIndex
-	ctrNameIndex *registrar.Registrar
-	ctrIDIndex   *truncindex.TruncIndex
+	pods          map[string]*Pod
+	containers    map[string]*Container
+	ctrDepends    map[string][]string
+	podContainers map[string]map[string]*Container
+	podNameIndex  *registrar.Registrar
+	podIDIndex    *truncindex.TruncIndex
+	ctrNameIndex  *registrar.Registrar
+	ctrIDIndex    *truncindex.TruncIndex
 }
 
 // NewInMemoryState initializes a new, empty in-memory state
@@ -28,6 +33,8 @@ func NewInMemoryState() (State, error) {
 
 	state.ctrDepends = make(map[string][]string)
 
+	state.podContainers = make(map[string]map[string]*Container)
+
 	state.podNameIndex = registrar.NewRegistrar()
 	state.ctrNameIndex = registrar.NewRegistrar()
 
@@ -293,22 +300,73 @@ func (s *InMemoryState) HasPod(id string) (bool, error) {
 	return ok, nil
 }
 
-// PodContainers retrieves the containers from a pod given the pod's full ID
-func (s *InMemoryState) PodContainers(id string) ([]*Container, error) {
-	if id == "" {
-		return nil, ErrEmptyID
+// PodHasContainer checks if the given pod has a container with the given ID
+func (s *InMemoryState) PodHasContainer(pod *Pod, ctrID string) (bool, error) {
+	if !pod.valid {
+		return false, errors.Wrapf(ErrPodRemoved, "pod %s is not valid")
 	}
 
-	pod, ok := s.pods[id]
+	podCtrs, ok := s.podContainers[pod.ID()]
+	if !ok {
+		pod.valid = false
+		return false, errors.Wrapf(ErrNoSuchPod, "no pod with ID %s found in state", pod.ID())
+	}
+
+	_, ok = podCtrs[ctrID]
+	return ok, nil
+}
+
+// PodContainersByID returns the IDs of all containers in the given pod
+func (s *InMemoryState) PodContainersByID(pod *Pod) ([]string, error) {
+	if !pod.valid {
+		return nil, errors.Wrapf(ErrPodRemoved, "pod %s is not valid")
+	}
+
+	podCtrs, ok := s.podContainers[pod.ID()]
 	if !ok {
-		return nil, errors.Wrapf(ErrNoSuchPod, "no pod with ID %s found", id)
+		pod.valid = false
+		return nil, errors.Wrapf(ErrNoSuchPod, "no pod with ID %s found in state", pod.ID())
+	}
+
+	length := len(podCtrs)
+	if length == 0 {
+		return []string{}, nil
 	}
 
-	return pod.GetContainers()
+	ctrs := make([]string, 0, length)
+	for _, ctr := range podCtrs {
+		ctrs = append(ctrs, ctr.ID())
+	}
+
+	return ctrs, nil
+}
+
+// PodContainers retrieves the containers from a pod
+func (s *InMemoryState) PodContainers(pod *Pod) ([]*Container, error) {
+	if !pod.valid {
+		return nil, errors.Wrapf(ErrPodRemoved, "pod %s is not valid")
+	}
+
+	podCtrs, ok := s.podContainers[pod.ID()]
+	if !ok {
+		pod.valid = false
+		return nil, errors.Wrapf(ErrNoSuchPod, "no pod with ID %s found in state", pod.ID())
+	}
+
+	length := len(podCtrs)
+	if length == 0 {
+		return []*Container{}, nil
+	}
+
+	ctrs := make([]*Container, 0, length)
+	for _, ctr := range podCtrs {
+		ctrs = append(ctrs, ctr)
+	}
+
+	return ctrs, nil
 }
 
 // AddPod adds a given pod to the state
-// Only empty pods can be added to the state
 func (s *InMemoryState) AddPod(pod *Pod) error {
 	if !pod.valid {
 		return errors.Wrapf(ErrPodRemoved, "pod %s is not valid and cannot be added", pod.ID())
@@ -318,8 +376,8 @@ func (s *InMemoryState) AddPod(pod *Pod) error {
 		return errors.Wrapf(ErrPodExists, "pod with ID %s already exists in state", pod.ID())
 	}
 
-	if len(pod.containers) != 0 {
-		return errors.Wrapf(ErrInternal, "only empty pods can be added to the state")
+	if _, ok := s.podContainers[pod.ID()]; ok {
+		return errors.Wrapf(ErrPodExists, "pod with ID %s already exists in state", pod.ID())
 	}
 
 	if err := s.podNameIndex.Reserve(pod.Name(), pod.ID()); err != nil {
@@ -333,11 +391,13 @@ func (s *InMemoryState) AddPod(pod *Pod) error {
 
 	s.pods[pod.ID()] = pod
 
+	s.podContainers[pod.ID()] = make(map[string]*Container)
+
 	return nil
 }
 
 // RemovePod removes a given pod from the state
-// Containers within the pod will not be removed or changed
+// Only empty pods can be removed
 func (s *InMemoryState) RemovePod(pod *Pod) error {
 	// Don't make many validity checks to ensure we can kick badly formed
 	// pods out of the state
@@ -345,22 +405,24 @@ func (s *InMemoryState) RemovePod(pod *Pod) error {
 	if _, ok := s.pods[pod.ID()]; !ok {
 		return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID())
 	}
+	podCtrs, ok := s.podContainers[pod.ID()]
+	if !ok {
+		return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID())
+	}
+	if len(podCtrs) != 0 {
+		return errors.Wrapf(ErrCtrExists, "pod %s is not empty and cannot be removed", pod.ID())
+	}
 
 	if err := s.podIDIndex.Delete(pod.ID()); err != nil {
 		return errors.Wrapf(err, "error removing pod ID %s from index", pod.ID())
 	}
 	delete(s.pods, pod.ID())
+	delete(s.podContainers, pod.ID())
 	s.podNameIndex.Release(pod.Name())
 
 	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 {
@@ -375,13 +437,20 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
 		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
+	// Retrieve pod containers list
+	podCtrs, ok := s.podContainers[pod.ID()]
+	if !ok {
+		pod.valid = false
+		return errors.Wrapf(ErrPodRemoved, "pod %s not found in state", pod.ID())
+	}
+
+	// Is the container already in the pod?
+	if _, ok := podCtrs[ctr.ID()]; ok {
+		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in pod %s", ctr.ID(), pod.ID())
 	}
 
 	// Add container to state
-	_, ok := s.containers[ctr.ID()]
+	_, ok = s.containers[ctr.ID()]
 	if ok {
 		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in state", ctr.ID())
 	}
@@ -397,6 +466,9 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
 
 	s.containers[ctr.ID()] = ctr
 
+	// Add container to pod containers
+	podCtrs[ctr.ID()] = ctr
+
 	return nil
 }
 
@@ -410,18 +482,16 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
 		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())
-	}
-	if !exists {
-		return errors.Wrapf(ErrNoSuchCtr, "no container %s in pod %s", ctr.ID(), pod.ID())
+	// Retrieve pod containers
+	podCtrs, ok := s.podContainers[pod.ID()]
+	if !ok {
+		pod.valid = false
+		return errors.Wrapf(ErrPodRemoved, "pod %s has been removed", pod.ID())
 	}
 
-	// Remove container from pod
-	if err := pod.removeContainer(ctr); err != nil {
-		return err
+	// Is the container in the pod?
+	if _, ok := podCtrs[ctr.ID()]; !ok {
+		return errors.Wrapf(ErrNoSuchCtr, "container with ID %s not found in pod %s", ctr.ID(), pod.ID())
 	}
 
 	// Remove container from state
@@ -435,6 +505,9 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
 	delete(s.containers, ctr.ID())
 	s.ctrNameIndex.Release(ctr.Name())
 
+	// Remove the container from the pod
+	delete(podCtrs, ctr.ID())
+
 	return nil
 }
 
diff --git a/libpod/pod.go b/libpod/pod.go
index 15899930b..91636546d 100644
--- a/libpod/pod.go
+++ b/libpod/pod.go
@@ -15,10 +15,9 @@ type Pod struct {
 	name   string
 	labels map[string]string
 
-	containers map[string]*Container
-
-	valid bool
-	lock  storage.Locker
+	valid   bool
+	runtime *Runtime
+	lock storage.Locker
 }
 
 // ID retrieves the pod's ID
@@ -42,17 +41,11 @@ func (p *Pod) Labels() map[string]string {
 }
 
 // Creates a new, empty pod
-func newPod(lockDir string) (*Pod, error) {
+func newPod(lockDir string, runtime *Runtime) (*Pod, error) {
 	pod := new(Pod)
 	pod.id = stringid.GenerateNonCryptoID()
 	pod.name = namesgenerator.GetRandomName(0)
-
-	pod.containers = make(map[string]*Container)
-
-	// TODO: containers and pods share a locks folder, but not tables in the
-	// database
-	// As the locks are 256-bit pseudorandom integers, collision is unlikely
-	// But it's something worth looking into
+	pod.runtime = runtime
 
 	// Path our lock file will reside at
 	lockPath := filepath.Join(lockDir, pod.id)
@@ -66,47 +59,9 @@ func newPod(lockDir string) (*Pod, error) {
 	return pod, nil
 }
 
-// Adds a container to the pod
-// Does not check that container's pod ID is set correctly, or attempt to set
-// pod ID after adding
-func (p *Pod) addContainer(ctr *Container) error {
-	p.lock.Lock()
-	defer p.lock.Unlock()
-
-	if !p.valid {
-		return ErrPodRemoved
-	}
-
-	if !ctr.valid {
-		return ErrCtrRemoved
-	}
-
-	if _, ok := p.containers[ctr.ID()]; ok {
-		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in pod %s", ctr.ID(), p.id)
-	}
-
-	p.containers[ctr.ID()] = ctr
-
-	return nil
-}
-
-// Removes a container from the pod
-// Does not perform any checks on the container
-func (p *Pod) removeContainer(ctr *Container) error {
-	p.lock.Lock()
-	defer p.lock.Unlock()
-
-	if !p.valid {
-		return ErrPodRemoved
-	}
-
-	if _, ok := p.containers[ctr.ID()]; !ok {
-		return errors.Wrapf(ErrNoSuchCtr, "no container with id %s in pod %s", ctr.ID(), p.id)
-	}
-
-	delete(p.containers, ctr.ID())
-
-	return nil
+// Init() initializes all containers within a pod that have not been initialized
+func (p *Pod) Init() error {
+	return ErrNotImplemented
 }
 
 // Start starts all containers within a pod that are not already running
@@ -126,20 +81,15 @@ func (p *Pod) Kill(signal uint) error {
 
 // HasContainer checks if a container is present in the pod
 func (p *Pod) HasContainer(id string) (bool, error) {
-	p.lock.Lock()
-	defer p.lock.Unlock()
-
 	if !p.valid {
 		return false, ErrPodRemoved
 	}
 
-	_, ok := p.containers[id]
-
-	return ok, nil
+	return p.runtime.state.PodHasContainer(p, id)
 }
 
-// GetContainers retrieves the containers in the pod
-func (p *Pod) GetContainers() ([]*Container, error) {
+// AllContainersID returns the container IDs of all the containers in the pod
+func (p *Pod) AllContainersByID() ([]string, error) {
 	p.lock.Lock()
 	defer p.lock.Unlock()
 
@@ -147,12 +97,19 @@ func (p *Pod) GetContainers() ([]*Container, error) {
 		return nil, ErrPodRemoved
 	}
 
-	ctrs := make([]*Container, 0, len(p.containers))
-	for _, ctr := range p.containers {
-		ctrs = append(ctrs, ctr)
+	return p.runtime.state.PodContainersByID(p)
+}
+
+// AllContainers retrieves the containers in the pod
+func (p *Pod) AllContainers() ([]*Container, error) {
+	p.lock.Lock()
+	defer p.lock.Unlock()
+
+	if !p.valid {
+		return nil, ErrPodRemoved
 	}
 
-	return ctrs, nil
+	return p.runtime.state.PodContainers(p)
 }
 
 // Status gets the status of all containers in the pod
diff --git a/libpod/runtime_pod.go b/libpod/runtime_pod.go
index 35c7f0642..d471b903f 100644
--- a/libpod/runtime_pod.go
+++ b/libpod/runtime_pod.go
@@ -24,7 +24,7 @@ func (r *Runtime) NewPod(options ...PodCreateOption) (*Pod, error) {
 		return nil, ErrRuntimeStopped
 	}
 
-	pod, err := newPod(r.lockDir)
+	pod, err := newPod(r.lockDir, r)
 	if err != nil {
 		return nil, errors.Wrapf(err, "error creating pod")
 	}
diff --git a/libpod/sql_state.go b/libpod/sql_state.go
index c10975d47..5c843ad6f 100644
--- a/libpod/sql_state.go
+++ b/libpod/sql_state.go
@@ -839,8 +839,20 @@ func (s *SQLState) HasPod(id string) (bool, error) {
 	return false, ErrNotImplemented
 }
 
+// PodHasContainer checks if the given pod containers a container with the given
+// ID
+func (s *SQLState) PodHasContainer(pod *Pod, ctrID string) (bool, error) {
+	return false, ErrNotImplemented
+}
+
+// PodContainersByID returns the container IDs of all containers in the given
+// pod
+func (s *SQLState) PodContainersByID(pod *Pod) ([]string, error) {
+	return nil, ErrNotImplemented
+}
+
 // PodContainers returns all the containers in a pod given the pod's full ID
-func (s *SQLState) PodContainers(id string) ([]*Container, error) {
+func (s *SQLState) PodContainers(pod *Pod) ([]*Container, error) {
 	return nil, ErrNotImplemented
 }
 
@@ -856,11 +868,6 @@ 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
diff --git a/libpod/state.go b/libpod/state.go
index 01ae58bd1..3e5da9306 100644
--- a/libpod/state.go
+++ b/libpod/state.go
@@ -42,17 +42,17 @@ type State interface {
 	LookupPod(idOrName string) (*Pod, error)
 	// Checks if a pod with the given ID is present in the state
 	HasPod(id string) (bool, error)
-	// Get all the containers in a pod. Accepts full ID of pod.
-	PodContainers(id string) ([]*Container, error)
+	// Check if a pod has a container with the given ID
+	PodHasContainer(pod *Pod, ctrID string) (bool, error)
+	// Get the IDs of all containers in a pod
+	PodContainersByID(pod *Pod) ([]string, error)
+	// Get all the containers in a pod
+	PodContainers(pod *Pod) ([]*Container, error)
 	// Adds pod to state
-	// Only empty pods can be added to the state
 	AddPod(pod *Pod) error
 	// Removes pod from state
-	// Containers within a pod will not be removed from the state, and will
-	// not be changed to remove them from the now-removed pod
+	// Only empty pods can be removed from the state
 	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