Rework state tests to avoid boilerplate. Begin adding pod tests.

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

Closes: #268
Approved by: rhatdan

libpod/in_memory_state.go

@@ -8,9 +8,9 @@ import (
 	"github.com/projectatomic/libpod/pkg/registrar"
 )
 
-// TODO: unified name/ID registry to ensure no name and ID conflicts between
+// TODO: Maybe separate idIndex for pod/containers
-// containers and pods
+// As of right now, partial IDs used in Lookup... need to be unique as well
-// This can probably be used to replace the existing trunc index and registrars
+// This may be undesirable?
 
 // An InMemoryState is a purely in-memory state store
 type InMemoryState struct {
@@ -486,7 +486,7 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error {
 	}
 
 	// Is the container already in the pod?
-	if _, ok := podCtrs[ctr.ID()]; ok {
+	if _, ok = podCtrs[ctr.ID()]; ok {
 		return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in pod %s", ctr.ID(), pod.ID())
 	}
 

libpod/pod.go

@@ -17,7 +17,7 @@ type Pod struct {
 
 	valid   bool
 	runtime *Runtime
-	lock storage.Locker
+	lock    storage.Locker
 }
 
 // ID retrieves the pod's ID
@@ -59,7 +59,7 @@ func newPod(lockDir string, runtime *Runtime) (*Pod, error) {
 	return pod, nil
 }
 
-// Init() initializes all containers within a pod that have not been initialized
+// Init initializes all containers within a pod that have not been initialized
 func (p *Pod) Init() error {
 	return ErrNotImplemented
 }
@@ -88,7 +88,7 @@ func (p *Pod) HasContainer(id string) (bool, error) {
 	return p.runtime.state.PodHasContainer(p, id)
 }
 
-// AllContainersID returns the container IDs of all the containers in the pod
+// AllContainersByID returns the container IDs of all the containers in the pod
 func (p *Pod) AllContainersByID() ([]string, error) {
 	p.lock.Lock()
 	defer p.lock.Unlock()

libpod/runtime_ctr.go

@@ -121,8 +121,9 @@ func (r *Runtime) removeContainer(c *Container, force bool) error {
 	// We need to lock the pod before we lock the container
 	// To avoid races around removing a container and the pod it is in
 	var pod *Pod
+	var err error
 	if c.config.Pod != "" {
-		pod, err := r.state.Pod(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())
 		}

libpod/sql_state.go

@@ -754,7 +754,10 @@ func (s *SQLState) PodContainers(pod *Pod) ([]*Container, error) {
 // AddPod adds a pod to the state
 // Only empty pods can be added to the state
 func (s *SQLState) AddPod(pod *Pod) (err error) {
-	const query = "INSERT INTO pods VALUES (?, ?, ?);"
+	const (
+		podQuery      = "INSERT INTO pods VALUES (?, ?, ?);"
+		registryQuery = "INSERT INTO registry VALUES (?, ?);"
+	)
 
 	if !s.valid {
 		return ErrDBClosed
@@ -781,8 +784,11 @@ func (s *SQLState) AddPod(pod *Pod) (err error) {
 		}
 	}()
 
-	_, err = tx.Exec(query, pod.ID(), pod.Name(), string(labelsJSON))
+	if _, err := tx.Exec(registryQuery, pod.ID(), pod.Name()); err != nil {
-	if err != nil {
+		return errors.Wrapf(err, "error adding pod %s to name/ID registry", pod.ID())
+	}
+
+	if _, err = tx.Exec(podQuery, pod.ID(), pod.Name(), string(labelsJSON)); err != nil {
 		return errors.Wrapf(err, "error adding pod %s to database", pod.ID())
 	}
 
@@ -796,7 +802,10 @@ 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 {
-	const query = "DELETE FROM pods WHERE ID=?;"
+	const (
+		removePod      = "DELETE FROM pods WHERE ID=?;"
+		removeRegistry = "DELETE FROM registry WHERE Id=?;"
+	)
 
 	if !s.valid {
 		return ErrDBClosed
@@ -814,8 +823,8 @@ func (s *SQLState) RemovePod(pod *Pod) error {
 		}
 	}()
 
-	// Check rows acted on for the first transaction, verify we actually removed something
+	// Check rows acted on for the first statement, verify we actually removed something
-	result, err := tx.Exec(query, pod.ID())
+	result, err := tx.Exec(removePod, pod.ID())
 	if err != nil {
 		return errors.Wrapf(err, "error removing pod %s from pods table", pod.ID())
 	}
@@ -826,6 +835,11 @@ func (s *SQLState) RemovePod(pod *Pod) error {
 		return ErrNoSuchPod
 	}
 
+	// We know it exists, remove it from registry
+	if _, err := tx.Exec(removeRegistry, pod.ID()); err != nil {
+		return errors.Wrapf(err, "error removing pod %s from name/ID registry", pod.ID())
+	}
+
 	if err := tx.Commit(); err != nil {
 		return errors.Wrapf(err, "error committing transaction to remove pod %s", pod.ID())
 	}
@@ -891,7 +905,7 @@ func (s *SQLState) RemovePodContainers(pod *Pod) error {
 		return errors.Wrapf(err, "error retrieving container rows")
 	}
 
-	// Have container IDs, now exec SQL to remove contianers in the pod
+	// Have container IDs, now exec SQL to remove containers in the pod
 	// Remove state first, as it needs the subquery on containers
 	// Don't bother checking if we actually removed anything, we just want to
 	// empty the pod

libpod/sql_state_internal.go

@@ -173,9 +173,6 @@ func checkDB(db *sql.DB, r *Runtime) (err error) {
 // Performs database setup including by not limited to initializing tables in
 // the database
 func prepareDB(db *sql.DB) (err error) {
-	// TODO create pod tables
-	// TODO add Pod ID to CreateStaticContainer as a FOREIGN KEY referencing podStatic(Id)
-	// TODO add ctr shared namespaces information - A separate table, probably? So we can FOREIGN KEY the ID
 	// TODO schema migration might be necessary and should be handled here
 	// TODO maybe make a port mappings table instead of JSONing the array and storing it?
 	// TODO prepared statements for common queries for performance
@@ -185,6 +182,14 @@ func prepareDB(db *sql.DB) (err error) {
 		return errors.Wrapf(err, "error enabling foreign key support in database")
 	}
 
+	// Create a table for holding container and pod names and IDs
+	const createRegistry = `
+        CREATE TABLE IF NOT EXISTS registry(
+            Id   TEXT NOT NULL PRIMARY KEY,
+            Name TEXT NOT NULL UNIQUE
+        );
+        `
+
 	// Create a table for unchanging container data
 	const createCtr = `
         CREATE TABLE IF NOT EXISTS containers(
@@ -235,7 +240,9 @@ func prepareDB(db *sql.DB) (err error) {
             CHECK (CreateNetNS IN (0, 1)),
             CHECK (Stdin IN (0, 1)),
             CHECK (StopSignal>=0),
-            FOREIGN KEY (Id)          REFERENCES containerState(Id) DEFERRABLE INITIALLY DEFERRED
+            FOREIGN KEY (Id)          REFERENCES registry(Id)       DEFERRABLE INITIALLY DEFERRED,
+            FOREIGN KEY (Id)          REFERENCES containerState(Id) DEFERRABLE INITIALLY DEFERRED,
+            FOREIGN KEY (Name)        REFERENCES registry(Name)     DEFERRABLE INITIALLY DEFERRED,
             FOREIGN KEY (Pod)         REFERENCES pods(Id)           DEFERRABLE INITIALLY DEFERRED,
             FOREIGN KEY (IPCNsCtr)    REFERENCES containers(Id)     DEFERRABLE INITIALLY DEFERRED,
             FOREIGN KEY (MountNsCtr)  REFERENCES containers(Id)     DEFERRABLE INITIALLY DEFERRED,
@@ -266,7 +273,8 @@ func prepareDB(db *sql.DB) (err error) {
 
             CHECK (State>0),
             CHECK (OomKilled IN (0, 1)),
-            FOREIGN KEY (Id) REFERENCES containers(Id) DEFERRABLE INITIALLY DEFERRED
+            FOREIGN KEY (Id)   REFERENCES registry(Id)   DEFERRABLE INITIALLY DEFERRED,
+            FOREIGN KEY (Id)   REFERENCES containers(Id) DEFERRABLE INITIALLY DEFERRED
         );
         `
 
@@ -275,7 +283,9 @@ func prepareDB(db *sql.DB) (err error) {
         CREATE TABLE IF NOT EXISTS pods(
             Id     TEXT NOT NULL PRIMARY KEY,
             Name   TEXT NOT NULL UNIQUE,
-            Labels TEXT NOT NULL
+            Labels TEXT NOT NULL,
+            FOREIGN KEY (Id)   REFERENCES registry(Id)   DEFERRABLE INITIALLY DEFERRED,
+            FOREIGN KEY (Name) REFERENCES registry(Name) DEFERRABLE INITIALLY DEFERRED
         );
         `
 
@@ -293,6 +303,9 @@ func prepareDB(db *sql.DB) (err error) {
 
 	}()
 
+	if _, err := tx.Exec(createRegistry); err != nil {
+		return errors.Wrapf(err, "error creating ID and Name registry in database")
+	}
 	if _, err := tx.Exec(createCtr); err != nil {
 		return errors.Wrapf(err, "error creating containers table in database")
 	}
@@ -720,6 +733,7 @@ func (s *SQLState) addContainer(ctr *Container) (err error) {
                     ?, ?, ?, ?, ?,
                     ?, ?, ?
                 );`
+		addRegistry = "INSERT INTO registry VALUES (?, ?);"
 	)
 
 	if !s.valid {
@@ -786,6 +800,11 @@ func (s *SQLState) addContainer(ctr *Container) (err error) {
 		}
 	}()
 
+	// 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())
+	}
+
 	// Add static container information
 	_, err = tx.Exec(addCtr,
 		ctr.ID(),
@@ -888,8 +907,9 @@ func (s *SQLState) addContainer(ctr *Container) (err error) {
 // Internal functions for removing containers
 func (s *SQLState) removeContainer(ctr *Container) error {
 	const (
-		removeCtr   = "DELETE FROM containers WHERE Id=?;"
+		removeCtr      = "DELETE FROM containers WHERE Id=?;"
-		removeState = "DELETE FROM containerState WHERE Id=?;"
+		removeState    = "DELETE FROM containerState WHERE Id=?;"
+		removeRegistry = "DELETE FROM registry WHERE Id=?;"
 	)
 
 	if !s.valid {
@@ -926,6 +946,12 @@ func (s *SQLState) removeContainer(ctr *Container) error {
 		return errors.Wrapf(err, "error removing container %s from state table", ctr.ID())
 	}
 
+	// Remove registry last of all
+	// So we know the container did exist
+	if _, err := tx.Exec(removeRegistry, ctr.ID()); err != nil {
+		return errors.Wrapf(err, "error removing container %s from name/ID registry", ctr.ID())
+	}
+
 	if err := tx.Commit(); err != nil {
 		return errors.Wrapf(err, "error committing transaction to remove container %s", ctr.ID())
 	}

libpod/test_common.go

@@ -24,7 +24,7 @@ func getTestContainer(id, name, locksDir string) (*Container, error) {
 			StaticDir:       "/does/not/exist/",
 			LogPath:         "/does/not/exist/",
 			Stdin:           true,
-			Labels:          make(map[string]string),
+			Labels:          map[string]string{"a": "b", "c": "d"},
 			StopSignal:      0,
 			StopTimeout:     0,
 			CreatedTime:     time.Now(),
@@ -74,6 +74,25 @@ func getTestContainer(id, name, locksDir string) (*Container, error) {
 	return ctr, nil
 }
 
+// nolint
+func getTestPod(id, name, locksDir string) (*Pod, error) {
+	pod := &Pod{
+		id:     id,
+		name:   name,
+		labels: map[string]string{"a": "b", "c": "d"},
+		valid:  true,
+	}
+
+	lockPath := filepath.Join(locksDir, id)
+	lock, err := storage.GetLockfile(lockPath)
+	if err != nil {
+		return nil, err
+	}
+	pod.lock = lock
+
+	return pod, nil
+}
+
 // This horrible hack tests if containers are equal in a way that should handle
 // empty arrays being dropped to nil pointers in the spec JSON
 // nolint
@@ -114,3 +133,26 @@ func testContainersEqual(a, b *Container) bool {
 
 	return reflect.DeepEqual(aStateJSON, bStateJSON)
 }
+
+// This tests pod equality
+// We cannot guarantee equality in lockfile objects so we can't simply compare
+// nolint
+func testPodsEqual(a, b *Pod) bool {
+	if a == nil && b == nil {
+		return true
+	} else if a == nil || b == nil {
+		return false
+	}
+
+	if a.id != b.id {
+		return false
+	}
+	if a.name != b.name {
+		return false
+	}
+	if a.valid != b.valid {
+		return false
+	}
+
+	return reflect.DeepEqual(a.labels, b.labels)
+}