Make podman update changes persistent

The logic here is more complex than I would like, largely due to
the behavior of `podman inspect` for running containers. When a
container is running, `podman inspect` will source as much as
possible from the OCI spec used to run that container, to grab
up-to-date information on things like devices. We don't want to
change this, it's definitely the right behavior, but it does make
updating a running container inconvenient: we have to rewrite the
OCI spec as part of the update to make sure that `podman inspect`
will read the correct resource limits.

Also, make update emit events. Docker does it, we should as well.

Signed-off-by: Matt Heon <mheon@redhat.com>

cmd/podman/common/completion.go

@@ -1423,7 +1423,7 @@ func AutocompleteEventFilter(cmd *cobra.Command, args []string, toComplete strin
 			events.PullError.String(), events.Push.String(), events.Refresh.String(), events.Remove.String(),
 			events.Rename.String(), events.Renumber.String(), events.Restart.String(), events.Restore.String(),
 			events.Save.String(), events.Start.String(), events.Stop.String(), events.Sync.String(), events.Tag.String(),
-			events.Unmount.String(), events.Unpause.String(), events.Untag.String(),
+			events.Unmount.String(), events.Unpause.String(), events.Untag.String(), events.Update.String(),
 		}, cobra.ShellCompDirectiveNoFileComp
 	}
 	eventTypes := func(_ string) ([]string, cobra.ShellCompDirective) {

docs/source/markdown/podman-events.1.md

@@ -47,6 +47,7 @@ The *container* event type reports the follow statuses:
  * sync
  * unmount
  * unpause
+ * update
 
 The *pod* event type reports the follow statuses:
  * create

docs/source/markdown/podman-update.1.md.in

@@ -1,7 +1,7 @@
 % podman-update 1
 
 ## NAME
-podman\-update - Update the cgroup configuration of a given container
+podman\-update - Update the configuration of a given container
 
 ## SYNOPSIS
 **podman update** [*options*] *container*
@@ -10,10 +10,8 @@ podman\-update - Update the cgroup configuration of a given container
 
 ## DESCRIPTION
 
-Updates the cgroup configuration of an already existing container. The currently supported options are a subset of the
-podman create/run resource limits options. These new options are non-persistent and only last for the current execution of the container; the configuration is honored on its next run.
-This means that this command can only be executed on an already running container and the changes made is erased the next time the container is stopped and restarted, this is to ensure immutability.
-This command takes one argument, a container name or ID, alongside the resource flags to modify the cgroup.
+Updates the configuration of an already existing container, allowing different resource limits to be set.
+The currently supported options are a subset of the podman create/run resource limit options.
 
 ## OPTIONS
 

docs/source/markdown/podman.1.md

@@ -384,7 +384,7 @@ the exit codes follow the `chroot` standard, see below:
 | [podman-unpause(1)](podman-unpause.1.md)         | Unpause one or more containers.                                             |
 | [podman-unshare(1)](podman-unshare.1.md)         | Run a command inside of a modified user namespace.                          |
 | [podman-untag(1)](podman-untag.1.md)             | Remove one or more names from a locally-stored image.                       |
-| [podman-update(1)](podman-update.1.md)           | Update the cgroup configuration of a given container.                       |
+| [podman-update(1)](podman-update.1.md)           | Update the configuration of a given container.                              |
 | [podman-version(1)](podman-version.1.md)         | Display the Podman version information.                                     |
 | [podman-volume(1)](podman-volume.1.md)           | Simple management tool for volumes.                                         |
 | [podman-wait(1)](podman-wait.1.md)               | Wait on one or more containers to stop and print their exit codes.          |

libpod/container_api.go

@@ -121,9 +121,19 @@ func (c *Container) Start(ctx context.Context, recursive bool) (finalErr error)
 // Update updates the given container.
 // only the cgroup config can be updated and therefore only a linux resource spec is passed.
 func (c *Container) Update(res *spec.LinuxResources) error {
-	if err := c.syncContainer(); err != nil {
-		return err
+	if !c.batched {
+		c.lock.Lock()
+		defer c.lock.Unlock()
+
+		if err := c.syncContainer(); err != nil {
+			return err
+		}
 	}
+
+	if c.ensureState(define.ContainerStateRemoving) {
+		return fmt.Errorf("container %s is being removed, cannot update: %w", c.ID(), define.ErrCtrStateInvalid)
+	}
+
 	return c.update(res)
 }
 

libpod/container_internal.go

@@ -2514,11 +2514,44 @@ func (c *Container) extractSecretToCtrStorage(secr *ContainerSecret) error {
 	return nil
 }
 
-// update calls the ociRuntime update function to modify a cgroup config after container creation
+// Update a container's resources after creation
 func (c *Container) update(resources *spec.LinuxResources) error {
-	if err := c.ociRuntime.UpdateContainer(c, resources); err != nil {
+	oldResources := c.config.Spec.Linux.Resources
+
+	if c.config.Spec.Linux == nil {
+		c.config.Spec.Linux = new(spec.Linux)
+	}
+	c.config.Spec.Linux.Resources = resources
+
+	if err := c.runtime.state.SafeRewriteContainerConfig(c, "", "", c.config); err != nil {
+		// Assume DB write failed, revert to old resources block
+		c.config.Spec.Linux.Resources = oldResources
 		return err
 	}
+
+	if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStatePaused) {
+		// So `podman inspect` on running containers sources its OCI spec from disk.
+		// To keep inspect accurate we need to update the on-disk OCI spec.
+		onDiskSpec, err := c.specFromState()
+		if err != nil {
+			return fmt.Errorf("retrieving on-disk OCI spec to update: %w", err)
+		}
+		if onDiskSpec.Linux == nil {
+			onDiskSpec.Linux = new(spec.Linux)
+		}
+		onDiskSpec.Linux.Resources = resources
+		if err := c.saveSpec(onDiskSpec); err != nil {
+			logrus.Errorf("Unable to update container %s OCI spec - `podman inspect` may not be accurate until container is restarted: %v", c.ID(), err)
+		}
+
+		if err := c.ociRuntime.UpdateContainer(c, resources); err != nil {
+			return err
+		}
+	}
+
 	logrus.Debugf("updated container %s", c.ID())
+
+	c.newContainerEvent(events.Update)
+
 	return nil
 }

libpod/events/config.go

@@ -208,6 +208,8 @@ const (
 	Unpause Status = "unpause"
 	// Untag ...
 	Untag Status = "untag"
+	// Update indicates that a container's configuration has been modified.
+	Update Status = "update"
 )
 
 // EventFilter for filtering events

libpod/events/events.go

@@ -231,6 +231,8 @@ func StringToStatus(name string) (Status, error) {
 		return Unpause, nil
 	case Untag.String():
 		return Untag, nil
+	case Update.String():
+		return Update, nil
 	}
 	return "", fmt.Errorf("unknown event status %q", name)
 }

test/e2e/update_test.go

@@ -218,4 +218,74 @@ var _ = Describe("Podman update", func() {
 		Expect(session).Should(ExitCleanly())
 		Expect(session.OutputToString()).Should(ContainSubstring("500000"))
 	})
+
+	It("podman update persists changes", func() {
+		SkipIfCgroupV1("testing flags that only work in cgroup v2")
+		SkipIfRootless("many of these handlers are not enabled while rootless in CI")
+
+		testCtr := "test-ctr-name"
+		ctr1 := podmanTest.Podman([]string{"run", "-d", "--name", testCtr, "-m", "512m", ALPINE, "top"})
+		ctr1.WaitWithDefaultTimeout()
+		Expect(ctr1).Should(ExitCleanly())
+
+		inspect1 := podmanTest.Podman([]string{"inspect", "--format", "{{ .HostConfig.Memory }}", testCtr})
+		inspect1.WaitWithDefaultTimeout()
+		Expect(inspect1).Should(ExitCleanly())
+		Expect(inspect1.OutputToString()).To(Equal("536870912"))
+
+		exec1 := podmanTest.Podman([]string{"exec", testCtr, "cat", "/sys/fs/cgroup/memory.max"})
+		exec1.WaitWithDefaultTimeout()
+		Expect(exec1).Should(ExitCleanly())
+		Expect(exec1.OutputToString()).Should(ContainSubstring("536870912"))
+
+		update := podmanTest.Podman([]string{"update", "-m", "256m", testCtr})
+		update.WaitWithDefaultTimeout()
+		Expect(update).Should(ExitCleanly())
+
+		inspect2 := podmanTest.Podman([]string{"inspect", "--format", "{{ .HostConfig.Memory }}", testCtr})
+		inspect2.WaitWithDefaultTimeout()
+		Expect(inspect2).Should(ExitCleanly())
+		Expect(inspect2.OutputToString()).To(Equal("268435456"))
+
+		exec2 := podmanTest.Podman([]string{"exec", testCtr, "cat", "/sys/fs/cgroup/memory.max"})
+		exec2.WaitWithDefaultTimeout()
+		Expect(exec2).Should(ExitCleanly())
+		Expect(exec2.OutputToString()).Should(ContainSubstring("268435456"))
+
+		restart := podmanTest.Podman([]string{"restart", testCtr})
+		restart.WaitWithDefaultTimeout()
+		Expect(restart).Should(ExitCleanly())
+
+		inspect3 := podmanTest.Podman([]string{"inspect", "--format", "{{ .HostConfig.Memory }}", testCtr})
+		inspect3.WaitWithDefaultTimeout()
+		Expect(inspect3).Should(ExitCleanly())
+		Expect(inspect3.OutputToString()).To(Equal("268435456"))
+
+		exec3 := podmanTest.Podman([]string{"exec", testCtr, "cat", "/sys/fs/cgroup/memory.max"})
+		exec3.WaitWithDefaultTimeout()
+		Expect(exec3).Should(ExitCleanly())
+		Expect(exec3.OutputToString()).Should(ContainSubstring("268435456"))
+
+		pause := podmanTest.Podman([]string{"pause", testCtr})
+		pause.WaitWithDefaultTimeout()
+		Expect(pause).Should(ExitCleanly())
+
+		update2 := podmanTest.Podman([]string{"update", "-m", "512m", testCtr})
+		update2.WaitWithDefaultTimeout()
+		Expect(update2).Should(ExitCleanly())
+
+		unpause := podmanTest.Podman([]string{"unpause", testCtr})
+		unpause.WaitWithDefaultTimeout()
+		Expect(unpause).Should(ExitCleanly())
+
+		inspect4 := podmanTest.Podman([]string{"inspect", "--format", "{{ .HostConfig.Memory }}", testCtr})
+		inspect4.WaitWithDefaultTimeout()
+		Expect(inspect4).Should(ExitCleanly())
+		Expect(inspect4.OutputToString()).To(Equal("536870912"))
+
+		exec4 := podmanTest.Podman([]string{"exec", testCtr, "cat", "/sys/fs/cgroup/memory.max"})
+		exec3.WaitWithDefaultTimeout()
+		Expect(exec4).Should(ExitCleanly())
+		Expect(exec4.OutputToString()).Should(ContainSubstring("536870912"))
+	})
 })