From bdabec2434bf54c98e8296ed783585d6f407892d Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 17:41:42 +0200 Subject: [PATCH] vm, controller: Create IPAMClaim for primary Signed-off-by: Enrique Llorente --- pkg/vminetworkscontroller/vmi_controller.go | 89 ++++++++++++------- .../vmi_controller_test.go | 85 +++++++++++++----- 2 files changed, 120 insertions(+), 54 deletions(-) diff --git a/pkg/vminetworkscontroller/vmi_controller.go b/pkg/vminetworkscontroller/vmi_controller.go index 3681b4f3..6069f060 100644 --- a/pkg/vminetworkscontroller/vmi_controller.go +++ b/pkg/vminetworkscontroller/vmi_controller.go @@ -27,6 +27,7 @@ import ( "github.com/kubevirt/ipam-extensions/pkg/claims" "github.com/kubevirt/ipam-extensions/pkg/config" + "github.com/kubevirt/ipam-extensions/pkg/udn" ) // VirtualMachineInstanceReconciler reconciles a VirtualMachineInstance object @@ -84,7 +85,7 @@ func (r *VirtualMachineInstanceReconciler) Reconcile( ownerInfo := ownerReferenceFor(vmi, vm) for logicalNetworkName, netConfigName := range vmiNetworks { - claimKey := fmt.Sprintf("%s.%s", vmi.Name, logicalNetworkName) + claimKey := claims.ComposeKey(vmi.Name, logicalNetworkName) ipamClaim := &ipamclaimsapi.IPAMClaim{ ObjectMeta: controllerruntime.ObjectMeta{ Name: claimKey, @@ -158,44 +159,68 @@ func (r *VirtualMachineInstanceReconciler) vmiNetworksClaimingIPAM( ) (map[string]string, error) { vmiNets := make(map[string]string) for _, net := range vmi.Spec.Networks { - if net.Pod != nil { - continue - } - if net.Multus != nil && !net.Multus.Default { - nadName := net.Multus.NetworkName - namespace := vmi.Namespace - namespaceAndName := strings.Split(nadName, "/") - if len(namespaceAndName) == 2 { - namespace = namespaceAndName[0] - nadName = namespaceAndName[1] + if err := r.ensureVMINetworksWithSecondaryUDN(ctx, vmi.Namespace, net, vmiNets); err != nil { + return nil, err } - - contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second) - defer cancel() - nad := &nadv1.NetworkAttachmentDefinition{} - if err := r.Client.Get( - contextWithTimeout, - apitypes.NamespacedName{Namespace: namespace, Name: nadName}, - nad, - ); err != nil { - if apierrors.IsNotFound(err) { - return nil, err - } + } else if net.Pod != nil { + if err := r.ensureVMINetworksWithPrimaryUDN(ctx, vmi.Namespace, net, vmiNets); err != nil { + return nil, err } + } + } - nadConfig, err := config.NewConfig(nad.Spec.Config) - if err != nil { - r.Log.Error(err, "failed extracting the relevant NAD configuration", "NAD name", nadName) - return nil, fmt.Errorf("failed to extract the relevant NAD information") - } + return vmiNets, nil +} - if nadConfig.AllowPersistentIPs { - vmiNets[net.Name] = nadConfig.Name - } +func (r *VirtualMachineInstanceReconciler) ensureVMINetworksWithSecondaryUDN(ctx context.Context, + namespace string, network virtv1.Network, vmiNets map[string]string) error { + nadName := network.Multus.NetworkName + namespaceAndName := strings.Split(nadName, "/") + if len(namespaceAndName) == 2 { + namespace = namespaceAndName[0] + nadName = namespaceAndName[1] + } + + contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second) + defer cancel() + nad := &nadv1.NetworkAttachmentDefinition{} + if err := r.Client.Get( + contextWithTimeout, + apitypes.NamespacedName{Namespace: namespace, Name: nadName}, + nad, + ); err != nil { + if apierrors.IsNotFound(err) { + return err } } - return vmiNets, nil + return r.ensureVMINetworkWithUDN(network, nad, vmiNets) +} + +func (r *VirtualMachineInstanceReconciler) ensureVMINetworksWithPrimaryUDN(ctx context.Context, + namespace string, network virtv1.Network, vmiNets map[string]string) error { + primaryNetworkNAD, err := udn.FindPrimaryNetwork(ctx, r.Client, namespace) + if err != nil { + return err + } + if primaryNetworkNAD == nil { + return nil + } + return r.ensureVMINetworkWithUDN(network, primaryNetworkNAD, vmiNets) +} + +func (r *VirtualMachineInstanceReconciler) ensureVMINetworkWithUDN(network virtv1.Network, + nad *nadv1.NetworkAttachmentDefinition, vmiNets map[string]string) error { + nadConfig, err := config.NewConfig(nad.Spec.Config) + if err != nil { + r.Log.Error(err, "failed extracting the relevant NAD configuration", "NAD name", nad.Name) + return fmt.Errorf("failed to extract the relevant NAD information") + } + + if nadConfig.AllowPersistentIPs { + vmiNets[network.Name] = nadConfig.Name + } + return nil } func shouldCleanFinalizers(vmi *virtv1.VirtualMachineInstance, vm *virtv1.VirtualMachine) bool { diff --git a/pkg/vminetworkscontroller/vmi_controller_test.go b/pkg/vminetworkscontroller/vmi_controller_test.go index d9af04c2..35ffc2ea 100644 --- a/pkg/vminetworkscontroller/vmi_controller_test.go +++ b/pkg/vminetworkscontroller/vmi_controller_test.go @@ -45,7 +45,7 @@ var ( type testConfig struct { inputVM *virtv1.VirtualMachine inputVMI *virtv1.VirtualMachineInstance - inputNAD *nadv1.NetworkAttachmentDefinition + inputNADs []*nadv1.NetworkAttachmentDefinition existingIPAMClaim *ipamclaimsapi.IPAMClaim expectedError error expectedResponse reconcile.Result @@ -94,8 +94,8 @@ var _ = Describe("VMI IPAM controller", Serial, func() { initialObjects = append(initialObjects, config.inputVMI) } - if config.inputNAD != nil { - initialObjects = append(initialObjects, config.inputNAD) + for _, nad := range config.inputNADs { + initialObjects = append(initialObjects, nad) } if config.existingIPAMClaim != nil { @@ -140,10 +140,13 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Expect(ipamClaimsCleaner(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims)) } }, - Entry("when the VM has an associated VMI pointing to an existing NAD", testConfig{ - inputVM: dummyVM(dummyVMISpec(nadName)), - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + Entry("when the VM has an associated VMI pointing to an existing NAD with a primary network at namespace", testConfig{ + inputVM: dummyVM(dummyVMISpec(nadName)), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + dummyPrimaryNetworkNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { @@ -162,19 +165,39 @@ var _ = Describe("VMI IPAM controller", Serial, func() { }, Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"}, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s.%s", vmName, "podnet"), + Namespace: namespace, + Finalizers: []string{claims.KubevirtVMFinalizer}, + Labels: claims.OwnedByVMLabel(vmName), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "kubevirt.io/v1", + Kind: "VirtualMachine", + Name: vmName, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true)}, + }, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "primarynet"}, + }, }, }), Entry("when the VM has an associated VMI pointing to an existing NAD but as multus default network", testConfig{ - inputVM: dummyVM(dummyVMIWithMultusDefaultNetworkSpec(nadName)), - inputVMI: dummyVMI(dummyVMIWithMultusDefaultNetworkSpec(nadName)), - inputNAD: dummyNAD(nadName), + inputVM: dummyVM(dummyVMIWithMultusDefaultNetworkSpec(nadName)), + inputVMI: dummyVMI(dummyVMIWithMultusDefaultNetworkSpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{}, }), Entry("when the VM has an associated VMI pointing to an existing NAD with an improper config", testConfig{ - inputVM: dummyVM(dummyVMISpec(nadName)), - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNADWrongFormat(nadName), + inputVM: dummyVM(dummyVMISpec(nadName)), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNADWrongFormat(nadName), + }, expectedError: fmt.Errorf("failed to extract the relevant NAD information"), }), Entry("the associated VMI exists but points to a NAD that doesn't exist", testConfig{ @@ -267,8 +290,10 @@ var _ = Describe("VMI IPAM controller", Serial, func() { }, }), Entry("standalone VMI which is marked for deletion, with active pods, should keep IPAMClaims finalizers", testConfig{ - inputVMI: dummyMarkedForDeletionVMIWithActivePods(nadName), - inputNAD: dummyNAD(nadName), + inputVMI: dummyMarkedForDeletionVMIWithActivePods(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -331,7 +356,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("everything is OK but there's already an IPAMClaim with this name", testConfig{ inputVM: dummyVM(dummyVMISpec(nadName)), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -344,7 +371,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("found an existing IPAMClaim for the same VM", testConfig{ inputVM: decorateVMWithUID(dummyUID, dummyVM(dummyVMISpec(nadName))), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -386,7 +415,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("found an existing IPAMClaim for a VM with same name but different UID", testConfig{ inputVM: decorateVMWithUID(dummyUID, dummyVM(dummyVMISpec(nadName))), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -407,8 +438,10 @@ var _ = Describe("VMI IPAM controller", Serial, func() { expectedError: fmt.Errorf("failed since it found an existing IPAMClaim for \"vm1.randomnet\""), }), Entry("a lonesome VMI (with no corresponding VM) is a valid migration use-case", testConfig{ - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { @@ -499,7 +532,7 @@ func dummyVMIWithMultusDefaultNetworkSpec(nadName string) virtv1.VirtualMachineI } } -func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { +func dummyNADWithConfig(nadName string, config string) *nadv1.NetworkAttachmentDefinition { namespaceAndName := strings.Split(nadName, "/") return &nadv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -507,11 +540,19 @@ func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { Name: namespaceAndName[1], }, Spec: nadv1.NetworkAttachmentDefinitionSpec{ - Config: `{"name": "goodnet", "allowPersistentIPs": true}`, + Config: config, }, } } +func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName, `{"name": "goodnet", "allowPersistentIPs": true}`) +} + +func dummyPrimaryNetworkNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName+"primary", `{"name": "primarynet", "role": "primary", "allowPersistentIPs": true}`) +} + func dummyNADWrongFormat(nadName string) *nadv1.NetworkAttachmentDefinition { namespaceAndName := strings.Split(nadName, "/") return &nadv1.NetworkAttachmentDefinition{