-
Notifications
You must be signed in to change notification settings - Fork 185
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
integration: Wait until test-pod was OOMKilled for oomkill related tests #2649
integration: Wait until test-pod was OOMKilled for oomkill related tests #2649
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested but the code looks good to me.
Feel free to merge if all the tests are green.
cca61a9
to
5bff79d
Compare
if *k8sDistro == K8sDistroAKSUbuntu { | ||
waitCommand = WaitUntilTestPodOOMKilledCommand(ns) | ||
} else { | ||
waitCommand = WaitUntilTestPodReadyCommand(ns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the difference of behaviour really about the distribution?
If the container is slow to get oomkilled, we should be able to detect the pod be condition=ready
and then oomkilled. If the container gets oomkilled fast, the pod status might go to oomkilled fast enough so we don't see the condition=ready
.
It seems to me we should wait for the condition "ready or oomkilled" to avoid the race condition, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the difference of behaviour really about the distribution?
So far, the tests are failing on AKS Ubuntu.
If the container is slow to get oomkilled, we should be able to detect the pod be
condition=ready
and then oomkilled. If the container gets oomkilled fast, the pod status might go to oomkilled fast enough so we don't see thecondition=ready
.It seems to me we should wait for the condition "ready or oomkilled" to avoid the race condition, if possible.
I am still understanding how to fix everything here.
Particularly, I am not even sure we would get event generated in this case (i.e. OOMKilled
on AKS Ubuntu), so I may just end up with skipping the test here ¯\(ツ)/¯.
More specifically, the pod never reaches this state on other distribution, because it is only tail
which is killed and not the pod itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so with using sleep
I was able to have the CI passed at one time:
https://github.com/inspektor-gadget/inspektor-gadget/actions/runs/8452074988
I will try with the approach to wait for ready or OOM killed, but I do not like the idea to have to run the CI up to 6 times to have it pass.
…l tests. Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
5bff79d
to
3ff5a59
Compare
080170c
to
3ff5a59
Compare
I will merge it as is. |
Thank you for the review! |
No description provided.