From 717a2cf7e0b96aceef46fa3be54ddf9c8b024d15 Mon Sep 17 00:00:00 2001 From: Yatin Karel Date: Wed, 12 Jun 2024 11:28:10 -0400 Subject: [PATCH] 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 --- controllers/ovncontroller_controller.go | 4 +- pkg/ovncontroller/network.go | 42 +++++++--- tests/functional/base_test.go | 27 +++++++ .../ovncontroller_controller_test.go | 81 +++++++++++++++++++ 4 files changed, 140 insertions(+), 14 deletions(-) diff --git a/controllers/ovncontroller_controller.go b/controllers/ovncontroller_controller.go index c077fbf1..29705da2 100644 --- 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 index 4b198b58..e671db99 100644 --- 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 index c9b4a036..0958e7d8 100644 --- 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 index 4421c5cc..5123a3d0 100644 --- 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,