Register user-defined-network (UDN) binding on kubevirt CR #3034

RamLavi · 2024-07-31T08:18:57Z

What this PR does / why we need it:
This PR adds support to user-defined-network binding to the kubevirt CR.
This is part of the process described on kubevirt user guide.
The behavior added is currently dev-preview, and as such is protected behind a FG: PrimaryUserDefinedNetworkBinding, that is set to false by default.

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Jira Ticket:

Release note:

NONE

kubevirt-bot · 2024-07-31T08:19:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-07-31T08:19:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

nunnatsa

Thanks for the PR. Added some inline comments.

My main comment is about modifying the HC CR in kubevirt.go. We need to avoid that.

also, some TODOs, yes, I know it's WIP...

please update docs/cluster-configuration.md
please update the "feature gate defaults" test in tests/func-tests/defaults_test.go

nunnatsa · 2024-07-31T08:27:05Z

controllers/operands/kubevirt.go

+const (
+	// Needs to align with the NAD that will be deployed by CNAO
+	primaryUDNNetworkBindingName      = "primaryUserDefinedNetwork"
+	primaryUDNNetworkBindingNamespace = "default"


why in the default ns?

In order for the NetworkAttachmentDefinition to be available to all VMs on all namespaces, it needs to be deployed on default namespace.
For the dev-preview we think this is a good enough approach, but IMO in the future we will need to think of other ways.

Also note that HCO I changed the PR since when we talked and not it is not not deploying the NetworkAttachmentDefinition (will happen on CNAO)

nunnatsa · 2024-07-31T08:29:31Z

controllers/operands/kubevirt.go

+		if hc.Spec.NetworkBinding == nil {
+			hc.Spec.NetworkBinding = make(map[string]kubevirtcorev1.InterfaceBindingPlugin)
+		}
+
+		var sidecarImage string
+		var ok bool
+		if sidecarImage, ok = os.LookupEnv(hcoutil.PrimaryUDNImageEnvV); !ok {
+			return nil, errors.New("failed to get primary UDN image env Var")
+		}
+		hc.Spec.NetworkBinding[primaryUDNNetworkBindingName] = primaryUserDefinedNetworkBinding(sidecarImage)


Why do we need to modify the HyperConverged CR? we'll need really good reason for that. the spec is for the user.

We can't directly change the kubevirt CR. If we do, HCO will reconcile back to what's on hc.Spec.NetworkBinding..

HCO currently just copies the NetworkBinding map to KV. I think we can change this code with the new logic.

e.g., instead of

NetworkConfiguration: &kubevirtcorev1.NetworkConfiguration{ NetworkInterface: string(kubevirtcorev1.MasqueradeInterface), Binding: hc.Spec.NetworkBinding, },

do something like

NetworkConfiguration: &kubevirtcorev1.NetworkConfiguration{ NetworkInterface: string(kubevirtcorev1.MasqueradeInterface), Binding: getNetworkBinding(hc), },

and the new logic will get into the new function getNetworkBinding

nunnatsa · 2024-07-31T08:30:56Z

deploy/images.csv

@@ -34,3 +34,4 @@ KUBEVIRT_CONSOLE_PROXY_IMAGE,quay.io/kubevirt-ui/kubevirt-apiserver-proxy,KUBEVI
 AAQ_OPERATOR_IMAGE,quay.io/kubevirt/aaq-operator,AAQ_VERSION,002513b78dfd795f5195534b651eace136ead3d73c8ac8cae145607e79c8a8ba
 AAQ_SERVER_IMAGE,quay.io/kubevirt/aaq-server,AAQ_VERSION,9db6ca9147b1d75ba4d6bb1326c4ec289327eb9f8aadbcd951fcc689e584cd0d
 AAQ_CONTROLLER_IMAGE,quay.io/kubevirt/aaq-controller,AAQ_VERSION,170abd7a75397ddce07ba0d64886ead25183c49cacbccf0d191a4dd37ca013fd
+PRIMARY_USER_DEFINED_NETWORK_BINDING_IMAGE,quay.io/kubevirt/network-passt-binding,KUBEVIRT_VERSION,3fb5f9fb87a9ab1f06bed2b2358ad862ef66bc47dc73044f1076a3eb9a4c55ff


please move it up, to be grouped with all the lines that use the KUBEVIRT_VERSION variable.

nunnatsa · 2024-07-31T08:33:07Z

hack/build-manifests.sh

@@ -325,7 +326,8 @@ ${PROJECT_ROOT}/tools/manifest-templator/manifest-templator \
  --aaq-version="${AAQ_VERSION}" \
  --operator-image="${HCO_OPERATOR_IMAGE}" \
  --webhook-image="${HCO_WEBHOOK_IMAGE}" \
-  --cli-downloads-image="${HCO_DOWNLOADS_IMAGE}"
+  --cli-downloads-image="${HCO_DOWNLOADS_IMAGE}" \
+  --primary-udn-image="${}"


you need use the PRIMARY_USER_DEFINED_NETWORK_BINDING_IMAGE var here

Also, the parameter name is wrong: should be primary-udn-binding-image

oh damn, I though I got everything.
Thanks for the catch!

api/v1beta1/hyperconverged_types.go

RamLavi · 2024-07-31T18:31:25Z

Change: Added unit tests.

RamLavi · 2024-08-01T07:49:19Z

Change: Added kubebuilder default labels on HCO CR tree, Added documentation for new FG

RamLavi · 2024-08-01T07:56:47Z

Change: auto-generating manifests, rebase

nunnatsa · 2024-08-01T08:23:18Z

controllers/operands/kubevirt.go

+		},
+		ComputeResourceOverhead: &corev1.ResourceRequirements{
+			Requests: corev1.ResourceList{
+				corev1.ResourceMemory: resource.MustParse("500Mi"),


This magic number of 500Mi is not clear. Can we use a constant with meaningful name, that will self explain it?

nunnatsa · 2024-08-01T08:25:25Z

controllers/operands/kubevirt.go

+		var sidecarImage string
+		var ok bool
+		if sidecarImage, ok = os.LookupEnv(hcoutil.PrimaryUDNImageEnvV); !ok {
+			return nil, errors.New("failed to get primary UDN image env Var")


Not sure about this error. This is not something the user can fix. Maybe it will better to check it on boot time and exit, to fail the pod? @tiraboschi WDYT?

I agree: this is something we are configuring at the bundle build time as part of the opinionated deployment and it's not something the user can or should try to fix.

nunnatsa · 2024-08-01T08:28:49Z

docs/cluster-configuration.md

+### primaryUserDefinedNetworkBinding Feature Gate
+Set the `primaryUserDefinedNetworkBinding` feature gate to deploy the needed configurations for kubevirt users, so they 
+can bind their VM to a user-defined network (UDN) on the VM's primary interface.
+


do we have some kind of documentation we can add as a reference? I'm afraid this description is a bit crypt.

@maiqueb do we?

what documentation are you hoping for @nunnatsa ?

I'm afraid all we have is the openshift enhancement document.

maybe worth to add the env var that can be set in order to determine non default value
(this what we are doing for example on CNAO readme)
together with the current info, it can give a simple user guide (please consider adding few words that will make it user guide in case it miss)

also good to have please on PR desc the important details, such as the feature gate name and so on

@oshoval

maybe worth to add the env var that can be set in order to determine non default value (this what we are doing for example on CNAO readme) together with the current info, it can give a simple user guide (please consider adding few words that will make it user guide in case it miss)

Not sure what you mean here. what env var? This FG is set by HCO CR, and is set to false by default.. In order to activate it, no env var is required - only changing the FG value to true.

also good to have please on PR desc the important details, such as the feature gate name and so on

DONE

I'm afraid all we have is the openshift enhancement document.

@nunnatsa will that be sufficient?

Not sure what you mean here. what env var? This FG is set by HCO CR, and is set to false by default.. In order to activate it, no env var is required - only changing the FG value to true.

I meant the one that allows to override the image, but no biggie

ah.. that's not something a user should be exposed to imo.

RamLavi · 2024-08-01T09:57:09Z

Change: Add meaningful name const.

hco-bot · 2024-08-12T12:51:05Z

hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

kubevirt-bot · 2024-08-12T12:51:12Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

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-sigs/prow repository.

nunnatsa

/approve

kubevirt-bot · 2024-08-12T13:04:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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~~ [nunnatsa]

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

nunnatsa · 2024-08-12T13:04:33Z

/retest

openshift-ci · 2024-08-12T14:30:39Z

@RamLavi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure	`508d61b`	link	false	`/test hco-e2e-upgrade-operator-sdk-sno-azure`
ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure	`508d61b`	link	true	`/test hco-e2e-consecutive-operator-sdk-upgrades-azure`
ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure	`508d61b`	link	true	`/test hco-e2e-upgrade-prev-operator-sdk-azure`
ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws	`508d61b`	link	false	`/test hco-e2e-upgrade-prev-operator-sdk-sno-aws`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

hco-bot · 2024-08-12T15:29:31Z

hco-e2e-upgrade-prev-operator-sdk-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws

kubevirt-bot · 2024-08-12T15:29:35Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws

In response to this:

hco-e2e-upgrade-prev-operator-sdk-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws

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-sigs/prow repository.

This HCO feature gate was introduced in [0]. [0] - kubevirt/hyperconverged-cluster-operator#3034 Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

This HCO feature gate was introduced in [0]; it also requires the KubeVirt IPAM controller component to persist the IP addresses between VM migrations, and restarts. [0] - kubevirt/hyperconverged-cluster-operator#3034 Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

…ubmits (#57704) * ovn-kubernetes, virt: add new AWS virt lane to run OVN presubmits Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com> * udn, virt: enable primary UDN binding This HCO feature gate was introduced in [0]; it also requires the KubeVirt IPAM controller component to persist the IP addresses between VM migrations, and restarts. [0] - kubevirt/hyperconverged-cluster-operator#3034 Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com> * udn, virt, tests: run only virt-aware tests A future PR will add tests for this feature gate and feature in openshift/origin project. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com> * ovn-kubernetes, virt: add new AWS virt lane to run OVN 4.18/4.19 presubmits Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com> --------- Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

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 Jul 31, 2024

kubevirt-bot requested review from avlitman and tiraboschi July 31, 2024 08:19

kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L labels Jul 31, 2024

nunnatsa reviewed Jul 31, 2024

View reviewed changes

RamLavi force-pushed the register_udn branch from 1b989c4 to cf13462 Compare July 31, 2024 09:02

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2024

RamLavi force-pushed the register_udn branch 2 times, most recently from 7e5a784 to a34a83a Compare July 31, 2024 18:28

RamLavi changed the title ~~[WIP] Register user-defined-network (UDN) binding on kubevirt CR~~ Register user-defined-network (UDN) binding on kubevirt CR Jul 31, 2024

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 31, 2024

RamLavi force-pushed the register_udn branch from a34a83a to fb4397b Compare August 1, 2024 07:43

RamLavi force-pushed the register_udn branch from fb4397b to bb5b1b0 Compare August 1, 2024 07:56

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2024

RamLavi marked this pull request as ready for review August 1, 2024 07:56

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 Aug 1, 2024

kubevirt-bot requested a review from sradco August 1, 2024 07:56

nunnatsa reviewed Aug 1, 2024

View reviewed changes

RamLavi force-pushed the register_udn branch from bb5b1b0 to a541cb0 Compare August 1, 2024 09:56

nunnatsa approved these changes Aug 12, 2024

View reviewed changes

kubevirt-bot assigned nunnatsa Aug 12, 2024

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

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

kubevirt-bot merged commit fc1c591 into kubevirt:main Aug 12, 2024
31 checks passed

RamLavi mentioned this pull request Dec 17, 2024

Remove passt network binding plugin Registration #3212

Draft

11 tasks

Register user-defined-network (UDN) binding on kubevirt CR #3034

Register user-defined-network (UDN) binding on kubevirt CR #3034

Conversation

RamLavi commented Jul 31, 2024 • edited Loading

kubevirt-bot commented Jul 31, 2024

openshift-ci bot commented Jul 31, 2024

nunnatsa 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi commented Jul 31, 2024

RamLavi commented Aug 1, 2024

RamLavi commented Aug 1, 2024

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

Choose a reason for hiding this comment

oshoval Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

RamLavi Aug 4, 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

RamLavi commented Aug 1, 2024

hco-bot commented Aug 12, 2024

kubevirt-bot commented Aug 12, 2024

nunnatsa left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 12, 2024

nunnatsa commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024 • edited Loading

hco-bot commented Aug 12, 2024

kubevirt-bot commented Aug 12, 2024

RamLavi commented Jul 31, 2024 •

edited

Loading

oshoval Aug 1, 2024 •

edited

Loading

RamLavi Aug 4, 2024 •

edited

Loading

openshift-ci bot commented Aug 12, 2024 •

edited

Loading