UDN: Use managedTap instead passt #73

oshoval · 2024-10-14T10:15:20Z

What this PR does / why we need it:
Use managedTap binding instead Passt for primary UDN.
It is registered as part of OVN kind.sh that we use in this repo to spin the cluster.

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:
Currently only IPv4 is supported.
A follow-up will add IPv6 support, by adding it to cloud-init, and deploying a dual stack cluster.
Once OVN use l2bridge naming we will adapt as well.

Release note:

None

oshoval · 2024-11-17T07:29:09Z

Hi @qinqon @maiqueb
once we want, we can switch to managedTap here (and then on CNAO when it tests it upstream as well)
almost just tests are changed on CNAO / here
(CNAO will also register only on upstream the binding / FGs via its upstream deployment scrips)

RamLavi

/lgtm

oshoval · 2024-11-26T08:07:33Z

@maiqueb @qinqon
Can you please add it to your review queue ?
Thanks

maiqueb

How can this work ?

Where do we register the bindings ?

Why are you dropping cloud init ?

oshoval · 2024-11-26T08:41:58Z

we register the binding on OVN k8s repo, that we use in this repo
we drop the cloud-init because as you also raised in the past (on slack, when you reviewed this gist [1]), this is not needed, it already happens automatically

[1] https://gist.github.com/RamLavi/91a25858a56f87e47039ceef99df662b

maiqueb · 2024-11-26T08:56:01Z

we register the binding on OVN k8s repo, that we use in this repo we drop the cloud-init because as you also raised in the past (on slack, when you reviewed this gist [1]), this is not needed, it already happens automatically

Please add this info to the commit message.

[1] https://gist.github.com/RamLavi/91a25858a56f87e47039ceef99df662b

qinqon · 2024-11-26T10:22:09Z

test/e2e/persistentips_test.go

-version: 2
-ethernets:
-  eth0:
-    dhcp4: true`
-	)


for managedTap we need to activate dhcpv4 and dhcpv6.

aint kubevirt assign ipv4 by default ?
VM does get ipv4, and this what i remember for the past, wdyt ?

ipv6 we don't test on git actions, (also on OVN CI we dont have the cluster with ipv6, because git actions doesnt support it), do we still want to add it ?
EDIT - hmm this one does have .github/workflows/test.yml: KIND_IPV6_SUPPORT: "${{ matrix.ipfamily == 'IPv6' || matrix.ipfamily == 'dualstack' }}"

wait. This depends on the cluster's configuration.
We need to introspect the cluster, figure out which IP families are enabled, and configure this accordingly.

OR we can hard-code it to support whatever it is we're deploying by default in the e2e tests, but make a real good note about it, then open an issue to introspect in a follow-up PR.

aint kubevirt assign ipv4 by default ? VM does get ipv4, and this what i remember for the past, wdyt ?

ipv6 we don't test on git actions, (also on OVN CI we dont have the cluster with ipv6, because git actions doesnt support it), do we still want to add it ? EDIT - hmm this one does have .github/workflows/test.yml: KIND_IPV6_SUPPORT: "${{ matrix.ipfamily == 'IPv6' || matrix.ipfamily == 'dualstack' }}"

That's not true, we run dual stack at ovn-k, and is ovn-k the one handling IPs.

best imo the 2nd option, have it on a follow-up
will allow me to concentrate on the other tasks first as well

wait. This depends on the cluster's configuration. We need to introspect the cluster, figure out which IP families are enabled, and configure this accordingly.

OR we can hard-code it to support whatever it is we're deploying by default in the e2e tests, but make a real good note about it, then open an issue to introspect in a follow-up PR.

We just acivate dhcp for both and that's all, if cluster is just ipv4 we will not have an ipv6 address.

i dont think we should add code that might not have an effect, better to have a task to investigate as Miguel said, what cluster we run here and only then add whatever config that is needed

btw does ipv6 is tested on OVN repo ? also something that worth checking
Once we have agreement on the direction i will do that, personally i prefer the one that doesnt have unused code and to open issue that is focused and changed whatever needed on all repos.
We can first finish the IPv4 flow on all the repos and then add the IPv6 tests according needs (and according what cluster we have).
Atm we are running IPv4 cluster on this repo.

btw does ipv6 is tested on OVN repo ?

It surely is in some lanes. It's a configuration knob.

Atm we are running IPv4 cluster on this repo.

So we'll need to change that. That means we're testing half the feature.

Sure, for now, you can move forward with ipv4 (since it appears it's the only thing we support).

But ensure you open a ticket to add IPv6 support to this repo.

It surely is in some lanes. It's a configuration knob.

ye i meant if it is tested for managedTap itself on OVN

np will open Jira ticket (done)
thanks

qinqon · 2024-11-26T10:22:29Z

test/e2e/persistentips_test.go

@@ -402,6 +395,5 @@ ethernets:
 				Pod: &kubevirtv1.PodNetwork{},
 			},
 		}),
-		testenv.WithCloudInitNoCloudVolume(cloudInitNetworkData),


we still need cloudInit for managedTap.

Can you please see #73 (comment) ?
according Miguel and from what i know we don't need it when using just IPv4

if you'll need to template it according to the ip families, it'll probably be better to always include it.

It's also more explicit.

But I don't care much; at least for fedora, I know it'll default to ipv4 using dhcp.

Your call.

ok will return it, thx

oshoval · 2024-11-28T07:11:55Z

once we merge the new binding name on HCO, need to adapt it on OVN and here
(not because HCO affect this repos, but to be aligned of course)

oshoval · 2024-12-02T13:45:04Z

we need ovn-kubernetes/ovn-kubernetes#4854
and then to adapt this PR to use the generic name first imo

oshoval · 2024-12-02T13:46:59Z

Updated the PR description (will update commit message as well once we converge)
I can return the cloud-init if desired, but it is not mandatory, the virt-launcher DHCP server already does it for IPv4,
maybe just once we have IPv6 we will need both ?

oshoval · 2024-12-16T11:15:17Z

from quick check, all is done now
ptal

maiqueb · 2024-12-16T11:17:24Z

/lgtm

oshoval · 2024-12-16T11:18:10Z

@RamLavi
would you able please to add the missing binding for CNAO as part of your ticket?
(update kubevirt CR with kubevirt FG and the binding registration as OVN / HCO does)
required since CNAO doesnt use -ikv that install kubevirt and does that by kind.sh of OVN
will affect once ipam-ext is released and bumped

EDIT
kubevirt/cluster-network-addons-operator#1919
this might help, forgot i opened it

oshoval · 2024-12-16T11:22:18Z

/hold

it will fail
we need ovn-kubernetes/ovn-kubernetes#4854

or to revert here back to the name managedTap
whatever you prefer

note that HCO uses l2bridge (it is just the custom name of course, behind the scene both are internally managedTap)
so best to do it right and one time

qinqon · 2024-12-16T11:45:32Z

/hold

it will fail we need ovn-kubernetes/ovn-kubernetes#4854

or to revert here back to the name managedTap whatever you prefer

note that HCO uses l2bridge (it is just the custom name of course, behind the scene both are internally managedTap) so best to do it right and one time

we patch replace managedTap after clonning ovn-kubernetes, is dirty but that will unblock this.

oshoval · 2024-12-16T12:00:57Z

/hold
it will fail we need ovn-kubernetes/ovn-kubernetes#4854
or to revert here back to the name managedTap whatever you prefer
note that HCO uses l2bridge (it is just the custom name of course, behind the scene both are internally managedTap) so best to do it right and one time

we patch replace managedTap after clonning ovn-kubernetes, is dirty but that will unblock this.

i dont like it, it will require fix later anyhow,
why not to just either wait for 4854 to be merged, or to use here managedTap which is already registered
and once 4854 is merged to adapt, easy to adapt

oshoval · 2024-12-16T13:06:21Z

just rebased

oshoval · 2024-12-16T13:23:51Z

just rebased

Signed-off-by: Or Shoval <[email protected]>

oshoval · 2024-12-16T13:26:04Z

updated name to managedTap until OVN adapt it to l2bridge
as discussed offline

/hold cancel

maiqueb · 2024-12-16T14:00:49Z

Let's get this in today, and adapt once ovn-k deploys l2bridge via ovn-kubernetes/ovn-kubernetes#4854

/lgtm

@qinqon any objection ?

qinqon · 2024-12-16T14:50:48Z

/lgtm
/approve

kubevirt-bot · 2024-12-16T14:50:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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:

~~OWNERS~~ [qinqon]

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

oshoval · 2024-12-17T08:26:00Z

Thanks

@RamLavi
i will issue a release ok ?
and then CNAO can be adapted to use managedTap

maybe just bump + kubevirt/cluster-network-addons-operator#1919 is needed

oshoval · 2024-12-17T08:32:38Z

since a tag was already created yesterday without this PR, and this one is needed i created a new one
v0.1.9-alpha

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 14, 2024

kubevirt-bot requested review from maiqueb and qinqon October 14, 2024 10:15

kubevirt-bot added size/XS size/S and removed size/XS labels Oct 14, 2024

oshoval changed the title ~~WIP: Use managedTap~~ WIP: Use managedTap instead passt Oct 14, 2024

oshoval force-pushed the tap branch 2 times, most recently from 3b8c3e1 to 03d7144 Compare October 14, 2024 13:11

oshoval mentioned this pull request Oct 14, 2024

UDN: Patch Kubevirt CR to support managedTap binding ovn-kubernetes/ovn-kubernetes#4773

Merged

oshoval force-pushed the tap branch from 03d7144 to cf194ea Compare November 17, 2024 07:26

oshoval changed the title ~~WIP: Use managedTap instead passt~~ UDN: Use managedTap instead passt Nov 17, 2024

RamLavi mentioned this pull request Nov 21, 2024

e2e, persistent-ip: Unify primary,secondary tests (primary UDNs use managedTap) #75

Merged

oshoval marked this pull request as ready for review November 21, 2024 08:26

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2024

RamLavi approved these changes Nov 21, 2024

View reviewed changes

kubevirt-bot assigned RamLavi Nov 21, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024

maiqueb reviewed Nov 26, 2024

View reviewed changes

qinqon requested changes Nov 26, 2024

View reviewed changes

kubevirt-bot assigned qinqon Nov 26, 2024

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024

oshoval requested review from qinqon, maiqueb and RamLavi December 16, 2024 11:16

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024

oshoval force-pushed the tap branch from 9f36d75 to db96b4f Compare December 16, 2024 13:06

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

oshoval force-pushed the tap branch from db96b4f to 0728a08 Compare December 16, 2024 13:23

UDN: Use managedTap instead passt

64bea26

Signed-off-by: Or Shoval <[email protected]>

oshoval force-pushed the tap branch from 0728a08 to 64bea26 Compare December 16, 2024 13:24

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024

oshoval mentioned this pull request Dec 16, 2024

UDN: Support managedTap binding, bump kubevirt-ipam-controller kubevirt/cluster-network-addons-operator#1919

Closed

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2024

kubevirt-bot merged commit 5e1af16 into kubevirt:main Dec 16, 2024
4 checks passed

oshoval mentioned this pull request Dec 17, 2024

bump kubevirt-ipam-controller to v0.1.8-alpha kubevirt/cluster-network-addons-operator#1948

Closed

UDN: Use managedTap instead passt #73

UDN: Use managedTap instead passt #73

Conversation

oshoval commented Oct 14, 2024 • edited Loading

oshoval commented Nov 17, 2024 • edited Loading

RamLavi left a comment

Choose a reason for hiding this comment

oshoval commented Nov 26, 2024

maiqueb left a comment

Choose a reason for hiding this comment

oshoval commented Nov 26, 2024

maiqueb commented Nov 26, 2024

Choose a reason for hiding this comment

oshoval Nov 26, 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 Nov 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

oshoval Nov 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval commented Nov 28, 2024

oshoval commented Dec 2, 2024

oshoval commented Dec 2, 2024 • edited Loading

oshoval commented Dec 16, 2024

maiqueb commented Dec 16, 2024

oshoval commented Dec 16, 2024 • edited Loading

oshoval commented Dec 16, 2024 • edited Loading

qinqon commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval commented Dec 16, 2024

maiqueb commented Dec 16, 2024

qinqon commented Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

oshoval commented Dec 17, 2024 • edited Loading

oshoval commented Dec 17, 2024

oshoval commented Oct 14, 2024 •

edited

Loading

oshoval commented Nov 17, 2024 •

edited

Loading

oshoval Nov 26, 2024 •

edited

Loading

oshoval Nov 26, 2024 •

edited

Loading

oshoval Nov 28, 2024 •

edited

Loading

oshoval commented Dec 2, 2024 •

edited

Loading

oshoval commented Dec 16, 2024 •

edited

Loading

oshoval commented Dec 16, 2024 •

edited

Loading

oshoval commented Dec 17, 2024 •

edited

Loading