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

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())
 

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())

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)
+// 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 {
+			// 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 {
-			ctr.state.NetNS = netNS
+				newState.NetNS = ns
 			} else {
 				logrus.Errorf("error joining network namespace for container %s", ctr.ID())
-			return false
+				ctr.valid = false
 			}
 		}
-	return true
+	} 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
+}
+
+// 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 ""
 }

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
+// replaceNetNS is exclusive to the Linux platform and is a no-op elsewhere
+func replaceNetNS(netNSPath string, ctr *Container, newState *containerState) error {
+	return nil
 }
-	return true
+
+// getNetNSPath is exclusive to the Linux platform and is a no-op elsewhere
+func getNetNSPath(ctr *Container) string {
+	return
 }

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 ""
-}

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 ""
-}

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