Merge pull request containers#22322 from mheon/update_the_config

Make `podman update` changes persistent

Author: openshift-merge-bot[bot]

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

cmd/podman/common/create.go

@@ -643,15 +643,17 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 			`If a container with the same name exists, replace it`,
 		)
 	}
-	if mode == entities.InfraMode || (mode == entities.CreateMode) { // infra container flags, create should also pick these up
+	// Restart is allowed for created, updated, and infra ctr
+	if mode == entities.InfraMode || mode == entities.CreateMode || mode == entities.UpdateMode {
 		restartFlagName := "restart"
 		createFlags.StringVar(
 			&cf.Restart,
 			restartFlagName, "",
 			`Restart policy to apply when a container exits ("always"|"no"|"never"|"on-failure"|"unless-stopped")`,
 		)
 		_ = cmd.RegisterFlagCompletionFunc(restartFlagName, AutocompleteRestartOption)
-
+	}
+	if mode == entities.InfraMode || (mode == entities.CreateMode) { // infra container flags, create should also pick these up
 		shmSizeFlagName := "shm-size"
 		createFlags.String(
 			shmSizeFlagName, shmSize(),

cmd/podman/containers/update.go

@@ -7,9 +7,11 @@ import (
 
 	"github.com/containers/podman/v5/cmd/podman/common"
 	"github.com/containers/podman/v5/cmd/podman/registry"
+	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/domain/entities"
 	"github.com/containers/podman/v5/pkg/specgen"
 	"github.com/containers/podman/v5/pkg/specgenutil"
+	"github.com/containers/podman/v5/pkg/util"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/spf13/cobra"
 )
@@ -70,6 +72,17 @@ func update(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	if updateOpts.Restart != "" {
+		policy, retries, err := util.ParseRestartPolicy(updateOpts.Restart)
+		if err != nil {
+			return err
+		}
+		s.RestartPolicy = policy
+		if policy == define.RestartPolicyOnFailure {
+			s.RestartRetries = &retries
+		}
+	}
+
 	// we need to pass the whole specgen since throttle devices are parsed later due to cross compat.
 	s.ResourceLimits, err = specgenutil.GetResources(s, &updateOpts)
 	if err != nil {

docs/source/markdown/options/restart.md

@@ -1,5 +1,5 @@
 ####> This option file is used in:
-####>   podman create, pod clone, pod create, run
+####>   podman create, pod clone, pod create, run, update
 ####> If file is edited, make sure the changes
 ####> are applicable to all of those.
 #### **--restart**=*policy*

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
 
@@ -55,6 +53,8 @@ This command takes one argument, a container name or ID, alongside the resource
 
 @@option pids-limit
 
+@@option restart
+
 
 ## EXAMPLEs
 

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

@@ -119,12 +119,24 @@ 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
+// Either resource limits or restart policy can be updated.
+// Either resourcs or restartPolicy must not be nil.
+// If restartRetries is not nil, restartPolicy must be set and must be "on-failure".
+func (c *Container) Update(resources *spec.LinuxResources, restartPolicy *string, restartRetries *uint) error {
+	if !c.batched {
+		c.lock.Lock()
+		defer c.lock.Unlock()
+
+		if err := c.syncContainer(); err != nil {
+			return err
+		}
 	}
-	return c.update(res)
+
+	if c.ensureState(define.ContainerStateRemoving) {
+		return fmt.Errorf("container %s is being removed, cannot update: %w", c.ID(), define.ErrCtrStateInvalid)
+	}
+
+	return c.update(resources, restartPolicy, restartRetries)
 }
 
 // StartAndAttach starts a container and attaches to it.

libpod/container_inspect.go

@@ -467,6 +467,9 @@ func (c *Container) generateInspectContainerHostConfig(ctrSpec *spec.Spec, named
 
 	restartPolicy := new(define.InspectRestartPolicy)
 	restartPolicy.Name = c.config.RestartPolicy
+	if restartPolicy.Name == "" {
+		restartPolicy.Name = define.RestartPolicyNo
+	}
 	restartPolicy.MaximumRetryCount = c.config.RestartRetries
 	hostConfig.RestartPolicy = restartPolicy
 	if c.config.NoCgroups {

libpod/container_internal.go

@@ -2514,11 +2514,77 @@ func (c *Container) extractSecretToCtrStorage(secr *ContainerSecret) error {
 	return nil
 }
 
-// update calls the ociRuntime update function to modify a cgroup config after container creation
-func (c *Container) update(resources *spec.LinuxResources) error {
-	if err := c.ociRuntime.UpdateContainer(c, resources); err != nil {
+// Update a container's resources or restart policy after creation.
+// At least one of resources or restartPolicy must not be nil.
+func (c *Container) update(resources *spec.LinuxResources, restartPolicy *string, restartRetries *uint) error {
+	if resources == nil && restartPolicy == nil {
+		return fmt.Errorf("must provide at least one of resources and restartPolicy to update a container: %w", define.ErrInvalidArg)
+	}
+	if restartRetries != nil && restartPolicy == nil {
+		return fmt.Errorf("must provide restart policy if updating restart retries: %w", define.ErrInvalidArg)
+	}
+
+	oldResources := c.config.Spec.Linux.Resources
+	oldRestart := c.config.RestartPolicy
+	oldRetries := c.config.RestartRetries
+
+	if restartPolicy != nil {
+		if err := define.ValidateRestartPolicy(*restartPolicy); err != nil {
+			return err
+		}
+
+		if restartRetries != nil {
+			if *restartPolicy != define.RestartPolicyOnFailure {
+				return fmt.Errorf("cannot set restart policy retries unless policy is on-failure: %w", define.ErrInvalidArg)
+			}
+		}
+
+		c.config.RestartPolicy = *restartPolicy
+		if restartRetries != nil {
+			c.config.RestartRetries = *restartRetries
+		} else {
+			c.config.RestartRetries = 0
+		}
+	}
+
+	if resources != nil {
+		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
+		c.config.RestartPolicy = oldRestart
+		c.config.RestartRetries = oldRetries
 		return err
 	}
+
+	if c.ensureState(define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStatePaused) && resources != nil {
+		// 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/define/container.go

@@ -1,5 +1,9 @@
 package define
 
+import (
+	"fmt"
+)
+
 // Valid restart policy types.
 const (
 	// RestartPolicyNone indicates that no restart policy has been requested
@@ -27,6 +31,16 @@ var RestartPolicyMap = map[string]string{
 	RestartPolicyUnlessStopped: RestartPolicyUnlessStopped,
 }
 
+// Validate that the given string is a valid restart policy.
+func ValidateRestartPolicy(policy string) error {
+	switch policy {
+	case RestartPolicyNone, RestartPolicyNo, RestartPolicyOnFailure, RestartPolicyAlways, RestartPolicyUnlessStopped:
+		return nil
+	default:
+		return fmt.Errorf("%q is not a valid restart policy: %w", policy, ErrInvalidArg)
+	}
+}
+
 // InitContainerTypes
 const (
 	// AlwaysInitContainer is an init container that runs on each

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

libpod/options.go

@@ -1392,13 +1392,12 @@ func WithRestartPolicy(policy string) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		switch policy {
-		case define.RestartPolicyNone, define.RestartPolicyNo, define.RestartPolicyOnFailure, define.RestartPolicyAlways, define.RestartPolicyUnlessStopped:
-			ctr.config.RestartPolicy = policy
-		default:
-			return fmt.Errorf("%q is not a valid restart policy: %w", policy, define.ErrInvalidArg)
+		if err := define.ValidateRestartPolicy(policy); err != nil {
+			return err
 		}
 
+		ctr.config.RestartPolicy = policy
+
 		return nil
 	}
 }