Skip to content
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

UDN: Patch Kubevirt CR to support managedTap binding #4773

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

oshoval
Copy link
Contributor

@oshoval oshoval commented Oct 13, 2024

What this PR does and why is it needed

Use managedTap instead passt for UDN

Which issue(s) this PR fixes

Fixes #

Special notes for reviewers

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

None

@github-actions github-actions bot added area/e2e-testing feature/kubevirt-live-migration All issues related to kubevirt live migration labels Oct 14, 2024
Copy link
Contributor

@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.

you only need one in upstream OVN-K, right ?

@oshoval
Copy link
Contributor Author

oshoval commented Oct 14, 2024

True, i just wanted to do it smoother, i.e that as long as IPAM repo is not updated, it wont break it
(we can make the change there first if you prefer and drop in this PR)

@maiqueb
Copy link
Contributor

maiqueb commented Oct 14, 2024

True, i just wanted to do it smoother, i.e that as long as IPAM repo is not updated, it wont break it (we can make the change there first if you prefer and drop in this PR)

Whatever is both conceptually correct and simpler for you.

@oshoval
Copy link
Contributor Author

oshoval commented Oct 14, 2024

Removed the passt here - going to test it on the dummy PR oshoval#3

also added the PR for IPAM that works on CI as well kubevirt/ipam-extensions#73

@oshoval oshoval changed the title UDN: Patch Kubevirt CR to support managedTap binding WIP UDN: Patch Kubevirt CR to support managedTap binding Oct 27, 2024
@oshoval
Copy link
Contributor Author

oshoval commented Oct 27, 2024

injected to use nightly
lets see all pass (and then we can remove it)

@oshoval
Copy link
Contributor Author

oshoval commented Oct 27, 2024

either i did something wrong or the managedTap is broken on nightly kubevirt
will check (worked 2 weeks ago, before PR were merged, while compiled locally)
https://github.com/ovn-org/ovn-kubernetes/actions/runs/11540164957/job/32121009180?pr=4773

@oshoval
Copy link
Contributor Author

oshoval commented Oct 27, 2024

added new required FG, lets see it works

@oshoval
Copy link
Contributor Author

oshoval commented Oct 27, 2024

Hi Miguel, please disregard the re-review request, we need anyhow first to discuss offline about generic name and such

@@ -328,6 +328,8 @@ install_kubevirt() {
# nightly tag - install specific nightly (i.e 20240910)
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"}

KUBEVIRT_VERSION=nightly
Copy link
Contributor Author

@oshoval oshoval Oct 28, 2024

Choose a reason for hiding this comment

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

well we would need to consume nightly or to release kubevirt or to pin it to one that has all the required code
but for sure can make it on its own commit nicely

@oshoval
Copy link
Contributor Author

oshoval commented Oct 28, 2024

failures are not related, the UDN ones passed

@oshoval oshoval changed the title WIP UDN: Patch Kubevirt CR to support managedTap binding UDN: Patch Kubevirt CR to support managedTap binding Oct 29, 2024
@oshoval oshoval marked this pull request as ready for review October 29, 2024 10:19
@oshoval oshoval requested a review from a team as a code owner October 29, 2024 10:19
Copy link
Contributor

@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.

Some questions.

contrib/kind-common Show resolved Hide resolved
test/e2e/kubevirt.go Show resolved Hide resolved
@oshoval
Copy link
Contributor Author

oshoval commented Oct 29, 2024

Updated as we spoke offline
CR support both
e2e tests here runs mangedTap

Note please the 3rd commit, if we dont want nightly because it might break OVN if kubevirt breaks,
we can pin specific nightly meanwhile 20241029 or ask for a stable but it might take time and we would be block until that if we do.

Copy link
Contributor

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

Looks good as far as CI is happy.

@@ -326,7 +326,7 @@ install_kubevirt() {
# vX.Y.Z - install specific stable (i.e v1.3.1)
# nightly - install newest nightly
# nightly tag - install specific nightly (i.e 20240910)
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"}
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"nightly"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wait for a kubevirt release ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will ask if a special one can be created but i doubt as the minor should change maybe,
will also ask when the stable is out, it might take time

imho we can use 20241029, it is safer, see please
#4773 (comment)
we will just need to update once there is a stable and wont be blocked
but your call

Copy link
Contributor Author

@oshoval oshoval Oct 29, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

disregard, stable.txt wont have it, checking about alternatives (i.e new file on that path etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

To make this clear: this is the only thing holding this PR. I.e. we want to pin a 1.4 RC version.

Right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

test/e2e/kubevirt.go Show resolved Hide resolved
Copy link
Contributor

@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.

Let's pin a KubeVirt 1.4 RC version (which is not available yet ... should be imminent).

@@ -326,7 +326,7 @@ install_kubevirt() {
# vX.Y.Z - install specific stable (i.e v1.3.1)
# nightly - install newest nightly
# nightly tag - install specific nightly (i.e 20240910)
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"}
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"nightly"}
Copy link
Contributor

Choose a reason for hiding this comment

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

To make this clear: this is the only thing holding this PR. I.e. we want to pin a 1.4 RC version.

Right ?

contrib/kind-common Show resolved Hide resolved
test/e2e/kubevirt.go Show resolved Hide resolved
@oshoval
Copy link
Contributor Author

oshoval commented Nov 3, 2024

Updated to use v1.4.0-rc.0

@oshoval oshoval requested a review from maiqueb November 3, 2024 14:23
@oshoval
Copy link
Contributor Author

oshoval commented Nov 3, 2024

rebased

Copy link
Contributor

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

/lgtm

maiqueb
maiqueb previously approved these changes Nov 4, 2024
Copy link
Contributor

@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.

Tks.

@tssurya could you take a look ? We are registering the new binding (that allows live migration) in KubeVirt.

We are keeping the other around because we have people working on improving it (the long term plan is to use it).

@@ -326,7 +326,7 @@ install_kubevirt() {
# vX.Y.Z - install specific stable (i.e v1.3.1)
# nightly - install newest nightly
# nightly tag - install specific nightly (i.e 20240910)
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"}
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"v1.4.0-rc.0"}
Copy link
Contributor Author

@oshoval oshoval Nov 13, 2024

Choose a reason for hiding this comment

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

1.4.0 is out, so i will first change to use stable as always
EDIT - done

Since hostname is populated now via DHCP, we can always use
the vmi name in the console expecter.

Signed-off-by: Or Shoval <[email protected]>
@oshoval
Copy link
Contributor Author

oshoval commented Nov 13, 2024

changes
stick to use stable instead #4773 (comment)
because it was released and has the required code

rebased

@oshoval oshoval requested review from qinqon and maiqueb November 13, 2024 14:51
@trozet trozet merged commit 7b50659 into ovn-kubernetes:master Nov 14, 2024
37 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/e2e-testing feature/kubevirt-live-migration All issues related to kubevirt live migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants