Refactor Pod to use a Config struct

This allows us to JSON it and stuff it in the DB - previously,
all pod fields were private, so JSON couldn't encode them. This
allows us to keep all pod fields private by having a substruct
with public fields.

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

Closes: #184
Approved by: baude

libpod/boltdb_state.go

@@ -466,6 +466,7 @@ func (s *BoltState) SaveContainer(ctr *Container) error {
 
 		ctrToSave := ctrBucket.Bucket(ctrID)
 		if ctrToSave == nil {
+			ctr.valid = false
 			return errors.Wrapf(ErrNoSuchCtr, "container %s does not exist in DB", ctr.ID())
 		}
 
@@ -612,6 +613,7 @@ func (s *BoltState) Pod(id string) (*Pod, error) {
 	podID := []byte(id)
 
 	pod := new(Pod)
+	pod.config = new(PodConfig)
 
 	db, err := s.getDBCon()
 	if err != nil {
@@ -645,6 +647,7 @@ func (s *BoltState) LookupPod(idOrName string) (*Pod, error) {
 	}
 
 	pod := new(Pod)
+	pod.config = new(PodConfig)
 
 	db, err := s.getDBCon()
 	if err != nil {
@@ -736,8 +739,8 @@ func (s *BoltState) HasPod(id string) (bool, error) {
 			return err
 		}
 
-		podBytes := podBkt.Get(podID)
+		podDB := podBkt.Bucket(podID)
-		if podBytes != nil {
+		if podDB != nil {
 			exists = true
 		}
 
@@ -913,6 +916,8 @@ func (s *BoltState) PodContainers(pod *Pod) ([]*Container, error) {
 		// Iterate through all containers in the pod
 		err = podCtrs.ForEach(func(id, val []byte) error {
 			newCtr := new(Container)
+			newCtr.config = new(ContainerConfig)
+			newCtr.state = new(containerState)
 			ctrs = append(ctrs, newCtr)
 
 			return s.getContainerFromDB(id, newCtr, ctrBkt)
@@ -943,7 +948,7 @@ func (s *BoltState) AddPod(pod *Pod) error {
 	podID := []byte(pod.ID())
 	podName := []byte(pod.Name())
 
-	podJSON, err := json.Marshal(pod)
+	podJSON, err := json.Marshal(pod.config)
 	if err != nil {
 		return errors.Wrapf(err, "error marshalling pod %s JSON", pod.ID())
 	}
@@ -1143,14 +1148,7 @@ func (s *BoltState) RemovePodContainers(pod *Pod) error {
 
 		// Traverse all containers in the pod with a cursor
 		// for-each has issues with data mutation
-		cursor := podCtrsBkt.Cursor()
+		err = podCtrsBkt.ForEach(func(id, name []byte) error {
-		id, name := cursor.Last()
-		for id != nil {
-			// Remove the container from the bucker
-			if err := podCtrsBkt.Delete(id); err != nil {
-				return errors.Wrapf(err, "error removing container %s from pod %s", string(id), pod.ID())
-			}
-
 			// Get the container so we can check dependencies
 			ctr := ctrBkt.Bucket(id)
 			if ctr == nil {
@@ -1165,15 +1163,10 @@ func (s *BoltState) RemovePodContainers(pod *Pod) error {
 				err = ctrDeps.ForEach(func(depID, name []byte) error {
 					exists := podCtrsBkt.Get(depID)
 					if exists == nil {
-						// The dependency isn't in the
+						return errors.Wrapf(ErrCtrExists, "container %s has dependency %s outside of pod %s", string(id), string(depID), pod.ID())
-						// pod
-						return errors.Wrapf(ErrCtrExists, "container %s has a dependency outside of pod %s", string(depID), pod.ID())
 					}
 					return nil
 				})
-				if err != nil {
-					return err
-				}
 			}
 
 			// Dependencies are set, we're clear to remove
@@ -1190,7 +1183,18 @@ func (s *BoltState) RemovePodContainers(pod *Pod) error {
 				return errors.Wrapf(err, "error deleting container %s name in DB", string(id))
 			}
 
-			id, name = cursor.Last()
+			return nil
+		})
+		if err != nil {
+			return err
+		}
+
+		// Delete and recreate the bucket to empty it
+		if err := podDB.DeleteBucket(containersBkt); err != nil {
+			return errors.Wrapf(err, "error removing pod %s containers bucket", pod.ID())
+		}
+		if _, err := podDB.CreateBucket(containersBkt); err != nil {
+			return errors.Wrapf(err, "error recreating pod %s containers bucket", pod.ID())
 		}
 
 		return nil
@@ -1290,6 +1294,7 @@ func (s *BoltState) AllPods() ([]*Pod, error) {
 			}
 
 			pod := new(Pod)
+			pod.config = new(PodConfig)
 
 			pods = append(pods, pod)
 

libpod/boltdb_state_internal.go

@@ -216,7 +216,7 @@ func (s *BoltState) getPodFromDB(id []byte, pod *Pod, podBkt *bolt.Bucket) error
 		return errors.Wrapf(ErrInternal, "pod %s is missing configuration key in DB", string(id))
 	}
 
-	if err := json.Unmarshal(podBytes, pod); err != nil {
+	if err := json.Unmarshal(podBytes, pod.config); err != nil {
 		return errors.Wrapf(err, "error unmarshalling pod %s from DB", string(id))
 	}
 

libpod/options.go

@@ -649,7 +649,7 @@ func WithPodName(name string) PodCreateOption {
 			return errors.Wrapf(ErrInvalidArg, "name must match regex [a-zA-Z0-9_-]+")
 		}
 
-		pod.name = name
+		pod.config.Name = name
 
 		return nil
 	}
@@ -662,9 +662,9 @@ func WithPodLabels(labels map[string]string) PodCreateOption {
 			return ErrPodFinalized
 		}
 
-		pod.labels = make(map[string]string)
+		pod.config.Labels = make(map[string]string)
 		for key, value := range labels {
-			pod.labels[key] = value
+			pod.config.Labels[key] = value
 		}
 
 		return nil

libpod/pod.go

@@ -11,29 +11,34 @@ import (
 
 // Pod represents a group of containers that may share namespaces
 type Pod struct {
-	id     string            `json:"id"`
+	config *PodConfig
-	name   string            `json:"name"`
-	labels map[string]string `json:"labels"`
 
-	valid   bool           `json:"-"`
+	valid   bool
-	runtime *Runtime       `json:"-"`
+	runtime *Runtime
-	lock    storage.Locker `json:"-"`
+	lock    storage.Locker
+}
+
+// PodConfig represents a pod's static configuration
+type PodConfig struct {
+	ID     string            `json:"id"`
+	Name   string            `json:"name"`
+	Labels map[string]string `json:""`
 }
 
 // ID retrieves the pod's ID
 func (p *Pod) ID() string {
-	return p.id
+	return p.config.ID
 }
 
 // Name retrieves the pod's name
 func (p *Pod) Name() string {
-	return p.name
+	return p.config.Name
 }
 
 // Labels returns the pod's labels
 func (p *Pod) Labels() map[string]string {
 	labels := make(map[string]string)
-	for key, value := range p.labels {
+	for key, value := range p.config.Labels {
 		labels[key] = value
 	}
 
@@ -43,13 +48,14 @@ func (p *Pod) Labels() map[string]string {
 // Creates a new, empty pod
 func newPod(lockDir string, runtime *Runtime) (*Pod, error) {
 	pod := new(Pod)
-	pod.id = stringid.GenerateNonCryptoID()
+	pod.config = new(PodConfig)
-	pod.name = namesgenerator.GetRandomName(0)
+	pod.config.ID = stringid.GenerateNonCryptoID()
-	pod.labels = make(map[string]string)
+	pod.config.Name = namesgenerator.GetRandomName(0)
+	pod.config.Labels = make(map[string]string)
 	pod.runtime = runtime
 
 	// Path our lock file will reside at
-	lockPath := filepath.Join(lockDir, pod.id)
+	lockPath := filepath.Join(lockDir, pod.config.ID)
 	// Grab a lockfile at the given path
 	lock, err := storage.GetLockfile(lockPath)
 	if err != nil {

libpod/sql_state.go

@@ -764,7 +764,7 @@ func (s *SQLState) AddPod(pod *Pod) (err error) {
 		return ErrPodRemoved
 	}
 
-	labelsJSON, err := json.Marshal(pod.labels)
+	labelsJSON, err := json.Marshal(pod.config.Labels)
 	if err != nil {
 		return errors.Wrapf(err, "error marshaling pod %s labels to JSON", pod.ID())
 	}

libpod/sql_state_internal.go

@@ -709,8 +709,9 @@ func (s *SQLState) podFromScannable(row scannable) (*Pod, error) {
 	}
 
 	pod := new(Pod)
-	pod.id = id
+	pod.config = new(PodConfig)
-	pod.name = name
+	pod.config.ID = id
+	pod.config.Name = name
 	pod.runtime = s.runtime
 
 	// Decode labels JSON
@@ -718,7 +719,7 @@ func (s *SQLState) podFromScannable(row scannable) (*Pod, error) {
 	if err := json.Unmarshal([]byte(labelsJSON), &podLabels); err != nil {
 		return nil, errors.Wrapf(err, "error unmarshaling pod %s labels JSON", id)
 	}
-	pod.labels = podLabels
+	pod.config.Labels = podLabels
 
 	// Retrieve pod lock
 	// Open and set the lockfile

libpod/state_test.go

@@ -848,11 +848,8 @@ func TestGetPodOnePod(t *testing.T) {
 		statePod, err := state.Pod(testPod.ID())
 		assert.NoError(t, err)
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, statePod.config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, statePod.valid)
-		if !testPodsEqual(testPod, statePod) {
-			assert.EqualValues(t, testPod, statePod)
-		}
 	})
 }
 
@@ -873,11 +870,8 @@ func TestGetOnePodFromTwo(t *testing.T) {
 		statePod, err := state.Pod(testPod1.ID())
 		assert.NoError(t, err)
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod1.config, statePod.config)
-		// between actual and expected
+		assert.Equal(t, testPod1.valid, statePod.valid)
-		if !testPodsEqual(testPod1, statePod) {
-			assert.EqualValues(t, testPod1, statePod)
-		}
 	})
 }
 
@@ -938,11 +932,8 @@ func TestLookupPodFullID(t *testing.T) {
 		statePod, err := state.LookupPod(testPod.ID())
 		assert.NoError(t, err)
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, statePod.config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, statePod.valid)
-		if !testPodsEqual(testPod, statePod) {
-			assert.EqualValues(t, testPod, statePod)
-		}
 	})
 }
 
@@ -957,11 +948,8 @@ func TestLookupPodUniquePartialID(t *testing.T) {
 		statePod, err := state.LookupPod(testPod.ID()[0:8])
 		assert.NoError(t, err)
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, statePod.config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, statePod.valid)
-		if !testPodsEqual(testPod, statePod) {
-			assert.EqualValues(t, testPod, statePod)
-		}
 	})
 }
 
@@ -995,11 +983,8 @@ func TestLookupPodByName(t *testing.T) {
 		statePod, err := state.LookupPod(testPod.Name())
 		assert.NoError(t, err)
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, statePod.config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, statePod.valid)
-		if !testPodsEqual(testPod, statePod) {
-			assert.EqualValues(t, testPod, statePod)
-		}
 	})
 }
 
@@ -1088,7 +1073,7 @@ func TestHasPodCtrIDFalse(t *testing.T) {
 
 func TestAddPodInvalidPodErrors(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		err := state.AddPod(&Pod{})
+		err := state.AddPod(&Pod{config: &PodConfig{},})
 		assert.Error(t, err)
 	})
 }
@@ -1105,11 +1090,8 @@ func TestAddPodValidPodSucceeds(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(allPods))
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, allPods[0].config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, allPods[0].valid)
-		if !testPodsEqual(testPod, allPods[0]) {
-			assert.EqualValues(t, testPod, allPods[0])
-		}
 	})
 }
 
@@ -1215,7 +1197,7 @@ func TestAddPodCtrNameConflictFails(t *testing.T) {
 
 func TestRemovePodInvalidPodErrors(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		err := state.RemovePod(&Pod{})
+		err := state.RemovePod(&Pod{config: &PodConfig{},})
 		assert.Error(t, err)
 	})
 }
@@ -1269,11 +1251,8 @@ func TestRemovePodFromPods(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(allPods))
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod2.config, allPods[0].config)
-		// between actual and expected
+		assert.Equal(t, testPod2.valid, allPods[0].valid)
-		if !testPodsEqual(testPod2, allPods[0]) {
-			assert.EqualValues(t, testPod2, allPods[0])
-		}
 	})
 }
 
@@ -1348,11 +1327,8 @@ func TestAllPodsFindsPod(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(allPods))
 
-		// Use assert.EqualValues if the test fails to pretty print diff
+		assert.EqualValues(t, testPod.config, allPods[0].config)
-		// between actual and expected
+		assert.Equal(t, testPod.valid, allPods[0].valid)
-		if !testPodsEqual(testPod, allPods[0]) {
-			assert.EqualValues(t, testPod, allPods[0])
-		}
 	})
 }
 
@@ -1396,7 +1372,7 @@ func TestAllPodsMultiplePods(t *testing.T) {
 
 func TestPodHasContainerNoSuchPod(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		_, err := state.PodHasContainer(&Pod{}, strings.Repeat("0", 32))
+		_, err := state.PodHasContainer(&Pod{config: &PodConfig{},}, strings.Repeat("0", 32))
 		assert.Error(t, err)
 	})
 }
@@ -1472,7 +1448,7 @@ func TestPodHasContainerSucceeds(t *testing.T) {
 
 func TestPodContainersByIDInvalidPod(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		_, err := state.PodContainersByID(&Pod{})
+		_, err := state.PodContainersByID(&Pod{config: &PodConfig{},})
 		assert.Error(t, err)
 	})
 }
@@ -1574,7 +1550,7 @@ func TestPodContainersByIDMultipleContainers(t *testing.T) {
 
 func TestPodContainersInvalidPod(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		_, err := state.PodContainers(&Pod{})
+		_, err := state.PodContainers(&Pod{config: &PodConfig{},})
 		assert.Error(t, err)
 	})
 }
@@ -1681,7 +1657,7 @@ func TestPodContainersMultipleContainers(t *testing.T) {
 
 func TestRemovePodContainersInvalidPod(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		err := state.RemovePodContainers(&Pod{})
+		err := state.RemovePodContainers(&Pod{config: &PodConfig{},})
 		assert.Error(t, err)
 	})
 }
@@ -1872,7 +1848,7 @@ func TestAddContainerToPodInvalidPod(t *testing.T) {
 		testCtr, err := getTestCtr1(lockPath)
 		assert.NoError(t, err)
 
-		err = state.AddContainerToPod(&Pod{}, testCtr)
+		err = state.AddContainerToPod(&Pod{config: &PodConfig{},}, testCtr)
 		assert.Error(t, err)
 	})
 }
@@ -2203,7 +2179,7 @@ func TestRemoveContainerFromPodBadPodFails(t *testing.T) {
 		testCtr, err := getTestCtr1(lockPath)
 		assert.NoError(t, err)
 
-		err = state.RemoveContainerFromPod(&Pod{}, testCtr)
+		err = state.RemoveContainerFromPod(&Pod{config: &PodConfig{},}, testCtr)
 		assert.Error(t, err)
 	})
 }

libpod/test_common.go

@@ -77,9 +77,11 @@ func getTestContainer(id, name, locksDir string) (*Container, error) {
 // nolint
 func getTestPod(id, name, locksDir string) (*Pod, error) {
 	pod := &Pod{
-		id:     id,
+		config: &PodConfig{
-		name:   name,
+			ID: id,
-		labels: map[string]string{"a": "b", "c": "d"},
+			Name: name,
+			Labels: map[string]string{"a": "b", "c": "d"},
+		},
 		valid:  true,
 	}
 
@@ -133,26 +135,3 @@ 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)
-}