Fix device limitations in podman-remote update on remote systems

Fixes: https://issues.redhat.com/browse/RUN-2381

Signed-off-by: Jan Rodák <hony.com@seznam.cz>

cmd/podman/containers/update.go

@@ -113,6 +113,16 @@ func GetChangedHealthCheckConfiguration(cmd *cobra.Command, vals *entities.Conta
 	return updateHealthCheckConfig
 }
 
+func GetChangedDeviceLimits(s *specgen.SpecGenerator) *define.UpdateContainerDevicesLimits {
+	updateDevicesLimits := define.UpdateContainerDevicesLimits{}
+	updateDevicesLimits.SetBlkIOWeightDevice(s.WeightDevice)
+	updateDevicesLimits.SetDeviceReadBPs(s.ThrottleReadBpsDevice)
+	updateDevicesLimits.SetDeviceWriteBPs(s.ThrottleWriteBpsDevice)
+	updateDevicesLimits.SetDeviceReadIOPs(s.ThrottleReadIOPSDevice)
+	updateDevicesLimits.SetDeviceWriteIOPs(s.ThrottleWriteIOPSDevice)
+	return &updateDevicesLimits
+}
+
 func update(cmd *cobra.Command, args []string) error {
 	var err error
 	// use a specgen since this is the easiest way to hold resource info
@@ -124,33 +134,35 @@ 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 {
 		return err
 	}
 
-	healthCheckConfig := GetChangedHealthCheckConfiguration(cmd, &updateOpts)
-	if err != nil {
-		return err
+	if s.ResourceLimits == nil {
+		s.ResourceLimits = &specs.LinuxResources{}
 	}
 
+	healthCheckConfig := GetChangedHealthCheckConfiguration(cmd, &updateOpts)
+
 	opts := &entities.ContainerUpdateOptions{
 		NameOrID:                        strings.TrimPrefix(args[0], "/"),
-		Specgen:                         s,
+		Resources:                       s.ResourceLimits,
 		ChangedHealthCheckConfiguration: &healthCheckConfig,
+		DevicesLimits:                   GetChangedDeviceLimits(s),
 	}
+
+	if cmd.Flags().Changed("restart") {
+		policy, retries, err := util.ParseRestartPolicy(updateOpts.Restart)
+		if err != nil {
+			return err
+		}
+		opts.RestartPolicy = &policy
+		if policy == define.RestartPolicyOnFailure {
+			opts.RestartRetries = &retries
+		}
+	}
+
 	rep, err := registry.ContainerEngine().ContainerUpdate(context.Background(), opts)
 	if err != nil {
 		return err

libpod/define/container.go

@@ -2,6 +2,8 @@ package define
 
 import (
 	"fmt"
+
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 // Valid restart policy types.
@@ -64,3 +66,111 @@ const (
 	// a Job kube yaml spec
 	K8sKindJob = "job"
 )
+
+type WeightDevice struct {
+	Path   string
+	Weight uint16
+}
+
+type ThrottleDevice struct {
+	Path string
+	Rate uint64
+}
+
+type UpdateContainerDevicesLimits struct {
+	// Block IO weight (relative device weight) in the form:
+	// ```[{"Path": "device_path", "Weight": weight}]```
+	BlkIOWeightDevice []WeightDevice `json:",omitempty"`
+	// Limit read rate (bytes per second) from a device, in the form:
+	// ```[{"Path": "device_path", "Rate": rate}]```
+	DeviceReadBPs []ThrottleDevice `json:",omitempty"`
+	// Limit write rate (bytes per second) to a device, in the form:
+	// ```[{"Path": "device_path", "Rate": rate}]```
+	DeviceWriteBPs []ThrottleDevice `json:",omitempty"`
+	// Limit read rate (IO per second) from a device, in the form:
+	// ```[{"Path": "device_path", "Rate": rate}]```
+	DeviceReadIOPs []ThrottleDevice `json:",omitempty"`
+	// Limit write rate (IO per second) to a device, in the form:
+	// ```[{"Path": "device_path", "Rate": rate}]```
+	DeviceWriteIOPs []ThrottleDevice `json:",omitempty"`
+}
+
+func (d *WeightDevice) addToLinuxWeightDevice(wd map[string]specs.LinuxWeightDevice) {
+	wd[d.Path] = specs.LinuxWeightDevice{
+		Weight:     &d.Weight,
+		LeafWeight: nil,
+	}
+}
+
+func LinuxWeightDeviceToWeightDevice(path string, d specs.LinuxWeightDevice) WeightDevice {
+	return WeightDevice{Path: path, Weight: *d.Weight}
+}
+
+func (d *ThrottleDevice) addToLinuxThrottleDevice(td map[string]specs.LinuxThrottleDevice) {
+	td[d.Path] = specs.LinuxThrottleDevice{Rate: d.Rate}
+}
+
+func LinuxThrottleDevicesToThrottleDevices(path string, d specs.LinuxThrottleDevice) ThrottleDevice {
+	return ThrottleDevice{Path: path, Rate: d.Rate}
+}
+
+func (u *UpdateContainerDevicesLimits) SetBlkIOWeightDevice(wd map[string]specs.LinuxWeightDevice) {
+	for path, dev := range wd {
+		u.BlkIOWeightDevice = append(u.BlkIOWeightDevice, LinuxWeightDeviceToWeightDevice(path, dev))
+	}
+}
+
+func copyLinuxThrottleDevicesFromMapToThrottleDevicesArray(source map[string]specs.LinuxThrottleDevice, dest []ThrottleDevice) []ThrottleDevice {
+	for path, dev := range source {
+		dest = append(dest, LinuxThrottleDevicesToThrottleDevices(path, dev))
+	}
+	return dest
+}
+
+func (u *UpdateContainerDevicesLimits) SetDeviceReadBPs(td map[string]specs.LinuxThrottleDevice) {
+	u.DeviceReadBPs = copyLinuxThrottleDevicesFromMapToThrottleDevicesArray(td, u.DeviceReadBPs)
+}
+
+func (u *UpdateContainerDevicesLimits) SetDeviceWriteBPs(td map[string]specs.LinuxThrottleDevice) {
+	u.DeviceWriteBPs = copyLinuxThrottleDevicesFromMapToThrottleDevicesArray(td, u.DeviceWriteBPs)
+}
+
+func (u *UpdateContainerDevicesLimits) SetDeviceReadIOPs(td map[string]specs.LinuxThrottleDevice) {
+	u.DeviceReadIOPs = copyLinuxThrottleDevicesFromMapToThrottleDevicesArray(td, u.DeviceReadIOPs)
+}
+
+func (u *UpdateContainerDevicesLimits) SetDeviceWriteIOPs(td map[string]specs.LinuxThrottleDevice) {
+	u.DeviceWriteIOPs = copyLinuxThrottleDevicesFromMapToThrottleDevicesArray(td, u.DeviceWriteIOPs)
+}
+
+func (u *UpdateContainerDevicesLimits) GetMapOfLinuxWeightDevice() map[string]specs.LinuxWeightDevice {
+	wd := make(map[string]specs.LinuxWeightDevice)
+	for _, dev := range u.BlkIOWeightDevice {
+		dev.addToLinuxWeightDevice(wd)
+	}
+	return wd
+}
+
+func getMapOfLinuxThrottleDevices(source []ThrottleDevice) map[string]specs.LinuxThrottleDevice {
+	td := make(map[string]specs.LinuxThrottleDevice)
+	for _, dev := range source {
+		dev.addToLinuxThrottleDevice(td)
+	}
+	return td
+}
+
+func (u *UpdateContainerDevicesLimits) GetMapOfDeviceReadBPs() map[string]specs.LinuxThrottleDevice {
+	return getMapOfLinuxThrottleDevices(u.DeviceReadBPs)
+}
+
+func (u *UpdateContainerDevicesLimits) GetMapOfDeviceWriteBPs() map[string]specs.LinuxThrottleDevice {
+	return getMapOfLinuxThrottleDevices(u.DeviceWriteBPs)
+}
+
+func (u *UpdateContainerDevicesLimits) GetMapOfDeviceReadIOPs() map[string]specs.LinuxThrottleDevice {
+	return getMapOfLinuxThrottleDevices(u.DeviceReadIOPs)
+}
+
+func (u *UpdateContainerDevicesLimits) GetMapOfDeviceWriteIOPs() map[string]specs.LinuxThrottleDevice {
+	return getMapOfLinuxThrottleDevices(u.DeviceWriteIOPs)
+}

pkg/api/handlers/libpod/containers.go

@@ -18,6 +18,7 @@ import (
 	api "github.com/containers/podman/v5/pkg/api/types"
 	"github.com/containers/podman/v5/pkg/domain/entities"
 	"github.com/containers/podman/v5/pkg/domain/infra/abi"
+	"github.com/containers/podman/v5/pkg/specgenutil"
 	"github.com/containers/podman/v5/pkg/util"
 	"github.com/gorilla/schema"
 	"github.com/sirupsen/logrus"
@@ -447,7 +448,14 @@ func UpdateContainer(w http.ResponseWriter, r *http.Request) {
 		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("decode(): %w", err))
 		return
 	}
-	err = ctr.Update(&options.LinuxResources, restartPolicy, restartRetries, &options.UpdateHealthCheckConfig)
+
+	resourceLimits, err := specgenutil.UpdateMajorAndMinorNumbers(&options.LinuxResources, &options.UpdateContainerDevicesLimits)
+	if err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+
+	err = ctr.Update(resourceLimits, restartPolicy, restartRetries, &options.UpdateHealthCheckConfig)
 	if err != nil {
 		utils.InternalServerError(w, err)
 		return

pkg/api/handlers/swagger/responses.go

@@ -324,9 +324,13 @@ type containerCreateResponse struct {
 	Body entities.ContainerCreateResponse
 }
 
+// Update container
+// swagger:response
 type containerUpdateResponse struct {
 	// in:body
-	ID string
+	Body struct {
+		ID string
+	}
 }
 
 // Wait container

pkg/api/handlers/types.go

@@ -76,6 +76,7 @@ type LibpodContainersRmReport struct {
 type UpdateEntities struct {
 	specs.LinuxResources
 	define.UpdateHealthCheckConfig
+	define.UpdateContainerDevicesLimits
 }
 
 type Info struct {

pkg/api/server/register_containers.go

@@ -1804,9 +1804,8 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
 	// produces:
 	// - application/json
 	// responses:
-	//   responses:
-	//     201:
-	//       $ref: "#/responses/containerUpdateResponse"
+	//   201:
+	//     $ref: "#/responses/containerUpdateResponse"
 	//   400:
 	//     $ref: "#/responses/badParamError"
 	//   404:

pkg/bindings/containers/update.go

@@ -20,15 +20,16 @@ func Update(ctx context.Context, options *types.ContainerUpdateOptions) (string,
 	}
 
 	params := url.Values{}
-	if options.Specgen.RestartPolicy != "" {
-		params.Set("restartPolicy", options.Specgen.RestartPolicy)
-		if options.Specgen.RestartRetries != nil {
-			params.Set("restartRetries", strconv.Itoa(int(*options.Specgen.RestartRetries)))
+	if options.RestartPolicy != nil {
+		params.Set("restartPolicy", *options.RestartPolicy)
+		if options.RestartRetries != nil {
+			params.Set("restartRetries", strconv.Itoa(int(*options.RestartRetries)))
 		}
 	}
 	updateEntities := &handlers.UpdateEntities{
-		LinuxResources:          *options.Specgen.ResourceLimits,
-		UpdateHealthCheckConfig: *options.ChangedHealthCheckConfiguration,
+		LinuxResources:               *options.Resources,
+		UpdateHealthCheckConfig:      *options.ChangedHealthCheckConfiguration,
+		UpdateContainerDevicesLimits: *options.DevicesLimits,
 	}
 	requestData, err := jsoniter.MarshalToString(updateEntities)
 	if err != nil {

pkg/domain/entities/types/containers.go

@@ -3,6 +3,7 @@ package types
 import (
 	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/specgen"
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 type ContainerCopyFunc func() error
@@ -36,7 +37,50 @@ type ContainerStatsReport struct {
 }
 
 type ContainerUpdateOptions struct {
-	NameOrID                        string
+	NameOrID string
+	// This individual items of Specgen are used to update container configuration:
+	// - ResourceLimits
+	// - RestartPolicy
+	// - RestartRetries
+	//
+	// Deprecated: Specgen should not be used to change the container configuration.
+	// Specgen is processed first, so values will be overwritten by values from other fields.
+	//
+	// To change configuration use other fields in ContainerUpdateOptions struct:
+	// - Resources to change resource configuration
+	// - DevicesLimits to Limit device
+	// - RestartPolicy to change restart policy
+	// - RestartRetries to change restart retries
 	Specgen                         *specgen.SpecGenerator
+	Resources                       *specs.LinuxResources
+	DevicesLimits                   *define.UpdateContainerDevicesLimits
 	ChangedHealthCheckConfiguration *define.UpdateHealthCheckConfig
+	RestartPolicy                   *string
+	RestartRetries                  *uint
+}
+
+func (u *ContainerUpdateOptions) ProcessSpecgen() {
+	if u.Specgen == nil {
+		return
+	}
+
+	if u.Resources == nil {
+		u.Resources = u.Specgen.ResourceLimits
+	}
+
+	if u.DevicesLimits == nil {
+		u.DevicesLimits = new(define.UpdateContainerDevicesLimits)
+		u.DevicesLimits.SetBlkIOWeightDevice(u.Specgen.WeightDevice)
+		u.DevicesLimits.SetDeviceReadBPs(u.Specgen.ThrottleReadBpsDevice)
+		u.DevicesLimits.SetDeviceWriteBPs(u.Specgen.ThrottleWriteBpsDevice)
+		u.DevicesLimits.SetDeviceReadIOPs(u.Specgen.ThrottleReadIOPSDevice)
+		u.DevicesLimits.SetDeviceWriteIOPs(u.Specgen.ThrottleWriteIOPSDevice)
+	}
+
+	if u.RestartPolicy == nil {
+		u.RestartPolicy = &u.Specgen.RestartPolicy
+	}
+	if u.RestartRetries == nil {
+		u.RestartRetries = u.Specgen.RestartRetries
+	}
 }

pkg/domain/infra/abi/containers.go

@@ -1792,14 +1792,7 @@ func (ic *ContainerEngine) ContainerClone(ctx context.Context, ctrCloneOpts enti
 
 // ContainerUpdate finds and updates the given container's cgroup config with the specified options
 func (ic *ContainerEngine) ContainerUpdate(ctx context.Context, updateOptions *entities.ContainerUpdateOptions) (string, error) {
-	err := specgen.WeightDevices(updateOptions.Specgen)
-	if err != nil {
-		return "", err
-	}
-	err = specgen.FinishThrottleDevices(updateOptions.Specgen)
-	if err != nil {
-		return "", err
-	}
+	updateOptions.ProcessSpecgen()
 	containers, err := getContainers(ic.Libpod, getContainersOptions{names: []string{updateOptions.NameOrID}})
 	if err != nil {
 		return "", err
@@ -1809,12 +1802,12 @@ func (ic *ContainerEngine) ContainerUpdate(ctx context.Context, updateOptions *e
 	}
 	container := containers[0].Container
 
-	var restartPolicy *string
-	if updateOptions.Specgen.RestartPolicy != "" {
-		restartPolicy = &updateOptions.Specgen.RestartPolicy
+	resourceLimits, err := specgenutil.UpdateMajorAndMinorNumbers(updateOptions.Resources, updateOptions.DevicesLimits)
+	if err != nil {
+		return "", err
 	}
 
-	if err = container.Update(updateOptions.Specgen.ResourceLimits, restartPolicy, updateOptions.Specgen.RestartRetries, updateOptions.ChangedHealthCheckConfiguration); err != nil {
+	if err = container.Update(resourceLimits, updateOptions.RestartPolicy, updateOptions.RestartRetries, updateOptions.ChangedHealthCheckConfiguration); err != nil {
 		return "", err
 	}
 	return containers[0].ID(), nil

pkg/domain/infra/tunnel/containers.go

@@ -1061,13 +1061,6 @@ func (ic *ContainerEngine) ContainerClone(ctx context.Context, ctrCloneOpts enti
 
 // ContainerUpdate finds and updates the given container's cgroup config with the specified options
 func (ic *ContainerEngine) ContainerUpdate(ctx context.Context, updateOptions *entities.ContainerUpdateOptions) (string, error) {
-	err := specgen.WeightDevices(updateOptions.Specgen)
-	if err != nil {
-		return "", err
-	}
-	err = specgen.FinishThrottleDevices(updateOptions.Specgen)
-	if err != nil {
-		return "", err
-	}
+	updateOptions.ProcessSpecgen()
 	return containers.Update(ic.ClientCtx, updateOptions)
 }

pkg/specgenutil/specgen.go

@@ -1297,3 +1297,27 @@ func GetResources(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions)
 	}
 	return s.ResourceLimits, nil
 }
+
+func UpdateMajorAndMinorNumbers(resources *specs.LinuxResources, devicesLimits *define.UpdateContainerDevicesLimits) (*specs.LinuxResources, error) {
+	spec := specgen.SpecGenerator{}
+	spec.ResourceLimits = &specs.LinuxResources{}
+	if resources != nil {
+		spec.ResourceLimits = resources
+	}
+
+	spec.WeightDevice = devicesLimits.GetMapOfLinuxWeightDevice()
+	spec.ThrottleReadBpsDevice = devicesLimits.GetMapOfDeviceReadBPs()
+	spec.ThrottleWriteBpsDevice = devicesLimits.GetMapOfDeviceWriteBPs()
+	spec.ThrottleReadIOPSDevice = devicesLimits.GetMapOfDeviceReadIOPs()
+	spec.ThrottleWriteIOPSDevice = devicesLimits.GetMapOfDeviceWriteIOPs()
+
+	err := specgen.WeightDevices(&spec)
+	if err != nil {
+		return nil, err
+	}
+	err = specgen.FinishThrottleDevices(&spec)
+	if err != nil {
+		return nil, err
+	}
+	return spec.ResourceLimits, nil
+}

test/apiv2/20-containers.at

@@ -686,19 +686,57 @@ t GET libpod/containers/$cname/json 200 \
 
 if root; then
   podman run -dt --name=updateCtr alpine
-  echo '{"Memory":{"Limit":500000}, "CPU":{"Shares":123}}' >${TMPD}/update.json
+  echo '{
+    "Memory":{"Limit":500000},
+    "CPU":{"Shares":123},
+    "DeviceReadBPs": [{ "Path": "/dev/zero", "Rate": 10485760 }],
+    "DeviceWriteBPs": [{ "Path": "/dev/zero", "Rate": 31457280 }],
+    "DeviceReadIOPs": [{ "Path": "/dev/zero", "Rate": 2000 }],
+    "DeviceWriteIOPs": [{ "Path": "/dev/zero", "Rate": 4000 }]
+    }' >${TMPD}/update.json
   t POST libpod/containers/updateCtr/update ${TMPD}/update.json 201
 
   cgroupPath=/sys/fs/cgroup/cpu.weight
   # 002 is the byte length
   cpu_weight_expect=$'\001\0025'
 
-  # Verify
+  # Verify CPU weight
   echo '{ "AttachStdout":true,"Cmd":["cat", "'$cgroupPath'"]}' >${TMPD}/exec.json
   t POST containers/updateCtr/exec ${TMPD}/exec.json 201 .Id~[0-9a-f]\\{64\\}
   eid=$(jq -r '.Id' <<<"$output")
   t POST exec/$eid/start 200 $cpu_weight_expect
 
+  BlkioDeviceReadBps_expected='[
+  {
+    "Path": "/dev/zero",
+    "Rate": 10485760
+  }
+]'
+  BlkioDeviceWriteBPs_expected='[
+  {
+    "Path": "/dev/zero",
+    "Rate": 31457280
+  }
+]'
+  BlkioDeviceReadIOPs_expected='[
+  {
+    "Path": "/dev/zero",
+    "Rate": 2000
+  }
+]'
+  BlkioDeviceWriteIOPs_expected='[
+  {
+    "Path": "/dev/zero",
+    "Rate": 4000
+  }
+]'
+  # Verify Device limits
+  t GET containers/updateCtr/json 200 \
+  .HostConfig.BlkioDeviceReadBps="$BlkioDeviceReadBps_expected"     \
+  .HostConfig.BlkioDeviceWriteBps="$BlkioDeviceWriteBPs_expected"   \
+  .HostConfig.BlkioDeviceReadIOps="$BlkioDeviceReadIOPs_expected"   \
+  .HostConfig.BlkioDeviceWriteIOps="$BlkioDeviceWriteIOPs_expected" \
+
   # Now use the compat API
   echo '{ "Memory": 536870912 }' >${TMPD}/compatupdate.json
   t POST containers/updateCtr/update ${TMPD}/compatupdate.json 200