[release-1.1] virt-handler: wait up to 2 seconds for the network-status downwardAPI to appear #10646
Conversation
kubevirt-bot
added
release-note
pkg/virt-handler/migration.go
Outdated
| for i := 0; i < 4; i++ { | ||
| dat, err = os.ReadFile(networkStatusPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read network status from downwards API") | ||
| } | ||
| if len(dat) != 0 { | ||
| break | ||
| } | ||
| time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
From your experience, are 1.5 seconds enough? Thanks
There was a problem hiding this comment.
Yes. 90% of the time, the file is there right away, and if not I've never seen it appear later than after the first sleep.
What I really want is to limit the delay on nodes that don't have the annotation, and therefore a file that will stay empty.
However, looking at this code again, there's a useless sleep on last iteration that I need to remove.
/hold
There was a problem hiding this comment.
Done, also added an extra loop to be extra cautious and match the PR/commit title. PTAL!
/hold cancel
kubevirt-bot
added
the
do-not-merge/hold
… to appear This is a band-aid to work around the downwardAPI slowness seen in k8s 1.27+. Future versions of the project will have a proper fix for this. Signed-off-by: Jed Lejosne <jed@redhat.com>
jean-edouard
force-pushed
the
release-1.1-givetimetodapi
branch
from
a9d3a8c
to
c753f79
Compare
kubevirt-bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xpivarc 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 |
kubevirt-bot
added
the
approved
|
/retest-required |
|
@jean-edouard: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest-required |
What this PR does / why we need it:
The network-status downward API file can take some time to get populated, and since virt-handler reads it just once on startup, the dedicated migration network can get ignored.
This is only a problem in k8s 1.27+.
This PR is a workaround, for 1.1 only (unless we decide to backport it). We are working on a proper fix in main, but it is taking longer than expected due to a potential bug in virt-operator.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes the "falky" dedicated migration test (the code is flaky, not the test)
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: