Fix NAD handling on create/update

Currently only NAD creation was handled for the provided nicMappings, this patch allows update to the nads. Added missing ownership to the managed net-attach-defs so they get's controlled with OVNController reconciler if deleted/updated explicitly. Also added functional tests and fixed some log messages. Closes-Issue: OSPRH-6894
karelyatin · Jun 14, 2024 · 717a2cf · 717a2cf
1 parent 62d4b4e
commit 717a2cf
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 14 deletions.
diff --git a/controllers/ovncontroller_controller.go b/controllers/ovncontroller_controller.go
@@ -441,8 +441,8 @@ func (r *OVNControllerReconciler) reconcileNormal(ctx context.Context, instance
 		common.AppSelector: ovnv1.ServiceNameOVS,
 	}
 
-	// Create additional Physical Network Attachments
-	networkAttachments, err := ovncontroller.CreateAdditionalNetworks(ctx, helper, instance, ovsServiceLabels)
+	// Create or Update additional Physical Network Attachments
+	networkAttachments, err := ovncontroller.CreateOrUpdateAdditionalNetworks(ctx, helper, instance, ovsServiceLabels)
 	if err != nil {
 		Log.Info(fmt.Sprintf("Failed to create additional networks: %s", err))
 		return ctrl.Result{}, err

diff --git a/pkg/ovncontroller/network.go b/pkg/ovncontroller/network.go
@@ -25,8 +25,8 @@ import (
 	ovnv1 "github.com/openstack-k8s-operators/ovn-operator/api/v1beta1"
 )
 
-// CreateAdditionalNetworks - creates network attachement definitions based on the provided mappings
-func CreateAdditionalNetworks(
+// CreateOrUpdateAdditionalNetworks - create or update network attachment definitions based on the provided mappings
+func CreateOrUpdateAdditionalNetworks(
 	ctx context.Context,
 	h *helper.Helper,
 	instance *ovnv1.OVNController,
@@ -37,6 +37,11 @@ func CreateAdditionalNetworks(
 	var networkAttachments []string
 
 	for physNet, interfaceName := range instance.Spec.NicMappings {
+		nadSpec := netattdefv1.NetworkAttachmentDefinitionSpec{
+			Config: fmt.Sprintf(
+				`{"cniVersion": "0.3.1", "name": "%s", "type": "host-device", "device": "%s"}`,
+				physNet, interfaceName),
+		}
 		nad = &netattdefv1.NetworkAttachmentDefinition{}
 		err := h.GetClient().Get(
 			ctx,
@@ -48,27 +53,40 @@ func CreateAdditionalNetworks(
 		)
 		if err != nil {
 			if !k8s_errors.IsNotFound(err) {
-				return nil, fmt.Errorf("can not get NetworkAttachmentDefinition %s/%s: %w",
+				return nil, fmt.Errorf("cannot get NetworkAttachmentDefinition %s/%s: %w",
 					physNet, interfaceName, err)
 			}
 
+			ownerRef := metav1.NewControllerRef(instance, instance.GroupVersionKind())
 			nad = &netattdefv1.NetworkAttachmentDefinition{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      physNet,
-					Namespace: instance.Namespace,
-					Labels:    labels,
-				},
-				Spec: netattdefv1.NetworkAttachmentDefinitionSpec{
-					Config: fmt.Sprintf(
-						`{"cniVersion": "0.3.1", "name": "%s", "type": "host-device", "device": "%s"}`,
-						physNet, interfaceName),
+					Name:            physNet,
+					Namespace:       instance.Namespace,
+					Labels:          labels,
+					OwnerReferences: []metav1.OwnerReference{*ownerRef},
 				},
+				Spec: nadSpec,
 			}
 			// Request object not found, lets create it
 			if err := h.GetClient().Create(ctx, nad); err != nil {
-				return nil, fmt.Errorf("can not create NetworkAttachmentDefinition %s/%s: %w",
+				return nil, fmt.Errorf("cannot create NetworkAttachmentDefinition %s/%s: %w",
 					physNet, interfaceName, err)
 			}
+		} else {
+			owned := false
+			for _, owner := range nad.GetOwnerReferences() {
+				if owner.Name == instance.Name {
+					owned = true
+					break
+				}
+			}
+			if owned {
+				nad.Spec = nadSpec
+				if err := h.GetClient().Update(ctx, nad); err != nil {
+					return nil, fmt.Errorf("cannot update NetworkAttachmentDefinition %s/%s: %w",
+						physNet, interfaceName, err)
+				}
+			}
 		}
 
 		networkAttachments = append(networkAttachments, physNet)

diff --git a/tests/functional/base_test.go b/tests/functional/base_test.go
@@ -25,6 +25,7 @@ import (
 	. "github.com/onsi/gomega" //revive:disable:dot-imports
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -291,6 +292,32 @@ func SimulateDaemonsetNumberReadyWithPods(name types.NamespacedName, networkIPs
 	logger.Info("Simulated daemonset success", "on", name)
 }
 
+func CreateNAD(name types.NamespacedName) *networkv1.NetworkAttachmentDefinition {
+	nad := &networkv1.NetworkAttachmentDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name.Name,
+			Namespace: name.Namespace,
+		},
+		Spec: networkv1.NetworkAttachmentDefinitionSpec{
+			Config: "",
+		},
+	}
+	Eventually(func(g Gomega) {
+		g.Expect(k8sClient.Create(ctx, nad)).Should(Succeed())
+	}).Should(Succeed())
+
+	return nad
+}
+
+func GetNAD(name types.NamespacedName) *networkv1.NetworkAttachmentDefinition {
+	nad := &networkv1.NetworkAttachmentDefinition{}
+	Eventually(func(g Gomega) {
+		g.Expect(k8sClient.Get(ctx, name, nad)).Should(Succeed())
+	}).Should(Succeed())
+
+	return nad
+}
+
 func GetDNSData(name types.NamespacedName) *infranetworkv1.DNSData {
 	dns := &infranetworkv1.DNSData{}
 	Eventually(func(g Gomega) {

diff --git a/tests/functional/ovncontroller_controller_test.go b/tests/functional/ovncontroller_controller_test.go
@@ -620,6 +620,87 @@ var _ = Describe("OVNController controller", func() {
 			DeferCleanup(th.DeleteInstance, instance)
 		})
 
+		It("reports that the networkattachment definition is created with OwnerReferences set", func() {
+			nad := types.NamespacedName{
+				Namespace: OVNControllerName.Namespace,
+				Name:      "physnet1",
+			}
+			// Ensure OwnerReferences set correctly for the created Network Attachment
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(nad).ObjectMeta.OwnerReferences[0].Name).To(Equal(
+					OVNControllerName.Name))
+			}, timeout, interval).Should(Succeed())
+		})
+
+		It("reports that the networkattachment definition is updated with nicMappings update", func() {
+			nad := types.NamespacedName{
+				Namespace: OVNControllerName.Namespace,
+				Name:      "physnet1",
+			}
+			// Ensure NAD exists with defined interface
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(nad).Spec.Config).Should(
+					ContainSubstring("enp2s0.100"))
+			}, timeout, interval).Should(Succeed())
+
+			// Update Interface in NicMappings
+			Eventually(func(g Gomega) {
+				ovnController := GetOVNController(OVNControllerName)
+				ovnController.Spec.NicMappings = map[string]string{
+					"physnet1": "enp3s0.100",
+				}
+				g.Expect(k8sClient.Update(ctx, ovnController)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			// Ensure OwnerReferences set correctly for the updated Network Attachment
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(nad).ObjectMeta.OwnerReferences[0].Name).To(Equal(
+					OVNControllerName.Name))
+			}, timeout, interval).Should(Succeed())
+
+			// Ensure Interface updated in the Network Attachment
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(nad).Spec.Config).Should(
+					ContainSubstring("enp3s0.100"))
+			}, timeout, interval).Should(Succeed())
+		})
+
+		It("should not update the networkattachment definition created externally", func() {
+			nad := types.NamespacedName{
+				Namespace: OVNControllerName.Namespace,
+				Name:      "physnet1",
+			}
+			// Ensure NAD exists with defined interface
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(nad).Spec.Config).Should(
+					ContainSubstring("enp2s0.100"))
+			}, timeout, interval).Should(Succeed())
+
+			extNADName := types.NamespacedName{Namespace: namespace, Name: "external"}
+			nadInstance := CreateNAD(extNADName)
+			DeferCleanup(th.DeleteInstance, nadInstance)
+
+			// Update NicMappings to use existing Network Attachment
+			Eventually(func(g Gomega) {
+				ovnController := GetOVNController(OVNControllerName)
+				ovnController.Spec.NicMappings = map[string]string{
+					"external": "enp3s0.100",
+				}
+				g.Expect(k8sClient.Update(ctx, ovnController)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			// Ensure OwnerReferences not added to not managed Network Attachment
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(extNADName).ObjectMeta.OwnerReferences).Should(BeNil())
+			}, timeout, interval).Should(Succeed())
+
+			// Ensure Interface not updated in not managed Network Attachment
+			Eventually(func(g Gomega) {
+				g.Expect(GetNAD(extNADName).Spec.Config).ShouldNot(
+					ContainSubstring("enp3s0.100"))
+			}, timeout, interval).Should(Succeed())
+		})
+
 		It("reports that the networkattachment definition is created based on nic configs", func() {
 			daemonSetName := types.NamespacedName{
 				Namespace: namespace,