Do not fetch pod and ctr State on retrieval in Bolt

It's not necessary to fill in state immediately, as we'll be
overwriting it on any API call accessing it thanks to
syncContainer(). It is also causing races when we fetch it
without holding the container lock (which syncContainer() does).
As such, just don't retrieve the state on initial pull from the
database with Bolt.

Also, refactor some Linux-specific netns handling functions out
of container_internal_linux.go into boltdb_linux.go.

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

Closes: #1186
Approved by: rhatdan

7 files changed, 64 insertions, 99 deletions

diff --git a/libpod/boltdb_state.go b/libpod/boltdb_state.go
index 49bdaea39..de6174c10 100644
--- a/libpod/boltdb_state.go
+++ b/libpod/boltdb_state.go
@@ -528,8 +528,10 @@ func (s *BoltState) UpdateContainer(ctr *Container) error {
 		return err
 	}
 
-	// Do we need to replace the container's netns?
-	ctr.setNamespace(netNSPath, newState)
+	// Handle network namespace
+	if err := replaceNetNS(netNSPath, ctr, newState); err != nil {
+		return err
+	}
 
 	// New state compiled successfully, swap it into the current state
 	ctr.state = newState
@@ -555,7 +557,7 @@ func (s *BoltState) SaveContainer(ctr *Container) error {
 	if err != nil {
 		return errors.Wrapf(err, "error marshalling container %s state to JSON", ctr.ID())
 	}
-	netNSPath := ctr.setNamespaceStatePath()
+	netNSPath := getNetNSPath(ctr)
 
 	ctrID := []byte(ctr.ID())
 
diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go
index 01ebae78f..cc7d106cc 100644
--- a/libpod/boltdb_state_internal.go
+++ b/libpod/boltdb_state_internal.go
@@ -256,25 +256,10 @@ func (s *BoltState) getContainerFromDB(id []byte, ctr *Container, ctrsBkt *bolt.
 		return errors.Wrapf(ErrInternal, "container %s missing config key in DB", string(id))
 	}
 
-	stateBytes := ctrBkt.Get(stateKey)
-	if stateBytes == nil {
-		return errors.Wrapf(ErrInternal, "container %s missing state key in DB", string(id))
-	}
-
-	netNSBytes := ctrBkt.Get(netNSKey)
-
 	if err := json.Unmarshal(configBytes, ctr.config); err != nil {
 		return errors.Wrapf(err, "error unmarshalling container %s config", string(id))
 	}
 
-	if err := json.Unmarshal(stateBytes, ctr.state); err != nil {
-		return errors.Wrapf(err, "error unmarshalling container %s state", string(id))
-	}
-
-	if !parseNetNSBoltData(ctr, netNSBytes) {
-		valid = false
-	}
-
 	// Get the lock
 	lockPath := filepath.Join(s.lockDir, string(id))
 	lock, err := storage.GetLockfile(lockPath)
@@ -311,15 +296,6 @@ func (s *BoltState) getPodFromDB(id []byte, pod *Pod, podBkt *bolt.Bucket) error
 		return errors.Wrapf(err, "error unmarshalling pod %s config from DB", string(id))
 	}
 
-	podStateBytes := podDB.Get(stateKey)
-	if podStateBytes == nil {
-		return errors.Wrapf(ErrInternal, "pod %s is missing state key in DB", string(id))
-	}
-
-	if err := json.Unmarshal(podStateBytes, pod.state); err != nil {
-		return errors.Wrapf(err, "error unmarshalling pod %s state from DB", string(id))
-	}
-
 	// Get the lock
 	lockPath := filepath.Join(s.lockDir, string(id))
 	lock, err := storage.GetLockfile(lockPath)
@@ -352,7 +328,7 @@ func (s *BoltState) addContainer(ctr *Container, pod *Pod) error {
 	if err != nil {
 		return errors.Wrapf(err, "error marshalling container %s state to JSON", ctr.ID())
 	}
-	netNSPath := ctr.setNamespaceStatePath()
+	netNSPath := getNetNSPath(ctr)
 	dependsCtrs := ctr.Dependencies()
 
 	ctrID := []byte(ctr.ID())
diff --git a/libpod/boltdb_state_linux.go b/libpod/boltdb_state_linux.go
index 9238d22fe..fce3a1b1e 100644
--- a/libpod/boltdb_state_linux.go
+++ b/libpod/boltdb_state_linux.go
@@ -6,20 +6,44 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// parseNetNSBoltData sets ctr.state.NetNS, if any, from netNSBytes.
-// Returns true if the data is valid.
-func parseNetNSBoltData(ctr *Container, netNSBytes []byte) bool {
-	// The container may not have a network namespace, so it's OK if this is
-	// nil
-	if netNSBytes != nil {
-		nsPath := string(netNSBytes)
-		netNS, err := joinNetNS(nsPath)
-		if err == nil {
-			ctr.state.NetNS = netNS
+// replaceNetNS handle network namespace transitions after updating a
+// container's state.
+func replaceNetNS(netNSPath string, ctr *Container, newState *containerState) error {
+	if netNSPath != "" {
+		// Check if the container's old state has a good netns
+		if ctr.state.NetNS != nil && netNSPath == ctr.state.NetNS.Path() {
+			newState.NetNS = ctr.state.NetNS
 		} else {
-			logrus.Errorf("error joining network namespace for container %s", ctr.ID())
-			return false
+			// Close the existing namespace.
+			// Whoever removed it from the database already tore it down.
+			if err := ctr.runtime.closeNetNS(ctr); err != nil {
+				return err
+			}
+
+			// Open the new network namespace
+			ns, err := joinNetNS(netNSPath)
+			if err == nil {
+				newState.NetNS = ns
+			} else {
+				logrus.Errorf("error joining network namespace for container %s", ctr.ID())
+				ctr.valid = false
+			}
+		}
+	} else {
+		// The container no longer has a network namespace
+		// Close the old one, whoever removed it from the DB should have
+		// cleaned it up already.
+		if err := ctr.runtime.closeNetNS(ctr); err != nil {
+			return err
 		}
 	}
-	return true
+	return nil
+}
+
+// getNetNSPath retrieves the netns path to be stored in the database
+func getNetNSPath(ctr *Container) string {
+	if ctr.state.NetNS != nil {
+		return ctr.state.NetNS.Path()
+	}
+	return ""
 }
diff --git a/libpod/boltdb_state_unsupported.go b/libpod/boltdb_state_unsupported.go
index 960faa911..4565c1890 100644
--- a/libpod/boltdb_state_unsupported.go
+++ b/libpod/boltdb_state_unsupported.go
@@ -6,12 +6,12 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// parseNetNSBoltData sets ctr.state.NetNS, if any, from netNSBytes.
-// Returns true if the data is valid.
-func parseNetNSBoltData(ctr *Container, netNSBytes []byte) bool {
-	if netNSBytes != nil {
-		logrus.Errorf("error loading %s: network namespaces are not supported on this platform", ctr.ID())
-		return false
-	}
-	return true
+// replaceNetNS is exclusive to the Linux platform and is a no-op elsewhere
+func replaceNetNS(netNSPath string, ctr *Container, newState *containerState) error {
+	return nil
+}
+
+// getNetNSPath is exclusive to the Linux platform and is a no-op elsewhere
+func getNetNSPath(ctr *Container) string {
+	return
 }
diff --git a/libpod/container_linux.go b/libpod/container_linux.go
index 1b1b3a1d9..c445fb8af 100644
--- a/libpod/container_linux.go
+++ b/libpod/container_linux.go
@@ -4,52 +4,11 @@ package libpod
 
 import (
 	"github.com/containernetworking/plugins/pkg/ns"
-	"github.com/sirupsen/logrus"
 )
 
 type containerPlatformState struct {
-
 	// NetNSPath is the path of the container's network namespace
 	// Will only be set if config.CreateNetNS is true, or the container was
 	// told to join another container's network namespace
 	NetNS ns.NetNS `json:"-"`
 }
-
-func (ctr *Container) setNamespace(netNSPath string, newState *containerState) error {
-	if netNSPath != "" {
-		// Check if the container's old state has a good netns
-		if ctr.state.NetNS != nil && netNSPath == ctr.state.NetNS.Path() {
-			newState.NetNS = ctr.state.NetNS
-		} else {
-			// Close the existing namespace.
-			// Whoever removed it from the database already tore it down.
-			if err := ctr.runtime.closeNetNS(ctr); err != nil {
-				return err
-			}
-
-			// Open the new network namespace
-			ns, err := joinNetNS(netNSPath)
-			if err == nil {
-				newState.NetNS = ns
-			} else {
-				logrus.Errorf("error joining network namespace for container %s", ctr.ID())
-				ctr.valid = false
-			}
-		}
-	} else {
-		// The container no longer has a network namespace
-		// Close the old one, whoever removed it from the DB should have
-		// cleaned it up already.
-		if err := ctr.runtime.closeNetNS(ctr); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func (ctr *Container) setNamespaceStatePath() string {
-	if ctr.state.NetNS != nil {
-		return ctr.state.NetNS.Path()
-	}
-	return ""
-}
diff --git a/libpod/container_unsupported.go b/libpod/container_unsupported.go
index 5b923b52a..e214b9465 100644
--- a/libpod/container_unsupported.go
+++ b/libpod/container_unsupported.go
@@ -3,11 +3,3 @@
 package libpod
 
 type containerPlatformState struct{}
-
-func (ctr *Container) setNamespace(netNSPath string, newState *containerState) error {
-	return ErrNotImplemented
-}
-
-func (ctr *Container) setNamespaceStatePath() string {
-	return ""
-}
diff --git a/libpod/state.go b/libpod/state.go
index 1b82349b3..273e81318 100644
--- a/libpod/state.go
+++ b/libpod/state.go
@@ -1,6 +1,18 @@
 package libpod
 
-// State is a storage backend for libpod's current state
+// State is a storage backend for libpod's current state.
+// A State is only initialized once per instance of libpod.
+// As such, initialization methods for State implementations may safely assume
+// they will be run as a singleton.
+// For all container and pod retrieval methods, a State must retrieve the
+// Configuration struct of the container or pod and include it in the returned
+// struct. The State of the container or pod may optionally be included as well,
+// but this is not a requirement.
+// As such, all containers and pods must be synced with the database via the
+// UpdateContainer and UpdatePod calls before any state-specific information is
+// retrieved after they are pulled from the database.
+// Generally speaking, the syncContainer() call should be run at the beginning
+// of all API operations, which will silently handle this.
 type State interface {
 	// Close performs any pre-exit cleanup (e.g. closing database
 	// connections) that may be required