-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 KVM not detecting containerd preload #17658
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spowelljr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
if err != nil { | ||
var rr *command.RunResult | ||
|
||
imageList := func() (err error) { |
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.
hm... I am trying to think, is there a better condition to wait for instead of retrying?
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.
EDIT: we probably don't have to wait/retry - pls see the comment below and the followup discussion
kvm2 driver with docker runtime
Times for minikube start: 49.2s 51.3s 48.2s 50.7s 52.6s Times for minikube (PR 17658) ingress: 27.2s 23.7s 27.1s 27.6s 27.2s docker driver with docker runtime
Times for minikube start: 20.9s 23.4s 24.4s 24.0s 25.0s Times for minikube ingress: 20.8s 20.3s 20.8s 20.8s 18.3s docker driver with containerd runtime
Times for minikube start: 23.4s 21.0s 23.1s 23.1s 23.4s Times for minikube ingress: 31.3s 31.3s 47.3s 31.3s 31.3s |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
hey @spowelljr i think the actual problem is in (how we handle/expect) the from logs:
so, the above underlying error:
comes from this line, and that effectively come from this block looking at the crt output, we have:
which has image to prove the point,
i've tested the following:
and it worked:
before:
after:
i'm happy to make a separate pr to fully test this, if it would help |
@prezha I wonder if you're running into another issue, if I output the error in the file
I believe what's happening is that the |
@spowelljr i think you're right: we probably have two separate issues here - the one that you observed happens before this other code that i referred to is hit here is the pr to address the latter in the comment above: #17671 so it makes sense to wait for the containerd to come up after the restart, and if we wait here, then i think it's reasonable to retry |
Cause
We were doing a
sudo systemctl restart containerd
right before we checked if the preloaded images are present. It was common on KVM that containerd wouldn't finish restarting before runningsudo crictl images --output json
which would fail, and would returnfalse
for the images being preloaded, resulting in the images be redownloaded unnecessarily.Fix
Added a retry to the
sudo crictl images --output json
command so if containerd is still restarting we don't report that the preloaded images aren't present.Result
First time start
Before: 59s
After: 38.6s
20.4s speedup (35%)
Normal start
Before: 48.7s
After: 30.5s
18.2s speedup (37%)
Also fixes the
TestErrorSpam/setup
test failure on KVM containerd due to the❌ Unable to load cached images
that was being output before.Before:
After: