Add primary UDN net-attach-def #1841

RamLavi · 2024-08-04T12:40:59Z

What this PR does / why we need it:
This PR adds support to user-defined-network binding by deploying the appropriate NetworkAttachmentDefinition.
This is part of the process described on kubevirt user guide.
The behavior added is currently dev-preview. Hence, the NetworkAttachmentDefinition is not deployed by the proper component, but is deployed by ipam-extentions. This should change in future PRs.

Special notes for your reviewer:

Release note:

NONE

oshoval · 2024-08-05T09:12:27Z

hack/components/bump-kubevirt-ipam-controller.sh

@@ -144,6 +144,31 @@ echo 'Adjust kubevirt-ipam-controller to CNAO'
  sed -i '/containers:/i\{{ if .EnableSCC }}\
      serviceAccountName: passt-binding-cni\
 {{ end }}' 003-passtbindingcni.yaml
+
+  cat <<EOF > NetworkAttachmentDefinition_primary-user-defined-network.yaml


instead all the changes in this file, since this yaml is generated by CNAO
you can just keep it in the data folder, and add it to line 176
which means what to not delete

rm -rf data/kubevirt-ipam-controller/!(002-rbac.yaml|<file_name_here>)

good idea. thanks!

oshoval · 2024-08-05T09:14:49Z

pkg/network/kubevirt_ipam_controller.go

@@ -49,6 +49,7 @@ func renderKubevirtIPAMController(conf *cnao.NetworkAddonsConfigSpec, manifestDi
 	}
 	data.Data["IsOpenshift"] = clusterInfo.OpenShift4
 	data.Data["EnableSCC"] = clusterInfo.SCCAvailable
+	data.Data["EnableNetworkAttachmentDefinition"] = clusterInfo.NetAttachDefAvailable


if the CRD is NA, we should add a fatal
so basically this isn't needed (just need to fatal if CRD is NA + conf.ipam == true)

the only advantage of having this var, is that we can disable it easily if needed
but imho it doesnt worth it for this reason
moreover we can check if CRD exists, only if conf.ipam == true, not always

wdyt ?

I think it was discussed that we want to fail early.. totally forgot. will fix. thanks!

fail fast, ye but only if conf.ipam == true + nad doesnt exists please imo

RamLavi · 2024-08-05T10:51:14Z

@oshoval do you understand why it fails?

oshoval · 2024-08-05T10:52:59Z

@oshoval do you understand why it fails?

error: timed out waiting for the condition on kubevirts/kubevirt
for some reason lately it cant install kubevirt all the time

maybe it is because the wait was too fast, and wait doesnt wait for the resource to be created
and then need a loop of wait with shorter timeout, or to use the new flag of kubectl wait that waits for the resource to exists kubernetes/kubernetes#122994

unless it is something deeper, and to know we would need logs

oshoval · 2024-08-05T11:56:49Z

pkg/network/kubevirt_ipam_controller_test.go

+		conf := &cnao.NetworkAddonsConfigSpec{ImagePullPolicy: v1.PullAlways, Multus: &cnao.Multus{}, KubevirtIpamController: &cnao.KubevirtIpamController{}, PlacementConfiguration: &cnao.PlacementConfiguration{Workloads: &cnao.Placement{}}}
+		manifestDir := "../../data"
+		openshiftNetworkConf := &osv1.Network{}
+		clusterInfo := &ClusterInfo{SCCAvailable: true, OpenShift4: false}


why SCCAvailable true ?

it shouldn't be. fixed.

pkg/network/kubevirt_ipam_controller_test.go

oshoval · 2024-08-05T11:59:01Z

test/check/check.go

@@ -346,6 +347,10 @@ func checkForComponent(component *Component) error {
 		errsAppend(checkForPrometheusRule(component.PrometheusRule))
 	}

+	if component.NetworkAttachmentDefinition != "" {
+		errsAppend(checkForNetworkAttachmentDefinition(component.PrometheusRule))


PrometheusRule is wrong here

ah copy err. thanks!

oshoval · 2024-08-05T12:00:49Z

test/check/check.go

@@ -649,6 +658,21 @@ func checkForPrometheusRule(name string) error {
 	return nil
 }

+func checkForNetworkAttachmentDefinition(name string) error {


but why we need all this logic if we just create the nad and if it doesnt exists we have the error that nad doesnt exists without any additional changes?

is it a must because it was added here ?
https://github.com/kubevirt/cluster-network-addons-operator/pull/1841/files#diff-dd3d0d54317e754fd95de914de01a73b5578626b42e755fdad4998186d552600R90

but why we need all this logic if we just create the nad

This code is for CNAO CI to make sure the NAD is deployed. It's now CNAO's responsibility, so CI should check it's deployed, and also removed.

and if it doesnt exists we have the error that nad doesnt exists without any additional changes?

small correction - only if the NAD's CRD doesn't exists then CNAO will fail to deploy with the error you mention.

is it a must because it was added here ? https://github.com/kubevirt/cluster-network-addons-operator/pull/1841/files#diff-dd3d0d54317e754fd95de914de01a73b5578626b42e755fdad4998186d552600R90

technically yeah, but it's intentional. As I mentioned above - it's CNAO's job to make sure the resources it is in charge of deploying are deployed.

RamLavi · 2024-08-05T12:03:18Z

@oshoval do you understand why it fails?

error: timed out waiting for the condition on kubevirts/kubevirt for some reason lately it cant install kubevirt all the time

maybe it is because the wait was too fast, and wait doesnt wait for the resource to be created and then need a loop of wait with shorter timeout, or to use the new flag of kubectl wait that waits for the resource to exists kubernetes/kubernetes#122994

unless it is something deeper, and to know we would need logs

Looks like kubevirt/kubevirt now wait 15m

oshoval · 2024-08-05T12:06:16Z

@oshoval do you understand why it fails?

error: timed out waiting for the condition on kubevirts/kubevirt for some reason lately it cant install kubevirt all the time
maybe it is because the wait was too fast, and wait doesnt wait for the resource to be created and then need a loop of wait with shorter timeout, or to use the new flag of kubectl wait that waits for the resource to exists kubernetes/kubernetes#122994
unless it is something deeper, and to know we would need logs

Looks like kubevirt/kubevirt now wait 15m

this change is 4y ago it seems, but worth to try to extend

oshoval · 2024-08-05T12:18:23Z

pkg/network/kubevirt_ipam_controller_test.go

+		conf := &cnao.NetworkAddonsConfigSpec{ImagePullPolicy: v1.PullAlways, Multus: &cnao.Multus{}, KubevirtIpamController: &cnao.KubevirtIpamController{}, PlacementConfiguration: &cnao.PlacementConfiguration{Workloads: &cnao.Placement{}}}
+		manifestDir := "../../data"
+		openshiftNetworkConf := &osv1.Network{}
+		clusterInfo := &ClusterInfo{SCCAvailable: false, OpenShift4: false}


basically since all are false, those are the default values anyhow
so you can just create an empty struct
unless one of them is better to be explicit false, but this is not the case here

oshoval · 2024-08-05T12:45:18Z

@oshoval do you understand why it fails?

error: timed out waiting for the condition on kubevirts/kubevirt for some reason lately it cant install kubevirt all the time
maybe it is because the wait was too fast, and wait doesnt wait for the resource to be created and then need a loop of wait with shorter timeout, or to use the new flag of kubectl wait that waits for the resource to exists kubernetes/kubernetes#122994
unless it is something deeper, and to know we would need logs

Looks like kubevirt/kubevirt now wait 15m

this change is 4y ago it seems, but worth to try to extend

I think the best we cant do is to install kubevirt using kind.sh
for that we need to add an option to bypass kubevirt-ipam-controller installation / ipam claim crd / cert-manager
or to undeploy them after installation is done
one of the reasons is that kind.sh does this when installing kubevirt (among few other little things)
or we can also just add those in our repo

        $OCI_BIN exec -t $node bash -c "echo 'fs.inotify.max_user_watches=1048576' >> /etc/sysctl.conf"
        $OCI_BIN exec -t $node bash -c "echo 'fs.inotify.max_user_instances=512' >> /etc/sysctl.conf"
        $OCI_BIN exec -i $node bash -c "sysctl -p /etc/sysctl.conf"
        if [[ "${node}" =~ worker ]]; then
            kubectl label nodes $node node-role.kubernetes.io/worker="" --overwrite=true
        fi

oshoval · 2024-08-05T13:55:51Z

pkg/components/components.go

@@ -742,6 +742,9 @@ func GetClusterRole(allowMultus bool) *rbacv1.ClusterRole {
 					"get",
 					"list",
 					"watch",
+					"create",
+					"update",


do we need update ? (maybe the reconcile use it when it recreate?)

please consider moving it under the else of allowMultus
otherwise there are duplicate / overlapping rbacs due to multusClusterRoles in U/S, which i tried to refrain from (well indeed on some places like get list watch i did add, because it was just for NAD and needed on both U/S and D/S, so whatever you prefer)

you can move the flag into multusClusterRoles and it will determine if to add * or to add just the diff needed please

do we need update ? (maybe the reconcile use it when it recreate?)

We do.

please consider moving it under the else of allowMultus otherwise there are duplicate / overlapping rbacs due to multusClusterRoles in U/S, which i tried to refrain from

you can move the flag into multusClusterRoles and it will determine if to add * or to add just the diff needed please

OK, DONE

RamLavi · 2024-08-05T14:12:09Z

@oshoval do you understand why it fails?

error: timed out waiting for the condition on kubevirts/kubevirt for some reason lately it cant install kubevirt all the time
maybe it is because the wait was too fast, and wait doesnt wait for the resource to be created and then need a loop of wait with shorter timeout, or to use the new flag of kubectl wait that waits for the resource to exists kubernetes/kubernetes#122994
unless it is something deeper, and to know we would need logs

Looks like kubevirt/kubevirt now wait 15m

this change is 4y ago it seems, but worth to try to extend

I think the best we cant do is to install kubevirt using kind.sh for that we need to add an option to bypass kubevirt-ipam-controller installation / ipam claim crd / cert-manager or to undeploy them after installation is done one of the reasons is that kind.sh does this when installing kubevirt (among few other little things) or we can also just add those in our repo
        $OCI_BIN exec -t $node bash -c "echo 'fs.inotify.max_user_watches=1048576' >> /etc/sysctl.conf"
        $OCI_BIN exec -t $node bash -c "echo 'fs.inotify.max_user_instances=512' >> /etc/sysctl.conf"
        $OCI_BIN exec -i $node bash -c "sysctl -p /etc/sysctl.conf"
        if [[ "${node}" =~ worker ]]; then
            kubectl label nodes $node node-role.kubernetes.io/worker="" --overwrite=true
        fi

Maybe, but it should be done in a separate PR to be sure.
IIRC this is the first time that lane began to be flaky, am I correct?

oshoval · 2024-08-05T14:15:20Z

Maybe, but it should be done in a separate PR to be sure. IIRC this is the first time that lane began to be flaky, am I correct?

started lately
saw it one time or so before this PR

oshoval · 2024-08-07T06:28:23Z

test/e2e/workflow/deployment_test.go

+				timeout := CheckImmediately
+				if len(components) > 1 {
+					By("allowing eventual consistency on co-dependant components deployments")
+					timeout = 10 * time.Second
+				}


what about just always allow 10s ?
and then we can drop this code

yeah I tend to agree..

nit: maybe the commit message of that need to be changed please

oshoval · 2024-08-07T08:32:01Z

about the workflow failure - this is not a solution but maybe a temporary workaround and then we can fix it on a follow-up (imo 10s is not enough as we spoke):
please check if you want this function deploy_cnao_cr, and make sure multus is in the CR before the ipam one
it will save some reconcile cycles

EDIT - if CNAO waits for processing / ready state too short, it might fail because in the middle the state becomes fail for various errors, only eventually it will be processing / ready - not sure if relevant here, but i did saw it before
in areas that remind this area - to conclude we should be here more tolerant imho and eventually consistent, not too strict

oshoval · 2024-08-07T09:19:21Z

test/check/check.go

+	if component.NetworkAttachmentDefinition != "" {
+		errsAppend(checkForNetworkAttachmentDefinitionRemoval(component.NetworkAttachmentDefinition))
+	}
+


The condition can be moved into the function
but we can consider please doing it on a follow-up for all those methods not in this PR

same for the checkForNetworkAttachmentDefinition family

but we can consider doing it on a follow-up for all those methods not in this PR

+1

RamLavi · 2024-08-07T10:01:08Z

about the workflow failure - this is not a solution but maybe a temporary workaround and then we can fix it on a follow-up (imo 10s is not enough as we spoke): please check if you want this function deploy_cnao_cr, and make sure multus is in the CR before the ipam one it will save some reconcile cycles

EDIT - if CNAO waits for processing / ready state too short, it might fail because in the middle the state becomes fail for various errors, only eventually it will be processing / ready - not sure if relevant here, but i did saw it before in areas that remind this area - to conclude we should be here more tolerant imho and eventually consistent, not too strict

The issue was not relevant to timing, something much more dumb.
Removed the timeouts, let's see how we handle without them.

oshoval · 2024-08-07T11:38:51Z

Please let me know once all is passing and ready for review,
I think it will be better for us both.
of course if there are any questions just mention me please
Thanks

oshoval

nits, beside that lgtm
thanks

oshoval · 2024-08-08T06:09:09Z

test/check/check.go

+	err := testenv.Client.Get(context.Background(), types.NamespacedName{Name: name, Namespace: corev1.NamespaceDefault}, &networkAttachmentDefinition)
+	if err != nil {
+		return err
+	}


please use
if ...; err != nil {
}

It's a cosmetic change, that should be done to all of the checkForX functions above. Let's do it in a separate refactor PR that does not change logic.

well this is a new function, but i wont insist

I will make the change in a separate PR after this one is merged

oshoval · 2024-08-08T06:10:32Z

test/check/check.go

+	if err != nil {
+		return err
+	}
+
+	return nil


here you can just return err basically, but one can consider it less readable (imho it is fine fwiw)
so either this please or short writing

if ...; err != nil { }

oshoval · 2024-08-08T06:52:01Z

pkg/components/components.go

@@ -742,6 +742,9 @@ func GetClusterRole(allowMultus bool) *rbacv1.ClusterRole {
 					"get",
 					"list",
 					"watch",
+					"create",


one thing we can consider
limit this to specific NS (i.e default atm)
or even better - use it as Role instead ClusterRole
well for that we need a Role / RoleBinding in the default NS (not that this is risky a bit with backward compatibility once the NS is changed - for this reason we can postpone thinking of it and keep it ClusterRole atm)

My thoughts exactly.. we can decide to use more specifc RBAC when we decide what the final resting place of this NAD

oshoval · 2024-08-08T06:59:22Z

/lgtm cancel

please at least consider #1841 (comment)
if needed we can done it on a follow-up (in this case please consider to open an issue to track it),
but if it is possible to do it in this one it is better

EDIT - since using a Role can be risky later for backward compatibility in case the NS will be changed
we better postpone thinking about it and just use ClusterRole atm

qinqon

first pass let's not reference UDN here.

qinqon · 2024-08-08T06:47:56Z

data/kubevirt-ipam-controller/004-primary-udn-networkattachdef.yaml

+apiVersion: "k8s.cni.cncf.io/v1"
+kind: NetworkAttachmentDefinition
+metadata:
+  name: primary-user-defined-network


Let's keep this primary-user-defined-network naming just for the name of the kubevirt binding, so it's exposed to users just as VM spec,

We should be specific here same ass we are with passt-binding-cni DaemonSet

So name should be something like

name: [CNAO namespace]-kubevirt-passt-binding-plugin

[CNAO namespace]

why CNAO namespace? CNAO only deploys, the ns has nothing to do with this NAD..

Also, CNAO's ns change on U/S,D/S, do we want the NAD's name to be different? (I mean, it's not braking anything, it's just kinda strange)

After some back and forth let's name this

name: primary-udn-kubevirt-binding

qinqon · 2024-08-08T07:00:51Z

data/kubevirt-ipam-controller/004-primary-udn-networkattachdef.yaml

+spec:
+  config: '{
+  "cniVersion": "1.0.0",
+  "name": "primary-user-defined-network",


Let's differenciate from nad name, since those are different

name: [CNAO namespace]-kubevirt-passt-binding-plugin-net

After some back and forth let's name this

name: primary-udn-kubevirt-binding

qinqon · 2024-08-08T07:01:35Z

hack/components/bump-kubevirt-ipam-controller.sh

@@ -148,7 +148,7 @@ echo 'Adjust kubevirt-ipam-controller to CNAO'

 echo 'Copy manifests'
 shopt -s extglob
-rm -rf data/kubevirt-ipam-controller/!(002-rbac.yaml)
+rm -rf data/kubevirt-ipam-controller/!(002-rbac.yaml|004-primary-udn-networkattachdef.yaml)


ditto: Let's not reference UDN at the nad, just at the kubevirt CR config on HCO.

hmm, I'm not against this, but how will anyone ever understand the connection between the NAD and the binding? shouldn't they have at least something in common? a label maybe?

Let's name this

004-primary-udn-kubevirt-binding-networkattachdef.yaml

maybe just short nad ?
004-primary-udn-kubevirt-binding-nad.yaml

This resource does not belong to kubevirt-ipam-controller, but is currently piggy-backing this component in order to deploy the primary-udn-kubevirt-binding net-attach-def [0]. This net-attach-def is deployed on default namespace, as this way it will be available to all VMs that need to consume it. Note that for CI, KubevirtIpamController will not be able to deploy without Multus (that deploys the net-attach-def CRD) alongside it, or the net-attach-def deployment will fail, with error: `could not apply (k8s.cni.cncf.io/v1, Kind=NetworkAttachmentDefinition) default/primary-user-defined-network: could not retrieve existing (k8s.cni.cncf.io/v1, Kind=NetworkAttachmentDefinition) default/primary-user-defined-network: no matches for kind "NetworkAttachmentDefinition" in version "k8s.cni.cncf.io/v1"` [0] https://kubevirt.io/user-guide/network/network_binding_plugins/#deployment Signed-off-by: Ram Lavi <[email protected]>

sonarqubecloud · 2024-08-08T08:20:34Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.8% Duplication on New Code

See analysis details on SonarCloud

RamLavi · 2024-08-08T08:21:59Z

Change: Fixes for @qinqon 's review

qinqon

/lgtm

RamLavi · 2024-08-08T08:26:46Z

/approve

kubevirt-bot · 2024-08-08T08:26:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

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

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

oshoval · 2024-08-08T13:36:02Z

Please create a patch release, so it will be consumed wherever needed

This reverts commit ccb7ca2. Signed-off-by: Ram Lavi <[email protected]>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 4, 2024

kubevirt-bot requested review from oshoval and qinqon August 4, 2024 12:41

kubevirt-bot added the size/L label Aug 4, 2024

RamLavi changed the title ~~[WIP] Add primary udn nad~~ [WIP] Add primary udn net-attach-def Aug 4, 2024

RamLavi force-pushed the add_primary_udn_nad branch from b6faf6a to 50d9dbd Compare August 5, 2024 05:19

RamLavi changed the title ~~[WIP] Add primary udn net-attach-def~~ Add primary udn net-attach-def Aug 5, 2024

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 5, 2024

RamLavi force-pushed the add_primary_udn_nad branch from 50d9dbd to 00d32ac Compare August 5, 2024 09:08