From b38841906cdc4e1133b70a358d212a871d588019 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Thu, 16 May 2024 16:18:52 +0200 Subject: [PATCH 1/5] controller: add KubeVirt finalizer on IPAMClaim creation Signed-off-by: Miguel Duarte Barroso --- pkg/vmnetworkscontroller/controller.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/vmnetworkscontroller/controller.go b/pkg/vmnetworkscontroller/controller.go index cd36e391..fe585f89 100644 --- a/pkg/vmnetworkscontroller/controller.go +++ b/pkg/vmnetworkscontroller/controller.go @@ -26,6 +26,8 @@ import ( "github.com/maiqueb/kubevirt-ipam-claims/pkg/config" ) +const kubevirtVMFinalizer = "kubevirt.io/persistent-ipam" + // VirtualMachineReconciler reconciles a VirtualMachineInstance object type VirtualMachineReconciler struct { client.Client @@ -114,10 +116,12 @@ func (r *VirtualMachineReconciler) Reconcile( Name: claimKey, Namespace: vmi.Namespace, OwnerReferences: []metav1.OwnerReference{ownerInfo}, + Finalizers: []string{kubevirtVMFinalizer}, }, Spec: ipamclaimsapi.IPAMClaimSpec{ Network: nadConfig.Name, - }} + }, + } if err := r.Client.Create(ctx, ipamClaim, &client.CreateOptions{}); err != nil { if apierrors.IsAlreadyExists(err) { From 4982cb06b3e720c9083b9cc6dcfc2d9450d45cdd Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Thu, 16 May 2024 17:17:32 +0200 Subject: [PATCH 2/5] controller: index the logical network names to CNI config network names This allows us to store this mapping, and later on iterate it for cleanup. Signed-off-by: Miguel Duarte Barroso --- pkg/vmnetworkscontroller/controller.go | 163 ++++++++++++++----------- 1 file changed, 90 insertions(+), 73 deletions(-) diff --git a/pkg/vmnetworkscontroller/controller.go b/pkg/vmnetworkscontroller/controller.go index fe585f89..defbae46 100644 --- a/pkg/vmnetworkscontroller/controller.go +++ b/pkg/vmnetworkscontroller/controller.go @@ -51,9 +51,9 @@ func (r *VirtualMachineReconciler) Reconcile( ) (controllerruntime.Result, error) { vmi := &virtv1.VirtualMachineInstance{} - ctx, cancel := context.WithTimeout(ctx, time.Second) + contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second) defer cancel() - err := r.Client.Get(ctx, request.NamespacedName, vmi) + err := r.Client.Get(contextWithTimeout, request.NamespacedName, vmi) if apierrors.IsNotFound(err) { r.Log.Error(err, "Error retrieving VMI") // Error reading the object - requeue the request. @@ -62,7 +62,9 @@ func (r *VirtualMachineReconciler) Reconcile( var ownerInfo metav1.OwnerReference vm := &virtv1.VirtualMachine{} - if err := r.Client.Get(ctx, request.NamespacedName, vm); apierrors.IsNotFound(err) { + contextWithTimeout, cancel = context.WithTimeout(ctx, time.Second) + defer cancel() + if err := r.Client.Get(contextWithTimeout, request.NamespacedName, vm); apierrors.IsNotFound(err) { r.Log.Info("Corresponding VM not found", "vm", request.NamespacedName) ownerInfo = metav1.OwnerReference{APIVersion: vmi.APIVersion, Kind: vmi.Kind, Name: vmi.Name, UID: vmi.UID} } else if err == nil { @@ -75,84 +77,53 @@ func (r *VirtualMachineReconciler) Reconcile( ) } - vmiSpec := vmi.Spec - for _, net := range vmiSpec.Networks { - if net.Pod != nil { - continue - } - - if net.Multus != nil { - nadName := net.Multus.NetworkName - namespace := vmi.Namespace - namespaceAndName := strings.Split(nadName, "/") - if len(namespaceAndName) == 2 { - namespace = namespaceAndName[0] - nadName = namespaceAndName[1] - } - - ctx, cancel := context.WithTimeout(ctx, time.Second) - defer cancel() - nad := &nadv1.NetworkAttachmentDefinition{} - if err := r.Client.Get( - ctx, - apitypes.NamespacedName{Namespace: namespace, Name: nadName}, - nad, - ); err != nil { - if apierrors.IsNotFound(err) { - return controllerruntime.Result{}, err - } - } + vmiNetworks, err := r.vmiNetworksClaimingIPAM(ctx, vmi) + if err != nil { + return controllerruntime.Result{}, 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 controllerruntime.Result{}, fmt.Errorf("failed to extract the relevant NAD information") - } + for logicalNetworkName, netConfigName := range vmiNetworks { + claimKey := fmt.Sprintf("%s.%s", vmi.Name, logicalNetworkName) + ipamClaim := &ipamclaimsapi.IPAMClaim{ + ObjectMeta: controllerruntime.ObjectMeta{ + Name: claimKey, + Namespace: vmi.Namespace, + OwnerReferences: []metav1.OwnerReference{ownerInfo}, + Finalizers: []string{kubevirtVMFinalizer}, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{ + Network: netConfigName, + }, + } - if nadConfig.AllowPersistentIPs { - claimKey := fmt.Sprintf("%s.%s", vmi.Name, net.Name) - ipamClaim := &ipamclaimsapi.IPAMClaim{ - ObjectMeta: controllerruntime.ObjectMeta{ - Name: claimKey, - Namespace: vmi.Namespace, - OwnerReferences: []metav1.OwnerReference{ownerInfo}, - Finalizers: []string{kubevirtVMFinalizer}, - }, - Spec: ipamclaimsapi.IPAMClaimSpec{ - Network: nadConfig.Name, - }, + if err := r.Client.Create(ctx, ipamClaim, &client.CreateOptions{}); err != nil { + if apierrors.IsAlreadyExists(err) { + claimKey := apitypes.NamespacedName{ + Namespace: vmi.Namespace, + Name: claimKey, } - if err := r.Client.Create(ctx, ipamClaim, &client.CreateOptions{}); err != nil { - if apierrors.IsAlreadyExists(err) { - claimKey := apitypes.NamespacedName{ - Namespace: vmi.Namespace, - Name: claimKey, - } - - existingIPAMClaim := &ipamclaimsapi.IPAMClaim{} - if err := r.Client.Get(ctx, claimKey, existingIPAMClaim); err != nil { - if apierrors.IsNotFound(err) { - // we assume it had already cleaned up in the few miliseconds it took to get here ... - // TODO does this make sense? ... It's pretty much just for completeness. - continue - } else if err != nil { - return controllerruntime.Result{}, fmt.Errorf("let us be on the safe side and retry later") - } - } - if len(existingIPAMClaim.OwnerReferences) == 1 && existingIPAMClaim.OwnerReferences[0].UID == vm.UID { - r.Log.Info("found existing IPAMClaim belonging to this VM, nothing to do", "VM UID", vm.UID) - continue - } else { - err := fmt.Errorf("failed since it found an existing IPAMClaim for %q", claimKey.Name) - r.Log.Error(err, "leaked IPAMClaim found", "existing owner", existingIPAMClaim.UID) - return controllerruntime.Result{}, err - } + existingIPAMClaim := &ipamclaimsapi.IPAMClaim{} + if err := r.Client.Get(ctx, claimKey, existingIPAMClaim); err != nil { + if apierrors.IsNotFound(err) { + // we assume it had already cleaned up in the few miliseconds it took to get here ... + // TODO does this make sense? ... It's pretty much just for completeness. + continue + } else if err != nil { + return controllerruntime.Result{}, fmt.Errorf("let us be on the safe side and retry later") } - r.Log.Error(err, "failed to create the IPAMClaim") + } + if len(existingIPAMClaim.OwnerReferences) == 1 && existingIPAMClaim.OwnerReferences[0].UID == vm.UID { + r.Log.Info("found existing IPAMClaim belonging to this VM, nothing to do", "VM UID", vm.UID) + continue + } else { + err := fmt.Errorf("failed since it found an existing IPAMClaim for %q", claimKey.Name) + r.Log.Error(err, "leaked IPAMClaim found", "existing owner", existingIPAMClaim.UID) return controllerruntime.Result{}, err } } + r.Log.Error(err, "failed to create the IPAMClaim") + return controllerruntime.Result{}, err } } @@ -183,3 +154,49 @@ func onVMPredicates() predicate.Funcs { }, } } + +func (r *VirtualMachineReconciler) vmiNetworksClaimingIPAM( + ctx context.Context, + vmi *virtv1.VirtualMachineInstance, +) (map[string]string, error) { + vmiNets := make(map[string]string) + for _, net := range vmi.Spec.Networks { + if net.Pod != nil { + continue + } + + if net.Multus != nil { + nadName := net.Multus.NetworkName + namespace := vmi.Namespace + 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 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") + } + + if nadConfig.AllowPersistentIPs { + vmiNets[net.Name] = nadConfig.Name + } + } + } + return vmiNets, nil +} From 3e442652e7effc94249bc2091e39193a4a385764 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Fri, 17 May 2024 10:57:34 +0200 Subject: [PATCH 3/5] controller: label IPAMClaims w/ their owner VM name When reacting to the VM deletion it will be easier to query for the IPAMClaims associated to a VM, because we will be able to query for this label. Signed-off-by: Miguel Duarte Barroso --- pkg/vmnetworkscontroller/controller.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/vmnetworkscontroller/controller.go b/pkg/vmnetworkscontroller/controller.go index defbae46..3fe9650f 100644 --- a/pkg/vmnetworkscontroller/controller.go +++ b/pkg/vmnetworkscontroller/controller.go @@ -90,6 +90,9 @@ func (r *VirtualMachineReconciler) Reconcile( Namespace: vmi.Namespace, OwnerReferences: []metav1.OwnerReference{ownerInfo}, Finalizers: []string{kubevirtVMFinalizer}, + Labels: map[string]string{ + virtv1.VirtualMachineLabel: vmi.Name, + }, }, Spec: ipamclaimsapi.IPAMClaimSpec{ Network: netConfigName, From 326cce0ff26a3aff433cc75f39fc2a92f4b89303 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Fri, 17 May 2024 11:51:00 +0200 Subject: [PATCH 4/5] controller: remove IPAMClaim finalizers on VMI delete events This way, we ensure the CNI only returns the IP addresses to the pool after the VMI has been deleted on VM foreground deletes. Without it, the IPAMClaim would be deleted by the Kubernetes garbage collector before the VMI was deleted, which would cause the IP address to be released to the pool (by the CNI) prematurely. More details on [0]. [0] - https://kubernetes.io/docs/concepts/architecture/garbage-collection/#foreground-deletion Signed-off-by: Miguel Duarte Barroso --- pkg/vmnetworkscontroller/controller.go | 38 ++++++-- pkg/vmnetworkscontroller/controller_test.go | 96 +++++++++++++++------ 2 files changed, 102 insertions(+), 32 deletions(-) diff --git a/pkg/vmnetworkscontroller/controller.go b/pkg/vmnetworkscontroller/controller.go index 3fe9650f..cccd0388 100644 --- a/pkg/vmnetworkscontroller/controller.go +++ b/pkg/vmnetworkscontroller/controller.go @@ -15,6 +15,7 @@ import ( controllerruntime "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -55,9 +56,11 @@ func (r *VirtualMachineReconciler) Reconcile( defer cancel() err := r.Client.Get(contextWithTimeout, request.NamespacedName, vmi) if apierrors.IsNotFound(err) { - r.Log.Error(err, "Error retrieving VMI") - // Error reading the object - requeue the request. - return controllerruntime.Result{}, err + r.Log.Info("could not retrieve VMI - will cleanup its IPAMClaims") + if err := r.Cleanup(request.NamespacedName); err != nil { + return controllerruntime.Result{}, fmt.Errorf("error removing the IPAMClaims finalizer: %w", err) + } + return controllerruntime.Result{}, nil } var ownerInfo metav1.OwnerReference @@ -90,9 +93,7 @@ func (r *VirtualMachineReconciler) Reconcile( Namespace: vmi.Namespace, OwnerReferences: []metav1.OwnerReference{ownerInfo}, Finalizers: []string{kubevirtVMFinalizer}, - Labels: map[string]string{ - virtv1.VirtualMachineLabel: vmi.Name, - }, + Labels: ownedByVMLabel(vmi.Name), }, Spec: ipamclaimsapi.IPAMClaimSpec{ Network: netConfigName, @@ -147,7 +148,7 @@ func onVMPredicates() predicate.Funcs { return true }, DeleteFunc: func(event.DeleteEvent) bool { - return false + return true }, UpdateFunc: func(updateEvent event.UpdateEvent) bool { return false @@ -203,3 +204,26 @@ func (r *VirtualMachineReconciler) vmiNetworksClaimingIPAM( } return vmiNets, nil } + +func (r *VirtualMachineReconciler) Cleanup(vmiKey apitypes.NamespacedName) error { + ipamClaims := &ipamclaimsapi.IPAMClaimList{} + if err := r.Client.List(context.Background(), ipamClaims, ownedByVMLabel(vmiKey.Name)); err != nil { + return fmt.Errorf("could not get list of IPAMClaims owned by VM %q: %w", vmiKey.String(), err) + } + + for _, claim := range ipamClaims.Items { + removedFinalizer := controllerutil.RemoveFinalizer(&claim, kubevirtVMFinalizer) + if removedFinalizer { + if err := r.Client.Update(context.Background(), &claim, &client.UpdateOptions{}); err != nil { + return client.IgnoreNotFound(err) + } + } + } + return nil +} + +func ownedByVMLabel(vmiName string) client.MatchingLabels { + return map[string]string{ + virtv1.VirtualMachineLabel: vmiName, + } +} diff --git a/pkg/vmnetworkscontroller/controller_test.go b/pkg/vmnetworkscontroller/controller_test.go index 86be14ad..a65f3498 100644 --- a/pkg/vmnetworkscontroller/controller_test.go +++ b/pkg/vmnetworkscontroller/controller_test.go @@ -45,7 +45,7 @@ type testConfig struct { existingIPAMClaim *ipamclaimsapi.IPAMClaim expectedError error expectedResponse reconcile.Result - expectedIPAMClaims []ipamclaimsapi.IPAMClaimSpec + expectedIPAMClaims []ipamclaimsapi.IPAMClaim } var _ = Describe("vm IPAM controller", Serial, func() { @@ -96,6 +96,14 @@ var _ = Describe("vm IPAM controller", Serial, func() { initialObjects = append(initialObjects, config.existingIPAMClaim) } + if vmiKey.Namespace == "" && vmiKey.Name == "" { + // must apply some default for the VMI DEL scenarios ... + vmiKey = apitypes.NamespacedName{ + Namespace: namespace, + Name: vmName, + } + } + ctrlOptions := controllerruntime.Options{ Scheme: scheme.Scheme, NewClient: func(_ *rest.Config, _ client.Options) (client.Client, error) { @@ -121,10 +129,9 @@ var _ = Describe("vm IPAM controller", Serial, func() { if len(config.expectedIPAMClaims) > 0 { ipamClaimList := &ipamclaimsapi.IPAMClaimList{} - Expect(mgr.GetClient().List(context.Background(), ipamClaimList, &client.ListOptions{ - Namespace: config.inputVMI.Namespace, - })).To(Succeed()) - Expect(ipamClaimsSpecExtractor(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims)) + + Expect(mgr.GetClient().List(context.Background(), ipamClaimList, ownedByVMLabel(vmName))).To(Succeed()) + Expect(ipamClaimsCleaner(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims)) } }, Entry("when the VM has an associated VMI pointing to an existing NAD", testConfig{ @@ -132,9 +139,16 @@ var _ = Describe("vm IPAM controller", Serial, func() { inputVMI: dummyVMI(nadName), inputNAD: dummyNAD(nadName), expectedResponse: reconcile.Result{}, - expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{ + expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { - Network: "goodnet", + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), + Namespace: namespace, + Finalizers: []string{kubevirtVMFinalizer}, + Labels: ownedByVMLabel(vmName), + OwnerReferences: []metav1.OwnerReference{{Name: vmName}}, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"}, }, }, }), @@ -162,17 +176,27 @@ var _ = Describe("vm IPAM controller", Serial, func() { }, }), Entry("the VMI does not exist on the datastore - it might have been deleted in the meantime", testConfig{ - expectedError: &errors.StatusError{ - ErrStatus: metav1.Status{ - Status: "Failure", - Message: "virtualmachineinstances.kubevirt.io \"\" not found", // no name printed since we're not passing a VMI - Reason: "NotFound", - Details: &metav1.StatusDetails{ - Name: "", - Group: "kubevirt.io", - Kind: "virtualmachineinstances", + expectedResponse: reconcile.Result{}, + }), + Entry("the VMI was deleted, thus the existing IPAMClaims finalizers must be removed", testConfig{ + expectedResponse: reconcile.Result{}, + existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), + Namespace: namespace, + Finalizers: []string{kubevirtVMFinalizer}, + Labels: ownedByVMLabel(vmName), + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"}, + }, + expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vm1.randomnet", + Namespace: "ns1", + Labels: ownedByVMLabel(vmName), }, - Code: 404, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"}, }, }, }), @@ -205,13 +229,29 @@ var _ = Describe("vm IPAM controller", Serial, func() { UID: dummyUID, }, }, + Labels: ownedByVMLabel(vmName), + Finalizers: []string{kubevirtVMFinalizer}, }, Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"}, }, expectedResponse: reconcile.Result{}, - expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{ + expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { - Network: "doesitmatter?", + ObjectMeta: metav1.ObjectMeta{ + Name: "vm1.randomnet", + Namespace: "ns1", + Labels: ownedByVMLabel(vmName), + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "virtualmachines", + Name: "vm1", + UID: dummyUID, + }, + }, + Finalizers: []string{kubevirtVMFinalizer}, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"}, }, }, }), @@ -219,9 +259,16 @@ var _ = Describe("vm IPAM controller", Serial, func() { inputVMI: dummyVMI(nadName), inputNAD: dummyNAD(nadName), expectedResponse: reconcile.Result{}, - expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{ + expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { - Network: "goodnet", + ObjectMeta: metav1.ObjectMeta{ + Name: "vm1.randomnet", + Namespace: "ns1", + Labels: ownedByVMLabel(vmName), + Finalizers: []string{kubevirtVMFinalizer}, + OwnerReferences: []metav1.OwnerReference{{Name: vmName}}, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"}, }, }, }), @@ -297,12 +344,11 @@ func dummyNADWrongFormat(nadName string) *nadv1.NetworkAttachmentDefinition { } } -func ipamClaimsSpecExtractor(ipamClaims ...ipamclaimsapi.IPAMClaim) []ipamclaimsapi.IPAMClaimSpec { - ipamClaimsSpec := make([]ipamclaimsapi.IPAMClaimSpec, len(ipamClaims)) +func ipamClaimsCleaner(ipamClaims ...ipamclaimsapi.IPAMClaim) []ipamclaimsapi.IPAMClaim { for i := range ipamClaims { - ipamClaimsSpec[i] = ipamClaims[i].Spec + ipamClaims[i].ObjectMeta.ResourceVersion = "" } - return ipamClaimsSpec + return ipamClaims } func decorateVMWithUID(uid string, vm *virtv1.VirtualMachine) *virtv1.VirtualMachine { From 1e76e8f2729e1c342cca7c7576a9123c0b205eda Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Fri, 17 May 2024 16:33:31 +0200 Subject: [PATCH 5/5] deploy: allow the controller to update IPAMClaims Signed-off-by: Miguel Duarte Barroso --- config/rbac/role.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 8627a972..a2335492 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -26,4 +26,4 @@ rules: - apiGroups: ["k8s.cni.cncf.io"] resources: - ipamclaims - verbs: [ "create" ] + verbs: [ "create", "update" ]