Rewrite Rename backend in a more atomic fashion

Move the core of renaming logic into the DB. This guarantees a
lot more atomicity than we have right now (our current solution,
removing the container from the DB and re-creating it, is *VERY*
not atomic and prone to leaving a corrupted state behind if
things go wrong. Moving things into the DB allows us to remove
most, but not all, of this - there's still a potential scenario
where the c/storage rename fails but the Podman rename succeeds,
and we end up with a mismatched state.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

libpod/boltdb_state.go

@@ -1681,6 +1681,104 @@ func (s *BoltState) RewriteContainerConfig(ctr *Container, newCfg *ContainerConf
 	return err
 }
 
+// SafeRewriteContainerConfig rewrites a container's configuration in a more
+// limited fashion than RewriteContainerConfig. It is marked as safe to use
+// under most circumstances, unlike RewriteContainerConfig.
+// DO NOT USE TO: Change container dependencies, change pod membership, change
+// locks, change container ID.
+func (s *BoltState) SafeRewriteContainerConfig(ctr *Container, oldName, newName string, newCfg *ContainerConfig) error {
+	if !s.valid {
+		return define.ErrDBClosed
+	}
+
+	if !ctr.valid {
+		return define.ErrCtrRemoved
+	}
+
+	if newName != "" && newCfg.Name != newName {
+		return errors.Wrapf(define.ErrInvalidArg, "new name %s for container %s must match name in given container config", newName, ctr.ID())
+	}
+	if newName != "" && oldName == "" {
+		return errors.Wrapf(define.ErrInvalidArg, "must provide old name for container if a new name is given")
+	}
+
+	newCfgJSON, err := json.Marshal(newCfg)
+	if err != nil {
+		return errors.Wrapf(err, "error marshalling new configuration JSON for container %s", ctr.ID())
+	}
+
+	db, err := s.getDBCon()
+	if err != nil {
+		return err
+	}
+	defer s.deferredCloseDBCon(db)
+
+	err = db.Update(func(tx *bolt.Tx) error {
+		if newName != "" {
+			idBkt, err := getIDBucket(tx)
+			if err != nil {
+				return err
+			}
+			namesBkt, err := getNamesBucket(tx)
+			if err != nil {
+				return err
+			}
+			allCtrsBkt, err := getAllCtrsBucket(tx)
+			if err != nil {
+				return err
+			}
+
+			needsRename := true
+			if exists := namesBkt.Get([]byte(newName)); exists != nil {
+				if string(exists) == ctr.ID() {
+					// Name already associated with the ID
+					// of this container. No need for a
+					// rename.
+					needsRename = false
+				} else {
+					return errors.Wrapf(define.ErrCtrExists, "name %s already in use, cannot rename container %s", newName, ctr.ID())
+				}
+			}
+
+			if needsRename {
+				// We do have to remove the old name. The other
+				// buckets are ID-indexed so we just need to
+				// overwrite the values there.
+				if err := namesBkt.Delete([]byte(oldName)); err != nil {
+					return errors.Wrapf(err, "error deleting container %s old name from DB for rename", ctr.ID())
+				}
+				if err := idBkt.Put([]byte(ctr.ID()), []byte(newName)); err != nil {
+					return errors.Wrapf(err, "error renaming container %s in ID bucket in DB", ctr.ID())
+				}
+				if err := namesBkt.Put([]byte(newName), []byte(ctr.ID())); err != nil {
+					return errors.Wrapf(err, "error adding new name %s for container %s in DB", newName, ctr.ID())
+				}
+				if err := allCtrsBkt.Put([]byte(ctr.ID()), []byte(newName)); err != nil {
+					return errors.Wrapf(err, "error renaming container %s in all containers bucket in DB", ctr.ID())
+				}
+			}
+		}
+
+		ctrBkt, err := getCtrBucket(tx)
+		if err != nil {
+			return err
+		}
+
+		ctrDB := ctrBkt.Bucket([]byte(ctr.ID()))
+		if ctrDB == nil {
+			ctr.valid = false
+			return errors.Wrapf(define.ErrNoSuchCtr, "no container with ID %s found in DB", ctr.ID())
+		}
+
+		if err := ctrDB.Put(configKey, newCfgJSON); err != nil {
+			return errors.Wrapf(err, "error updating container %s config JSON", ctr.ID())
+		}
+
+		return nil
+	})
+	return err
+}
+
 // RewritePodConfig rewrites a pod's configuration.
 // WARNING: This function is DANGEROUS. Do not use without reading the full
 // comment on this function in state.go.

libpod/in_memory_state.go

@@ -822,6 +822,46 @@ func (s *InMemoryState) RewriteContainerConfig(ctr *Container, newCfg *Container
 	return nil
 }
 
+// SafeRewriteContainerConfig rewrites a container's configuration.
+// It's safer than RewriteContainerConfig, but still has limitations. Please
+// read the comment in state.go before using.
+func (s *InMemoryState) SafeRewriteContainerConfig(ctr *Container, oldName, newName string, newCfg *ContainerConfig) error {
+	if !ctr.valid {
+		return define.ErrCtrRemoved
+	}
+
+	if _, err := s.nameIndex.Get(newName); err == nil {
+		return errors.Wrapf(define.ErrCtrExists, "name %s is in use", newName)
+	}
+
+	// If the container does not exist, return error
+	stateCtr, ok := s.containers[ctr.ID()]
+	if !ok {
+		ctr.valid = false
+		return errors.Wrapf(define.ErrNoSuchCtr, "container with ID %s not found in state", ctr.ID())
+	}
+
+	// Change name in registry.
+	if s.namespace != "" {
+		nsIndex, ok := s.namespaceIndexes[s.namespace]
+		if !ok {
+			return define.ErrInternal
+		}
+		nsIndex.nameIndex.Release(oldName)
+		if err := nsIndex.nameIndex.Reserve(newName, ctr.ID()); err != nil {
+			return errors.Wrapf(err, "error registering name %s", newName)
+		}
+	}
+	s.nameIndex.Release(oldName)
+	if err := s.nameIndex.Reserve(newName, ctr.ID()); err != nil {
+		return errors.Wrapf(err, "error registering name %s", newName)
+	}
+
+	stateCtr.config = newCfg
+
+	return nil
+}
+
 // RewritePodConfig rewrites a pod's configuration.
 // This function is DANGEROUS, even with in-memory state.
 // Please read the full comment on it in state.go before using it.

libpod/runtime_ctr.go

@@ -74,8 +74,7 @@ func (r *Runtime) RestoreContainer(ctx context.Context, rSpec *spec.Spec, config
 }
 
 // RenameContainer renames the given container.
-// The given container object will be rendered unusable, and a new, renamed
-// Container will be returned.
+// Returns a copy of the container that has been renamed if successful.
 func (r *Runtime) RenameContainer(ctx context.Context, ctr *Container, newName string) (*Container, error) {
 	ctr.lock.Lock()
 	defer ctr.lock.Unlock()
@@ -88,26 +87,6 @@ func (r *Runtime) RenameContainer(ctx context.Context, ctr *Container, newName s
 		return nil, define.RegexError
 	}
 
-	// Check if the name is available.
-	// This is *100% NOT ATOMIC* so any failures in-flight will do
-	// *VERY BAD THINGS* to the state. So we have to try and catch all we
-	// can before starting.
-	if _, err := r.state.LookupContainerID(newName); err == nil {
-		return nil, errors.Wrapf(define.ErrCtrExists, "name %s is already in use by another container", newName)
-	}
-	if _, err := r.state.LookupPod(newName); err == nil {
-		return nil, errors.Wrapf(define.ErrPodExists, "name %s is already in use by another pod", newName)
-	}
-
-	// TODO: Investigate if it is possible to remove this limitation.
-	depCtrs, err := r.state.ContainerInUse(ctr)
-	if err != nil {
-		return nil, err
-	}
-	if len(depCtrs) > 0 {
-		return nil, errors.Wrapf(define.ErrCtrExists, "cannot rename container %s as it is in use by other containers: %v", ctr.ID(), strings.Join(depCtrs, ","))
-	}
-
 	// We need to pull an updated config, in case another rename fired and
 	// the config was re-written.
 	newConf, err := r.state.GetContainerConfig(ctr.ID())
@@ -116,95 +95,33 @@ func (r *Runtime) RenameContainer(ctx context.Context, ctr *Container, newName s
 	}
 	ctr.config = newConf
 
-	// TODO: This is going to fail if we have active exec sessions, too.
-	// Investigate fixing that at a later date.
-
-	var pod *Pod
-	if ctr.config.Pod != "" {
-		tmpPod, err := r.state.Pod(ctr.config.Pod)
-		if err != nil {
-			return nil, errors.Wrapf(err, "error retrieving container %s pod", ctr.ID())
-		}
-		pod = tmpPod
-		// Lock pod to ensure it's not removed while we're working
-		pod.lock.Lock()
-		defer pod.lock.Unlock()
-	}
-
-	// Lock all volumes to ensure they are not removed while we're working
-	volsLocked := make(map[string]bool)
-	for _, namedVol := range ctr.config.NamedVolumes {
-		if volsLocked[namedVol.Name] {
-			continue
-		}
-		vol, err := r.state.Volume(namedVol.Name)
-		if err != nil {
-			return nil, errors.Wrapf(err, "error retrieving volume used by container %s", ctr.ID())
-		}
-
-		volsLocked[vol.Name()] = true
-		vol.lock.Lock()
-		defer vol.lock.Unlock()
-	}
-
 	logrus.Infof("Going to rename container %s from %q to %q", ctr.ID(), ctr.Name(), newName)
 
-	// Step 1: remove the old container.
-	if pod != nil {
-		if err := r.state.RemoveContainerFromPod(pod, ctr); err != nil {
-			return nil, errors.Wrapf(err, "error renaming container %s", ctr.ID())
-		}
-	} else {
-		if err := r.state.RemoveContainer(ctr); err != nil {
-			return nil, errors.Wrapf(err, "error renaming container %s", ctr.ID())
-		}
+	// Step 1: Alter the config. Save the old name, we need it to rewrite
+	// the config.
+	oldName := ctr.config.Name
+	ctr.config.Name = newName
+
+	// Step 2: rewrite the old container's config in the DB.
+	if err := r.state.SafeRewriteContainerConfig(ctr, oldName, ctr.config.Name, ctr.config); err != nil {
+		// Assume the rename failed.
+		// Set config back to the old name so reflect what is actually
+		// present in the DB.
+		ctr.config.Name = oldName
+		return nil, errors.Wrapf(err, "error renaming container %s", ctr.ID())
 	}
 
-	// Step 2: Make a new container based on the old one.
-	// TODO: Should we deep-copy the container config and state, to be safe?
-	newCtr := new(Container)
-	newCtr.config = ctr.config
-	newCtr.state = ctr.state
-	newCtr.lock = ctr.lock
-	newCtr.ociRuntime = ctr.ociRuntime
-	newCtr.runtime = r
-	newCtr.rootlessSlirpSyncR = ctr.rootlessSlirpSyncR
-	newCtr.rootlessSlirpSyncW = ctr.rootlessSlirpSyncW
-	newCtr.rootlessPortSyncR = ctr.rootlessPortSyncR
-	newCtr.rootlessPortSyncW = ctr.rootlessPortSyncW
-
-	newCtr.valid = true
-	newCtr.config.Name = newName
-
-	// Step 3: Add that new container to the DB
-	if pod != nil {
-		if err := r.state.AddContainerToPod(pod, newCtr); err != nil {
-			return nil, errors.Wrapf(err, "error renaming container %s", newCtr.ID())
-		}
-	} else {
-		if err := r.state.AddContainer(newCtr); err != nil {
-			return nil, errors.Wrapf(err, "error renaming container %s", newCtr.ID())
-		}
-	}
-
-	// Step 4: Save the new container, to force the state to be written to
-	// the DB. This may not be necessary, depending on DB implementation,
-	// but let's do it to be safe.
-	if err := newCtr.save(); err != nil {
-		return nil, err
-	}
-
-	// Step 5: rename the container in c/storage.
+	// Step 3: rename the container in c/storage.
 	// This can fail if the name is already in use by a non-Podman
 	// container. This puts us in a bad spot - we've already renamed the
 	// container in Podman. We can swap the order, but then we have the
 	// opposite problem. Atomicity is a real problem here, with no easy
 	// solution.
-	if err := r.store.SetNames(newCtr.ID(), []string{newCtr.Name()}); err != nil {
+	if err := r.store.SetNames(ctr.ID(), []string{ctr.Name()}); err != nil {
 		return nil, err
 	}
 
-	return newCtr, nil
+	return ctr, nil
 }
 
 func (r *Runtime) initContainerVariables(rSpec *spec.Spec, config *ContainerConfig) (*Container, error) {

libpod/state.go

@@ -155,6 +155,19 @@ type State interface {
 	// answer is this: use this only very sparingly, and only if you really
 	// know what you're doing.
 	RewriteContainerConfig(ctr *Container, newCfg *ContainerConfig) error
+	// This is a more limited version of RewriteContainerConfig, though it
+	// comes with the added ability to alter a container's name. In exchange
+	// it loses the ability to manipulate the container's locks.
+	// It is not intended to be as restrictive as RewriteContainerConfig, in
+	// that we allow it to be run while other Podman processes are running,
+	// and without holding the alive lock.
+	// Container ID and pod membership still *ABSOLUTELY CANNOT* be altered.
+	// Also, you cannot change a container's dependencies - shared namespace
+	// containers or generic dependencies - at present. This is
+	// theoretically possible but not yet implemented.
+	// If newName is not "" the container will be renamed to the new name.
+	// The oldName parameter is only required if newName is given.
+	SafeRewriteContainerConfig(ctr *Container, oldName, newName string, newCfg *ContainerConfig) error
 	// PLEASE READ THE DESCRIPTION FOR RewriteContainerConfig BEFORE USING.
 	// This function is identical to RewriteContainerConfig, save for the
 	// fact that it is used with pods instead.

test/e2e/rename_test.go

@@ -89,4 +89,25 @@ var _ = Describe("podman rename", func() {
 		Expect(ps.ExitCode()).To(Equal(0))
 		Expect(ps.OutputToString()).To(ContainSubstring(newName))
 	})
+
+	It("Rename a running container with exec sessions", func() {
+		ctrName := "testCtr"
+		ctr := podmanTest.Podman([]string{"run", "-d", "--name", ctrName, ALPINE, "top"})
+		ctr.WaitWithDefaultTimeout()
+		Expect(ctr.ExitCode()).To(Equal(0))
+
+		exec := podmanTest.Podman([]string{"exec", "-d", ctrName, "top"})
+		exec.WaitWithDefaultTimeout()
+		Expect(exec.ExitCode()).To(Equal(0))
+
+		newName := "aNewName"
+		rename := podmanTest.Podman([]string{"rename", ctrName, newName})
+		rename.WaitWithDefaultTimeout()
+		Expect(rename.ExitCode()).To(Equal(0))
+
+		ps := podmanTest.Podman([]string{"ps", "-aq", "--filter", fmt.Sprintf("name=%s", newName), "--format", "{{ .Names }}"})
+		ps.WaitWithDefaultTimeout()
+		Expect(ps.ExitCode()).To(Equal(0))
+		Expect(ps.OutputToString()).To(ContainSubstring(newName))
+	})
 })