Skip to content

tests/from.bats "from cpu-shares test": update cgroupv2 weights#6674

Merged
lsm5 merged 1 commit into
podman-container-tools:mainfrom
nalind:cpu-shares-weight
Mar 6, 2026
Merged

tests/from.bats "from cpu-shares test": update cgroupv2 weights#6674
lsm5 merged 1 commit into
podman-container-tools:mainfrom
nalind:cpu-shares-weight

Conversation

@nalind

@nalind nalind commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind other

What this PR does / why we need it:

Update the weights expected in the "from cpu-shares test" test to account for both the old way that runtimes would convert from cgroupsv1 cpu shares to cgroupsv2 cpu weights, and the new way, catching it up with the "bud with --cpu-shares, checked" test, which will now also check with a few different values.

How to verify it

Updated tests!

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 6, 2026
@packit-as-a-service

Copy link
Copy Markdown

Ephemeral COPR build failed. @containers/packit-build please check.

2 similar comments
@packit-as-a-service

Copy link
Copy Markdown

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service

Copy link
Copy Markdown

Ephemeral COPR build failed. @containers/packit-build please check.

@nalind

nalind commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

/packit rebuild-failed

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread tests/bud.bats Outdated
Comment on lines 6672 to 6673

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nalind do we need both of these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, neither. Dropped them.

@lsm5 lsm5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart from couple of nit questions.

Comment thread tests/from.bats Outdated
Comment on lines +218 to +228
# https://kubernetes.io/blog/2026/01/30/new-cgroup-v1-to-v2-cpu-conversion-formula/
# there's an old way to convert the value, and a new way to convert the value, and we
# don't know which one our runtime is using, so accept the values that either would
# compute for ${shares}
local oldconverted="$((1 + ((${shares} - 2) * 9999) / 262142))"
test -n "$oldconverted"
local oldexpect="weight ${oldconverted}"
local newconverted=$(awk '{if ($1 <= 2) { print "1"} else if ($1 >= 262144) {print "10000"} else {l=log($1)/log(2); e=((((l+125)*l)/612.0) - 7.0/34.0); p = exp(e*log(10)); if ( p == int(p) ) {print p} else { print int(p+1) }}}' <<< "${shares}")
test -n "$newconverted"
local newexpect="weight ${newconverted}"
local expect="($oldexpect|$newexpect)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block seems to be a repeat of tests/bud.bats. Can it be made reusable? Ignore comment if overkill.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factored into a helper function.

Comment thread tests/from.bats
Update the weights expected in the "from cpu-shares test" test to
account for both the old way that runtimes would convert from cgroupsv1
cpu shares to cgroupsv2 cpu weights, and the new way, catching it up
with the "bud with --cpu-shares, checked" test, which will now also
check with a few different values.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

LGTM
I restarted the flaked test

@lsm5 lsm5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nalind

nalind commented Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

I think that testing-farm:fedora-rawhide-x86_64:buildah-fedora failure's a bug. I can't quite figure how we didn't notice it at the time, but the helper the new tests in #6675 use isn't packaged in the RPM. I added a commit that should fix it to the tip of #6705 because that's where I noticed what was going on.

@lsm5

lsm5 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

I think that testing-farm:fedora-rawhide-x86_64:buildah-fedora failure's a bug. I can't quite figure how we didn't notice it at the time, but the helper the new tests in #6675 use isn't packaged in the RPM. I added a commit that should fix it to the tip of #6705 because that's where I noticed what was going on.

Was the rawhide test triggered 2 days ago separately? That would explain why only rawhide caught it and the others passed (those ran 2 weeks ago).

@lsm5

lsm5 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

I added a commit that should fix it to the tip of #6705 because that's where I noticed what was going on.

LGTM'd that one. Thanks!

@lsm5

lsm5 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

#6705 is merged. So, I guess we can ignore the CI failure here and merge this.

@lsm5
lsm5 merged commit c86654b into podman-container-tools:main Mar 6, 2026
39 of 40 checks passed
@nalind
nalind deleted the cpu-shares-weight branch March 6, 2026 14:52
@nalind

nalind commented Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

I think that testing-farm:fedora-rawhide-x86_64:buildah-fedora failure's a bug. I can't quite figure how we didn't notice it at the time, but the helper the new tests in #6675 use isn't packaged in the RPM. I added a commit that should fix it to the tip of #6705 because that's where I noticed what was going on.

Was the rawhide test triggered 2 days ago separately? That would explain why only rawhide caught it and the others passed (those ran 2 weeks ago).

I'm not sure I follow. We run it for every PR, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants