Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cpu burst test failure on newer kernels #4374

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Aug 14, 2024

A kernel bug which resulted in cpu.max.burst value read which is 1000
times smaller than it should be has recently been fixed (see 1).

Adapt the test so it works with either broken or fixed kernel.

Fixes: #4373

🔔 For CI, see #4375.

1. Rename current -> got, expected -> want.
2. check_cgroup_value: add file name to output.
3. Improve functions description.

This is mostly to simplify debugging test failures.
Example output before:

	current 500000 !? 500

After:

	cpu.max.burst: got 500000, want 500

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A kernel bug which resulted in cpu.max.burst value read which is 1000
times smaller than it should be has recently been fixed (see [1]).

Adapt the test so it works with either broken or fixed kernel.

[1]: https://lore.kernel.org/all/20240424132438.514720-1-serein.chengyu@huawei.com/

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

No need to backport this PR to 1.1 as this functionality (initially added by #3749) was not backported.

@lifubang
Copy link
Member

lifubang commented Aug 14, 2024

I think you should add the commits in this PR to #4360, these two PRs are depended on each other.

@kolyshkin
Copy link
Contributor Author

I think you should add the commits in this PR to #4360, these two PRs are depended on each other.

Why do you think they are dependent? They are definitely not: this one is about fixing a test for newer kernel, while #4360 is about removing Go 1.21 and adding Go 1.23.

It just happened that we have CI broken in two places now, and each of these PRs fixes one of such places.

This is why I opened #4375 -- to see that CI is green again with both this PR and #4360 are in, and for reviewers of both this PR and #4360 to make sure it's green.

Note we don't have to have a green CI in order to merge a PR. So a plan is to have two LGTMs on either this one or #4360, merge one, rebase the other.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM, great finding!

I guess the upstream commit will be applied to all stable branches, so maybe in a few weeks/months we can remove this?

@rata
Copy link
Member

rata commented Aug 14, 2024

2 LGTM and @kolyshkin said we don't need to wait for CI to be green to merge this. This, combined with another PR, is what will make the CI green. Furthermore, cirrus (the only failing checks) failed to install dependencies.

Merging then. Please let me know if I missed something!

@rata rata merged commit 94eda74 into opencontainers:main Aug 14, 2024
38 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpu burst test failures on Ubuntu 24.04
4 participants