integration/container: migrate StatsNoStreamGetCPU test#52159
integration/container: migrate StatsNoStreamGetCPU test#52159deahtstroke wants to merge 1 commit intomoby:masterfrom
Conversation
deahtstroke
force-pushed
the
50159-migrate-testAPINoStreamGetCPU
branch
2 times, most recently
from
c2e741a to
088d262
Compare
|
There seems to be a race condition issue with oracle-linux 8 using Cgroups V1. Added polling for CPU stats to overcome this. Ran into the same issue in my other PR regarding network stats. |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
thanks! overall LGTM; I left two small comments / suggestions; no hard blockers, but let me know what you think!
|
|
||
| var res client.ContainerStatsResult | ||
|
|
||
| poll.WaitOn(t, func(t poll.LogT) poll.Result { |
There was a problem hiding this comment.
This was the poll added for the Oracle VM? If so, it could be good to add a very short comment (and/or link to your comment on GitHub?) to explain we (only?) needed it for that test-environment.
Having a bit of a breadcrumb to know "why did we need this?" can help deciding in future if it's still needed.
| } | ||
|
|
||
| return poll.Success() | ||
| }, poll.WithDelay(pollDelay), poll.WithTimeout(pollTimeout)) |
There was a problem hiding this comment.
Looks like those variables are only used here; perhaps would be fine to just inline them
|
CI still red, hmm... the other PR had a smaller timeout, but I see that is still erroring as well. Also im working on implementing the changes you suggested 😄 |
|
The VM tests are flaky in general, so could be unrelated, but yeah, possibly it needs tweaking. Also worth noting that they are in CI for cgroups v1 tests, so it's possible those tests were skipped elsewhere / in the old location (haven't checked 😅) |
|
I still want to try myself at replicating the issue, I have x86 hardware lying around somewhere, if I'm not able to replicate it should I just skip Cgroups V1? |
Is this failure specific to running in rootless mode with cgroups v1? After some digging, this combination seems fundamentally broken. Rootless Docker wants me to use cgroups v2, running |
Yes, it should be skipped |
deahtstroke
force-pushed
the
50159-migrate-testAPINoStreamGetCPU
branch
from
088d262 to
205b843
Compare
github-actions
Bot
added
area/builder
deahtstroke
force-pushed
the
50159-migrate-testAPINoStreamGetCPU
branch
from
55b8cac to
81c6337
Compare
|
Hi, its me again with a rebase issue. Labeller picked up on it and punished me. I fixed the PR itself but the labels are definitely wrong, should only have |
deahtstroke
force-pushed
the
50159-migrate-testAPINoStreamGetCPU
branch
from
81c6337 to
84a058e
Compare
|
I removed the polling in the new test since the polling was added for Cgroups V1 + Rootless, now we're just skipping that. |
…uite Signed-off-by: Daniel Villavicencio <[email protected]>
deahtstroke
force-pushed
the
50159-migrate-testAPINoStreamGetCPU
branch
from
84a058e to
404d22c
Compare
|
A lot of failing CI workflows, the |
3 participants
- What I did
Migrated the test
TestAPIStatsNoStreamGetCPUfrom theintegration-clitointegrationtest suite. Additionally removed the Cgroups v2 skip since we can calculate CPU usage without relying on missing fields from Cgroupvs2, namelyCPUStats.CPUUsage.PercpuUsage- How I did it
Rewriting the test using the Go-client and moving it to the
integration/module- How to verify it
Running the following command:
TESTDIRS='./integration/container/' TESTFLAGS='-test.run TestStatsNoStreamGetCPU' make test-integration- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
