UDN: Patch Kubevirt CR to support managedTap binding #4773

oshoval · 2024-10-13T09:32:27Z

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

maiqueb

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

oshoval · 2024-10-14T09:39:12Z

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 · 2024-10-14T09:50:40Z

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 · 2024-10-14T13:19:06Z

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 · 2024-10-27T11:40:10Z

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

oshoval · 2024-10-27T13:40:27Z

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 · 2024-10-27T14:10:12Z

added new required FG, lets see it works

oshoval · 2024-10-27T15:15:10Z

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

oshoval · 2024-10-28T10:49:40Z

contrib/kind-common

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

+    KUBEVIRT_VERSION=nightly


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 · 2024-10-28T10:56:42Z

failures are not related, the UDN ones passed

maiqueb

Some questions.

maiqueb · 2024-10-29T10:21:30Z

contrib/kind-common

-
-    local kubevirt_stable_release_url=$(get_kubevirt_release_url "stable")
-    local passt_binding_image="quay.io/kubevirt/network-passt-binding:${kubevirt_stable_release_url##*/}"
-    kubectl -n kubevirt patch kubevirt kubevirt --type=json --patch '[{"op":"add","path":"/spec/configuration/network","value":{}},{"op":"add","path":"/spec/configuration/network/binding","value":{"passt":{"computeResourceOverhead":{"requests":{"memory":"500Mi"}},"migration":{"method":"link-refresh"},"networkAttachmentDefinition":"default/primary-udn-kubevirt-binding","sidecarImage":"'"${passt_binding_image}"'"}}}]'


why don't we want both of them ? and let the user pick which binding to use ?

just because you asked so i dropped
i can return it if desired as we spoke

maiqueb · 2024-10-29T10:22:00Z

test/e2e/kubevirt.go

-				} else {
-					return kubevirt.LoginToFedoraWithHostname(vmi, "core", "fedora", "localhost")
-				}


why did this binding type changed the prompt ?

since with the new one the DHCP populate now the right hostname, no need this special treatment that was to workaround this when passt is used (when the hostname is not set, prompt has localhost instead vm name, this what the code that was removed done, and not needed now)
Quique is familiar with it best, and said it is good btw

to me, it seems like we first need to decide if we want to keep both bindings around or not.

like, if we keep passt around, we can't have this change.

those are the e2e tests, in tests as long as we dont test both passt and managedTap we can have just one here,
(otherwise there should be another condition here based on the binding, but i dont think we want to test both upstream, nor to just have support for one that isnt tested by a new condition on the used binding)
it is orthogonal to have both on the CR and then the user can still choose, this can be done anyhow

just to add some context, passt do not support the DHCPv4 option hostname (12), so VM ends up with localhost, also is good that we test this kind of things since at managedTap we pass that with the OVN DHCP.

Signed-off-by: Or Shoval <oshoval@redhat.com>

Since hostname is populated now via DHCP, we can always use the vmi name in the console expecter. Signed-off-by: Or Shoval <oshoval@redhat.com>

Until we have a new stable release, this will allow us to have the new required managedTap and dynamic interface name detection that were presented by kubevirt. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval · 2024-10-29T11:40:44Z

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.

qinqon

Looks good as far as CI is happy.

qinqon · 2024-10-29T11:42:14Z

contrib/kind-common

@@ -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"}


Should we wait for a kubevirt release ?

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

https://github.com/kubevirt/sig-release/blob/main/releases/v1.4/schedule.md
lets wait, rc should be any day
iiuc it will appear on https://storage.googleapis.com/kubevirt-prow/release/kubevirt/kubevirt/stable.txt that we use by default

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

qinqon · 2024-10-29T11:44:21Z

test/e2e/kubevirt.go

-				} else {
-					return kubevirt.LoginToFedoraWithHostname(vmi, "core", "fedora", "localhost")
-				}


just to add some context, passt do not support the DHCPv4 option hostname (12), so VM ends up with localhost, also is good that we test this kind of things since at managedTap we pass that with the OVN DHCP.

oshoval mentioned this pull request Oct 13, 2024

DMM Test TAP oshoval/ovn-kubernetes#3

Closed

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

maiqueb requested changes Oct 14, 2024

View reviewed changes

oshoval force-pushed the mantap branch from 9796e8b to a130554 Compare October 14, 2024 13:18

oshoval force-pushed the mantap branch from a130554 to 2a7c604 Compare October 27, 2024 11:39

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 force-pushed the mantap branch from 2a7c604 to 8ae2809 Compare October 27, 2024 14:03

oshoval mentioned this pull request Oct 27, 2024

WIP: Use managedTap instead passt kubevirt/ipam-extensions#73

Draft

oshoval requested a review from maiqueb October 27, 2024 15:13

oshoval commented Oct 28, 2024

View reviewed changes

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 marked this pull request as ready for review October 29, 2024 10:19

oshoval requested a review from a team as a code owner October 29, 2024 10:19

maiqueb reviewed Oct 29, 2024

View reviewed changes

oshoval added 3 commits October 29, 2024 13:38

UDN: Patch Kubevirt CR to support managedTap binding

5b2d961

Signed-off-by: Or Shoval <oshoval@redhat.com>

UDN: Adapt tests to use managedTap instead passt

9366134

Since hostname is populated now via DHCP, we can always use the vmi name in the console expecter. Signed-off-by: Or Shoval <oshoval@redhat.com>

kubevirt: Use nightly version

0920420

Until we have a new stable release, this will allow us to have the new required managedTap and dynamic interface name detection that were presented by kubevirt. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval force-pushed the mantap branch from 8ae2809 to 0920420 Compare October 29, 2024 11:38

qinqon approved these changes Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UDN: Patch Kubevirt CR to support managedTap binding #4773

UDN: Patch Kubevirt CR to support managedTap binding #4773

oshoval commented Oct 13, 2024 •

edited

Loading

maiqueb left a comment

oshoval commented Oct 14, 2024 •

edited

Loading

maiqueb commented Oct 14, 2024

oshoval commented Oct 14, 2024 •

edited

Loading

oshoval commented Oct 27, 2024

oshoval commented Oct 27, 2024 •

edited

Loading

oshoval commented Oct 27, 2024

oshoval commented Oct 27, 2024

oshoval Oct 28, 2024 •

edited

Loading

oshoval commented Oct 28, 2024

maiqueb left a comment

maiqueb Oct 29, 2024

oshoval Oct 29, 2024

maiqueb Oct 29, 2024

oshoval Oct 29, 2024 •

edited

Loading

maiqueb Oct 29, 2024

oshoval Oct 29, 2024 •

edited

Loading

qinqon Oct 29, 2024

oshoval commented Oct 29, 2024

qinqon left a comment

qinqon Oct 29, 2024

oshoval Oct 29, 2024

oshoval Oct 29, 2024 •

edited

Loading

oshoval Oct 29, 2024

qinqon Oct 29, 2024

UDN: Patch Kubevirt CR to support managedTap binding #4773

Are you sure you want to change the base?

UDN: Patch Kubevirt CR to support managedTap binding #4773

Conversation

oshoval commented Oct 13, 2024 • edited Loading

What this PR does and why is it needed

Which issue(s) this PR 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?

maiqueb left a comment

Choose a reason for hiding this comment

oshoval commented Oct 14, 2024 • edited Loading

maiqueb commented Oct 14, 2024

oshoval commented Oct 14, 2024 • edited Loading

oshoval commented Oct 27, 2024

oshoval commented Oct 27, 2024 • edited Loading

oshoval commented Oct 27, 2024

oshoval commented Oct 27, 2024

oshoval Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval commented Oct 28, 2024

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval commented Oct 29, 2024

qinqon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval commented Oct 13, 2024 •

edited

Loading

oshoval commented Oct 14, 2024 •

edited

Loading

oshoval commented Oct 14, 2024 •

edited

Loading

oshoval commented Oct 27, 2024 •

edited

Loading

oshoval Oct 28, 2024 •

edited

Loading

oshoval Oct 29, 2024 •

edited

Loading

oshoval Oct 29, 2024 •

edited

Loading

oshoval Oct 29, 2024 •

edited

Loading