Merge pull request #12 from maiqueb/add-finalizer-to-ipam-claim

Add finalizer to ipam claim
kubevirt · May 17, 2024 · 06b38f4 · 06b38f4
2 parents 2f88cd9 + 1e76e8f
commit 06b38f4
Show file tree

Hide file tree

Showing 3 changed files with 194 additions and 100 deletions.
diff --git 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" ]
diff --git 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"
 
@@ -26,6 +27,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
@@ -49,18 +52,22 @@ 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.
-		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
 	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 {
@@ -73,8 +80,91 @@ func (r *VirtualMachineReconciler) Reconcile(
 		)
 	}
 
-	vmiSpec := vmi.Spec
-	for _, net := range vmiSpec.Networks {
+	vmiNetworks, err := r.vmiNetworksClaimingIPAM(ctx, vmi)
+	if err != nil {
+		return controllerruntime.Result{}, err
+	}
+
+	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},
+				Labels:          ownedByVMLabel(vmi.Name),
+			},
+			Spec: ipamclaimsapi.IPAMClaimSpec{
+				Network: netConfigName,
+			},
+		}
+
+		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
+				}
+			}
+			r.Log.Error(err, "failed to create the IPAMClaim")
+			return controllerruntime.Result{}, err
+		}
+	}
+
+	return controllerruntime.Result{}, nil
+}
+
+// Setup sets up the controller with the Manager passed in the constructor.
+func (r *VirtualMachineReconciler) Setup() error {
+	return controllerruntime.NewControllerManagedBy(r.manager).
+		For(&virtv1.VirtualMachineInstance{}).
+		WithEventFilter(onVMPredicates()).
+		Complete(r)
+}
+
+func onVMPredicates() predicate.Funcs {
+	return predicate.Funcs{
+		CreateFunc: func(createEvent event.CreateEvent) bool {
+			return true
+		},
+		DeleteFunc: func(event.DeleteEvent) bool {
+			return true
+		},
+		UpdateFunc: func(updateEvent event.UpdateEvent) bool {
+			return false
+		},
+		GenericFunc: func(event.GenericEvent) bool {
+			return false
+		},
+	}
+}
+
+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
 		}
@@ -88,94 +178,52 @@ func (r *VirtualMachineReconciler) Reconcile(
 				nadName = namespaceAndName[1]
 			}
 
-			ctx, cancel := context.WithTimeout(ctx, time.Second)
+			contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second)
 			defer cancel()
 			nad := &nadv1.NetworkAttachmentDefinition{}
 			if err := r.Client.Get(
-				ctx,
+				contextWithTimeout,
 				apitypes.NamespacedName{Namespace: namespace, Name: nadName},
 				nad,
 			); err != nil {
 				if apierrors.IsNotFound(err) {
-					return controllerruntime.Result{}, 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 controllerruntime.Result{}, fmt.Errorf("failed to extract the relevant NAD information")
+				return nil, fmt.Errorf("failed to extract the relevant NAD information")
 			}
 
 			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},
-					},
-					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,
-						}
-
-						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
-						}
-					}
-					r.Log.Error(err, "failed to create the IPAMClaim")
-					return controllerruntime.Result{}, err
-				}
+				vmiNets[net.Name] = nadConfig.Name
 			}
 		}
 	}
-
-	return controllerruntime.Result{}, nil
+	return vmiNets, nil
 }
 
-// Setup sets up the controller with the Manager passed in the constructor.
-func (r *VirtualMachineReconciler) Setup() error {
-	return controllerruntime.NewControllerManagedBy(r.manager).
-		For(&virtv1.VirtualMachineInstance{}).
-		WithEventFilter(onVMPredicates()).
-		Complete(r)
+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 onVMPredicates() predicate.Funcs {
-	return predicate.Funcs{
-		CreateFunc: func(createEvent event.CreateEvent) bool {
-			return true
-		},
-		DeleteFunc: func(event.DeleteEvent) bool {
-			return false
-		},
-		UpdateFunc: func(updateEvent event.UpdateEvent) bool {
-			return false
-		},
-		GenericFunc: func(event.GenericEvent) bool {
-			return false
-		},
+func ownedByVMLabel(vmiName string) client.MatchingLabels {
+	return map[string]string{
+		virtv1.VirtualMachineLabel: vmiName,
 	}
 }