runc update: support per-device weight and iops

[kolyshkin]

This support was missing from runc, and thus the example from the podman-update wasn't working.

To fix, introduce a function to either update or insert new weights and iops.

Add integration tests.

Fixes: https://issues.redhat.com/browse/RHEL-81042

See also: containers/podman#26308

[Copilot]

Pull Request Overview

This PR adds support for per-device weight and IOPS settings in runc by updating the configuration and introducing two helper functions (upsertWeightDevice and upsertThrottleDevice) to either update or insert device-specific resource limits. It also includes new integration tests to verify that both weight and IOPS updates are applied correctly.

• Added support for per-device weight and IOPS to the update configuration.
• Introduced helper functions upsertWeightDevice and upsertThrottleDevice.
• Expanded integration tests to cover the new functionality.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
update.go	Added per-device weight & IOPS update logic and helper functions
tests/integration/update.bats	New integration test for per-device iops/bps value updates
tests/integration/helpers.bash	Added check_cgroup_dev_iops helper for validating cgroup changes
tests/integration/cgroups.bats	Updated integration test for verifying updated weight settings

Comments suppressed due to low confidence (2)

update.go:404

• Consider adding a brief inline comment for upsertWeightDevice to describe its behavior, particularly regarding the update of the first matching device and appending a new device if no match is found.

func upsertWeightDevice(devices []*cgroups.WeightDevice, wd specs.LinuxWeightDevice) []*cgroups.WeightDevice {

update.go:431

• Consider adding a short comment to upsertThrottleDevice outlining that it updates the rate for the first matching device and appends a new throttle device when none match, to improve code clarity for future maintainers.

func upsertThrottleDevice(devices []*cgroups.ThrottleDevice, td specs.LinuxThrottleDevice) []*cgroups.ThrottleDevice {

[lifubang]

Overall LGTM.

One small suggestion: this loop might be better run in reverse order.

Currently, we don't deduplicate devices when parsing config.json, and we write to the cgroup filesystem one by one. As a result, the final rate value ends up being the last one in the config. For example::

diff --git a/tests/integration/update.bats b/tests/integration/update.bats
index 9e0c64bd..69496f0f 100644
--- a/tests/integration/update.bats
+++ b/tests/integration/update.bats
@@ -905,6 +905,10 @@ EOF
                echo "==="
                skip "can't get device major number from /proc/partitions (got $major)"
        fi
+       update_config '   .linux.resources.blockIO.throttleReadBpsDevice |= [
+                               { major: '"$major"', minor: 0, rate: 485760 },
+                               { major: '"$major"', minor: 0, rate: 485760 }
+                       ]'
 
        runc run -d --console-socket "$CONSOLE_SOCKET" test_update
        [ "$status" -eq 0 ]

[kolyshkin]

Makes sense, implemented as a separate commit. PTAL

[rata]

This mostly LGTM, left a comment that I don't really understand. I feel I might be missing something obvious, but I don't see it :D

[rata]

I've seen the comment, the commit msg, etc. I might be tired, but I don't see why to do it in reverse order or not.

If it's duplicated, the first value must take precedence instead of the last value? Is this on the spec? Intuitively, it seems like an error to me (the config.json contradicts itself).

[kolyshkin]

To add a bit of context, this was added as a response to @lifubang suggestion at: #4775 (comment)

The spec doesn't tell anything about duplicated entries (meaning that they are implicitly allowed, and the precedence rules are not documented either).

The runc behavior is/was to apply all rules one-by-one, so if there is a duplicated rule, the latter one overwrites the former. This is the reason why we iterate backwards.

In practice, though, it doesn't matter much, because if there are no duplicated entries, the order doesn't matter, and my infinite belief in human decency tells me no one has duplicated entries anyway. Yet, if someone does, this trick here saves them from a disaster.

[kolyshkin]

You can also run the test case from the second commit without the fix from the same commit to see what's happening if we iterate as usual.

[rata]

Yes, sorry, I wanted to say I read that too. But today, a little bit less tired, I think I see it. We return after setting it, I didn't realize the for loop ended when we found the entry. Sorry!

It is not exactly the same behavior as before, if we were processing all the rules, though. Because if one property (let's say weight) wasn't set in the last entry of the config, it would have the value of the previous entry that set it.

I think this is edgy enough that we might be able to change it without anyone realizing. So let's try it :)

[kolyshkin]

It is not exactly the same behavior as before, if we were processing all the rules, though. Because if one property (let's say weight) wasn't set in the last entry of the config, it would have the value of the previous entry that set it.

I saw that one, too, but I think no one is actually using LeafWeight these days, so practically we only have one property.

[rata]

LGTM, thanks!

[rata]

Yes, sorry, I wanted to say I read that too. But today, a little bit less tired, I think I see it. We return after setting it, I didn't realize the for loop ended when we found the entry. Sorry!

It is not exactly the same behavior as before, if we were processing all the rules, though. Because if one property (let's say weight) wasn't set in the last entry of the config, it would have the value of the previous entry that set it.

I think this is edgy enough that we might be able to change it without anyone realizing. So let's try it :)