Skip to content

e2e: use Eventually for IP assertions to avoid race condition #129

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.

Sign up for GitHub

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

Jump to bottom
Merged
kubevirt-bot merged 1 commit into from
Nov 13, 2025
Merged

e2e: use Eventually for IP assertions to avoid race condition #129

kubevirt-bot merged 1 commit into from
Nov 13, 2025

Conversation

@RamLavi
Copy link
Contributor

@RamLavi RamLavi commented Oct 30, 2025

What this PR does / why we need it:
In the e2e tests there is a hidden assumption that once the VMI is in Ready=true condition then the status.interface.ipAddress is set. This is not guaranteed, causing a flake.
This PR adds Eventually with 30s timeout to wait for IPs to appear.

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 #

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Oct 30, 2025
@RamLavi RamLavi force-pushed the fix_vmi_ready_status_ip_set_race branch from 50c3913 to d23b2b3 Compare October 30, 2025 10:59
maiqueb
maiqueb reviewed Oct 30, 2025
View reviewed changes
Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

just a thought

@RamLavi RamLavi force-pushed the fix_vmi_ready_status_ip_set_race branch from d23b2b3 to 043b831 Compare October 30, 2025 14:02
@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 10, 2025

@maiqueb until this is fixed, the test will keep flaking on CNAO ipam-ext tier1 test (example) - making me investigate it every time. Let's free ourselves from this burden.
How do you want to proceed with this PR?

@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 11, 2025

Seen again here

@maiqueb
Copy link
Collaborator

maiqueb commented Nov 12, 2025

@maiqueb until this is fixed, the test will keep flaking on CNAO ipam-ext tier1 test (example) - making me investigate it every time. Let's free ourselves from this burden. How do you want to proceed with this PR?

I worry that patching the immediate problem with this fix is just introducing tech debt that I worry will never be tackled.

So yes, you will no longer be nagged. But the test just got harder to follow and understand.

I would prefer if we did this properly.

@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 12, 2025

@maiqueb until this is fixed, the test will keep flaking on CNAO ipam-ext tier1 test (example) - making me investigate it every time. Let's free ourselves from this burden. How do you want to proceed with this PR?

I worry that patching the immediate problem with this fix is just introducing tech debt that I worry will never be tackled.

So yes, you will no longer be nagged. But the test just got harder to follow and understand.

I would prefer if we did this properly.

Can you elaborate? is using eventually in order for the status to eventually consist the data we want - not the proper solution?

@maiqueb
Copy link
Collaborator

maiqueb commented Nov 12, 2025 •
edited
Loading

@maiqueb until this is fixed, the test will keep flaking on CNAO ipam-ext tier1 test (example) - making me investigate it every time. Let's free ourselves from this burden. How do you want to proceed with this PR?

I worry that patching the immediate problem with this fix is just introducing tech debt that I worry will never be tackled.
So yes, you will no longer be nagged. But the test just got harder to follow and understand.
I would prefer if we did this properly.

Can you elaborate? is using eventually in order for the status to eventually consist the data we want - not the proper solution?

I'm speaking about this, which you actually +1ed.

@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 12, 2025 •
edited
Loading

@maiqueb until this is fixed, the test will keep flaking on CNAO ipam-ext tier1 test (example) - making me investigate it every time. Let's free ourselves from this burden. How do you want to proceed with this PR?

I worry that patching the immediate problem with this fix is just introducing tech debt that I worry will never be tackled.
So yes, you will no longer be nagged. But the test just got harder to follow and understand.
I would prefer if we did this properly.

Can you elaborate? is using eventually in order for the status to eventually consist the data we want - not the proper solution?

I'm speaking about this, which you actually +1ed.

ah, np! Why didn't you say so earlier :)
(I mean, you didn't NACK my suggestion to do it in a follow up, so I didn't know this is what's holding it)

@RamLavi
test: Use BeReadyWithIPsAtInterface in persistent IPs tests
f6c86f7 
Replace separate VMI readiness and IP assignment checks with
the composite BeReadyWithIPsAtInterface matcher.

This makes the test intent clearer and checks both conditions
atomically.
Also eliminates race condition, as VMI Ready state doesn't guarantee IPs
are populated in status.interfaces.

Assisted-by: Claude <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>
@RamLavi RamLavi force-pushed the fix_vmi_ready_status_ip_set_race branch from 043b831 to f6c86f7 Compare November 13, 2025 12:09
maiqueb
maiqueb approved these changes Nov 13, 2025
View reviewed changes
Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Nice !

I really favor this approach to the previous one; thanks for complying.

/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2025
@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Nov 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maiqueb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2025
@maiqueb
Copy link
Collaborator

maiqueb commented Nov 13, 2025

Consider writing release notes for this bug. @RamLavi

@kubevirt-bot kubevirt-bot merged commit 65bf911 into kubevirt:main Nov 13, 2025
4 checks passed
@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 13, 2025

Consider writing release notes for this bug. @RamLavi

There is no bug. We just assumed that vm ready ==> vm. Status.ips is there. That is simply not the case.

@RamLavi
Copy link
Contributor Author

RamLavi commented Nov 13, 2025

@maiqueb can you issue a release on main branch so that we could consume the tests on cnao main branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maiqueb maiqueb maiqueb approved these changes

@oshoval oshoval Awaiting requested review from oshoval

Assignees

@maiqueb maiqueb

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RamLavi @maiqueb @kubevirt-bot