Fix bugs identified by unit tests

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

Closes: #268
Approved by: rhatdan

libpod/in_memory_state.go

@@ -123,6 +123,17 @@ func (s *InMemoryState) AddContainer(ctr *Container) error {
 		return errors.Wrapf(ErrInvalidArg, "cannot add a container that is in a pod with AddContainer, use AddContainerToPod")
 	}
 
+	// There are potential race conditions with this
+	// But in-memory state is intended purely for testing and not production
+	// use, so this should be fine.
+	depCtrs := ctr.Dependencies()
+	for _, depCtr := range depCtrs {
+		_, ok = s.containers[depCtr]
+		if !ok {
+			return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depCtr)
+		}
+	}
+
 	if err := s.nameIndex.Reserve(ctr.Name(), ctr.ID()); err != nil {
 		return errors.Wrapf(err, "error registering container name %s", ctr.Name())
 	}
@@ -135,7 +146,6 @@ func (s *InMemoryState) AddContainer(ctr *Container) error {
 	s.containers[ctr.ID()] = ctr
 
 	// Add containers this container depends on
-	depCtrs := ctr.Dependencies()
 	for _, depCtr := range depCtrs {
 		s.addCtrToDependsMap(ctr.ID(), depCtr)
 	}
@@ -157,6 +167,7 @@ func (s *InMemoryState) RemoveContainer(ctr *Container) error {
 	}
 
 	if _, ok := s.containers[ctr.ID()]; !ok {
+		ctr.valid = false
 		return errors.Wrapf(ErrNoSuchCtr, "no container exists in state with ID %s", ctr.ID())
 	}
 
@@ -301,6 +312,10 @@ func (s *InMemoryState) PodHasContainer(pod *Pod, ctrID string) (bool, error) {
 		return false, errors.Wrapf(ErrPodRemoved, "pod %s is not valid")
 	}
 
+	if ctrID == "" {
+		return false, ErrEmptyID
+	}
+
 	podCtrs, ok := s.podContainers[pod.ID()]
 	if !ok {
 		pod.valid = false
@@ -398,10 +413,12 @@ func (s *InMemoryState) RemovePod(pod *Pod) error {
 	// pods out of the state
 
 	if _, ok := s.pods[pod.ID()]; !ok {
+		pod.valid = false
 		return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID())
 	}
 	podCtrs, ok := s.podContainers[pod.ID()]
 	if !ok {
+		pod.valid = false
 		return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID())
 	}
 	if len(podCtrs) != 0 {
@@ -430,16 +447,14 @@ func (s *InMemoryState) RemovePodContainers(pod *Pod) error {
 	// Get pod containers
 	podCtrs, ok := s.podContainers[pod.ID()]
 	if !ok {
+		pod.valid = false
 		return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID())
 	}
 
 	// Go through container dependencies. Check to see if any are outside the pod.
 	for ctr := range podCtrs {
 		ctrDeps, ok := s.ctrDepends[ctr]
-		if !ok {
+		if ok {
-			return errors.Wrapf(ErrInternal, "cannot find dependencies for container %s", ctr)
-		}
-
 			for _, dep := range ctrDeps {
 				_, ok := podCtrs[dep]
 				if !ok {
@@ -447,6 +462,7 @@ func (s *InMemoryState) RemovePodContainers(pod *Pod) error {
 				}
 			}
 		}
+	}
 
 	// All dependencies are OK to remove
 	// Remove all containers
@@ -490,6 +506,17 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
 		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in pod %s", ctr.ID(), pod.ID())
 	}
 
+	// There are potential race conditions with this
+	// But in-memory state is intended purely for testing and not production
+	// use, so this should be fine.
+	depCtrs := ctr.Dependencies()
+	for _, depCtr := range depCtrs {
+		_, ok = s.containers[depCtr]
+		if !ok {
+			return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depCtr)
+		}
+	}
+
 	// Add container to state
 	_, ok = s.containers[ctr.ID()]
 	if ok {
@@ -510,6 +537,11 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
 	// Add container to pod containers
 	podCtrs[ctr.ID()] = ctr
 
+	// Add containers this container depends on
+	for _, depCtr := range depCtrs {
+		s.addCtrToDependsMap(ctr.ID(), depCtr)
+	}
+
 	return nil
 }
 
@@ -523,6 +555,13 @@ 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())
 	}
 
+	// Ensure we don't remove a container which other containers depend on
+	deps, ok := s.ctrDepends[ctr.ID()]
+	if ok && len(deps) != 0 {
+		depsStr := strings.Join(deps, ", ")
+		return errors.Wrapf(ErrCtrExists, "the following containers depend on container %s: %s", ctr.ID(), depsStr)
+	}
+
 	// Retrieve pod containers
 	podCtrs, ok := s.podContainers[pod.ID()]
 	if !ok {
@@ -530,6 +569,12 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
 		return errors.Wrapf(ErrPodRemoved, "pod %s has been removed", pod.ID())
 	}
 
+	// Does the container exist?
+	if _, ok := s.containers[ctr.ID()]; !ok {
+		ctr.valid = false
+		return errors.Wrapf(ErrNoSuchCtr, "container %s does not exist in state", ctr.ID())
+	}
+
 	// 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())
@@ -549,6 +594,12 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
 	// Remove the container from the pod
 	delete(podCtrs, ctr.ID())
 
+	// Remove us from container dependencies
+	depCtrs := ctr.Dependencies()
+	for _, depCtr := range depCtrs {
+		s.removeCtrFromDependsMap(ctr.ID(), depCtr)
+	}
+
 	return nil
 }
 

libpod/pod.go

@@ -45,6 +45,7 @@ func newPod(lockDir string, runtime *Runtime) (*Pod, error) {
 	pod := new(Pod)
 	pod.id = stringid.GenerateNonCryptoID()
 	pod.name = namesgenerator.GetRandomName(0)
+	pod.labels = make(map[string]string)
 	pod.runtime = runtime
 
 	// Path our lock file will reside at

libpod/sql_state.go

@@ -14,7 +14,7 @@ import (
 
 // DBSchema is the current DB schema version
 // Increments every time a change is made to the database's tables
-const DBSchema = 9
+const DBSchema = 10
 
 // SQLState is a state implementation backed by a persistent SQLite3 database
 type SQLState struct {
@@ -252,7 +252,7 @@ func (s *SQLState) AddContainer(ctr *Container) (err error) {
 		return errors.Wrapf(ErrPodExists, "cannot add container that belongs to a pod, use AddContainerToPod instead")
 	}
 
-	return s.addContainer(ctr)
+	return s.addContainer(ctr, nil)
 }
 
 // UpdateContainer updates a container's state from the database
@@ -381,7 +381,7 @@ func (s *SQLState) UpdateContainer(ctr *Container) error {
 }
 
 // SaveContainer updates a container's state in the database
-func (s *SQLState) SaveContainer(ctr *Container) error {
+func (s *SQLState) SaveContainer(ctr *Container) (err error) {
 	const update = `UPDATE containerState SET
                           State=?,
                           ConfigPath=?,
@@ -508,7 +508,7 @@ func (s *SQLState) RemoveContainer(ctr *Container) error {
 		return errors.Wrapf(ErrPodExists, "container %s belongs to a pod, use RemoveContainerFromPod", ctr.ID())
 	}
 
-	return s.removeContainer(ctr)
+	return s.removeContainer(ctr, nil)
 }
 
 // AllContainers retrieves all the containers presently in the state
@@ -801,7 +801,7 @@ func (s *SQLState) AddPod(pod *Pod) (err error) {
 
 // RemovePod removes a pod from the state
 // Only empty pods can be removed
-func (s *SQLState) RemovePod(pod *Pod) error {
+func (s *SQLState) RemovePod(pod *Pod) (err error) {
 	const (
 		removePod      = "DELETE FROM pods WHERE ID=?;"
 		removeRegistry = "DELETE FROM registry WHERE Id=?;"
@@ -832,6 +832,7 @@ func (s *SQLState) RemovePod(pod *Pod) error {
 	if err != nil {
 		return errors.Wrapf(err, "error retrieving number of rows in transaction removing pod %s", pod.ID())
 	} else if rows == 0 {
+		pod.valid = false
 		return ErrNoSuchPod
 	}
 
@@ -851,11 +852,11 @@ func (s *SQLState) RemovePod(pod *Pod) error {
 // This can avoid issues with dependencies within the pod
 // The operation will fail if any container in the pod has a dependency from
 // outside the pod
-func (s *SQLState) RemovePodContainers(pod *Pod) error {
+func (s *SQLState) RemovePodContainers(pod *Pod) (err error) {
 	const (
 		getPodCtrs     = "SELECT Id FROM containers WHERE pod=?;"
 		removeCtr      = "DELETE FROM containers WHERE pod=?;"
-		removeCtrState = "DELETE FROM containerState WHERE ID=ANY(SELECT Id FROM containers WHERE pod=?);"
+		removeCtrState = "DELETE FROM containerState WHERE ID IN (SELECT Id FROM containers WHERE pod=?);"
 	)
 
 	if !s.valid {
@@ -880,6 +881,16 @@ func (s *SQLState) RemovePodContainers(pod *Pod) error {
 		}
 	}()
 
+	// Check if the pod exists
+	exists, err := podExistsTx(pod.ID(), tx)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		pod.valid = false
+		return ErrNoSuchPod
+	}
+
 	// First get all containers in the pod
 	rows, err := tx.Query(getPodCtrs, pod.ID())
 	if err != nil {
@@ -920,7 +931,7 @@ func (s *SQLState) RemovePodContainers(pod *Pod) error {
 	}
 
 	if err := tx.Commit(); err != nil {
-		return errors.Wrapf(err, "error committing transaction to add pod %s", pod.ID())
+		return errors.Wrapf(err, "error committing transaction remove pod %s containers", pod.ID())
 	}
 
 	committed = true
@@ -963,7 +974,7 @@ func (s *SQLState) AddContainerToPod(pod *Pod, ctr *Container) error {
 		return errors.Wrapf(ErrInvalidArg, "container's pod ID does not match given pod's ID")
 	}
 
-	return s.addContainer(ctr)
+	return s.addContainer(ctr, pod)
 }
 
 // RemoveContainerFromPod removes a container from the given pod
@@ -972,7 +983,7 @@ func (s *SQLState) RemoveContainerFromPod(pod *Pod, ctr *Container) error {
 		return errors.Wrapf(ErrInvalidArg, "container %s is not in pod %s", ctr.ID(), pod.ID())
 	}
 
-	return s.removeContainer(ctr)
+	return s.removeContainer(ctr, pod)
 }
 
 // AllPods retrieves all pods presently in the state

libpod/sql_state_internal.go

@@ -37,6 +37,7 @@ const (
                           FROM containers
                           INNER JOIN
                               containerState ON containers.Id = containerState.Id `
+	ExistsQuery = "SELECT 1 FROM pods WHERE Id=?;"
 )
 
 // Checks that the DB configuration matches the runtime's configuration
@@ -326,9 +327,7 @@ func prepareDB(db *sql.DB) (err error) {
 // Check if given pod exists
 // Internal-only version of hasPod
 func (s *SQLState) podExists(id string) (bool, error) {
-	const query = "SELECT 1 FROM pods WHERE Id=?;"
+	row := s.db.QueryRow(ExistsQuery, id)
-
-	row := s.db.QueryRow(query, id)
 
 	var check int
 	err := row.Scan(&check)
@@ -343,7 +342,26 @@ func (s *SQLState) podExists(id string) (bool, error) {
 	}
 
 	return true, nil
+}
 
+// Check if given pod exists within a transaction
+// Transaction-based version of podExists
+func podExistsTx(id string, tx *sql.Tx) (bool, error) {
+	row := tx.QueryRow(ExistsQuery, id)
+
+	var check int
+	err := row.Scan(&check)
+	if err != nil {
+		if err == sql.ErrNoRows {
+			return false, nil
+		}
+
+		return false, errors.Wrapf(err, "error questing database for existence of pod %s", id)
+	} else if check != 1 {
+		return false, errors.Wrapf(ErrInternal, "check digit for podExists query incorrect")
+	}
+
+	return true, nil
 }
 
 // Get filename for OCI spec on disk
@@ -717,7 +735,7 @@ func (s *SQLState) podFromScannable(row scannable) (*Pod, error) {
 }
 
 // Internal function for adding containers
-func (s *SQLState) addContainer(ctr *Container) (err error) {
+func (s *SQLState) addContainer(ctr *Container, pod *Pod) (err error) {
 	const (
 		addCtr = `INSERT INTO containers VALUES (
                     ?, ?, ?, ?, ?,
@@ -800,6 +818,19 @@ func (s *SQLState) addContainer(ctr *Container) (err error) {
 		}
 	}()
 
+	// First check if pod exists, if one is given
+	if pod != nil {
+		exists, err := podExistsTx(pod.ID(), tx)
+		if err != nil {
+			return err
+		}
+
+		if !exists {
+			pod.valid = false
+			return errors.Wrapf(ErrNoSuchPod, "pod %s does not exist in state, cannot add container to it", pod.ID())
+		}
+	}
+
 	// Add container to registry
 	if _, err := tx.Exec(addRegistry, ctr.ID(), ctr.Name()); err != nil {
 		return errors.Wrapf(err, "error adding container %s to name/ID registry", ctr.ID())
@@ -905,11 +936,13 @@ func (s *SQLState) addContainer(ctr *Container) (err error) {
 }
 
 // Internal functions for removing containers
-func (s *SQLState) removeContainer(ctr *Container) error {
+func (s *SQLState) removeContainer(ctr *Container, pod *Pod) (err error) {
 	const (
 		removeCtr      = "DELETE FROM containers WHERE Id=?;"
 		removeState    = "DELETE FROM containerState WHERE Id=?;"
 		removeRegistry = "DELETE FROM registry WHERE Id=?;"
+		existsInPod    = "SELECT 1 FROM containers WHERE Id=? AND Pod=?;"
+		ctrExists      = "SELECT 1 FROM containers WHERE Id=?;"
 	)
 
 	if !s.valid {
@@ -930,6 +963,48 @@ func (s *SQLState) removeContainer(ctr *Container) error {
 		}
 	}()
 
+	if pod != nil {
+		// Check to see if the pod exists
+		exists, err := podExistsTx(pod.ID(), tx)
+		if err != nil {
+			return err
+		}
+
+		if !exists {
+			pod.valid = false
+			return errors.Wrapf(ErrNoSuchPod, "pod %s does not exist in state, cannot add container to it", pod.ID())
+		}
+
+		var check int
+
+		// Check to see if the container exists
+		row := tx.QueryRow(ctrExists, ctr.ID())
+		err = row.Scan(&check)
+		if err != nil {
+			if err == sql.ErrNoRows {
+				ctr.valid = false
+				return errors.Wrapf(ErrNoSuchCtr, "container %s does not exist", ctr.ID())
+			}
+
+			return errors.Wrapf(err, "error querying database for container %s existence", ctr.ID())
+		} else if check != 1 {
+			return errors.Wrapf(ErrInternal, "check digit for ctr exists query incorrect")
+		}
+
+		// Check to see if the container is in the pod
+		row = tx.QueryRow(existsInPod, ctr.ID(), pod.ID())
+		err = row.Scan(&check)
+		if err != nil {
+			if err == sql.ErrNoRows {
+				return errors.Wrapf(ErrNoSuchCtr, "container %s is not in pod %s", ctr.ID(), pod.ID())
+			}
+
+			return errors.Wrapf(err, "error querying database for container %s existence", ctr.ID())
+		} else if check != 1 {
+			return errors.Wrapf(ErrInternal, "check digit for ctr exists in pod query incorrect")
+		}
+	}
+
 	// Check rows acted on for the first transaction, verify we actually removed something
 	result, err := tx.Exec(removeCtr, ctr.ID())
 	if err != nil {
@@ -939,6 +1014,7 @@ func (s *SQLState) removeContainer(ctr *Container) error {
 	if err != nil {
 		return errors.Wrapf(err, "error retrieving number of rows in transaction removing container %s", ctr.ID())
 	} else if rows == 0 {
+		ctr.valid = false
 		return ErrNoSuchCtr
 	}