Merge pull request #40 from oshoval/fin

Fix Finalizer logic

cmd/main.go

@@ -46,6 +46,7 @@ import (
 
 	"github.com/kubevirt/ipam-extensions/pkg/ipamclaimswebhook"
 	"github.com/kubevirt/ipam-extensions/pkg/vminetworkscontroller"
+	"github.com/kubevirt/ipam-extensions/pkg/vmnetworkscontroller"
 	//+kubebuilder:scaffold:imports
 )
 
@@ -156,6 +157,11 @@ func main() {
 		os.Exit(1)
 	}
 
+	if err = vmnetworkscontroller.NewVMReconciler(mgr).Setup(); err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", "VirtualMachine")
+		os.Exit(1)
+	}
+
 	if err = vminetworkscontroller.NewVMIReconciler(mgr).Setup(); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "VirtualMachineInstance")
 		os.Exit(1)

pkg/claims/claims.go

@@ -0,0 +1,43 @@
+package claims
+
+import (
+	"context"
+	"fmt"
+
+	apitypes "k8s.io/apimachinery/pkg/types"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	ipamclaimsapi "github.com/k8snetworkplumbingwg/ipamclaims/pkg/crd/ipamclaims/v1alpha1"
+
+	virtv1 "kubevirt.io/api/core/v1"
+)
+
+const KubevirtVMFinalizer = "kubevirt.io/persistent-ipam"
+
+func Cleanup(c client.Client, vmiKey apitypes.NamespacedName) error {
+	ipamClaims := &ipamclaimsapi.IPAMClaimList{}
+	listOpts := []client.ListOption{
+		client.InNamespace(vmiKey.Namespace),
+		OwnedByVMLabel(vmiKey.Name),
+	}
+	if err := c.List(context.Background(), ipamClaims, listOpts...); err != nil {
+		return fmt.Errorf("could not get list of IPAMClaims owned by VM %q: %w", vmiKey.String(), err)
+	}
+
+	for _, claim := range ipamClaims.Items {
+		if controllerutil.RemoveFinalizer(&claim, KubevirtVMFinalizer) {
+			if err := c.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,
+	}
+}

pkg/vminetworkscontroller/vmi_controller.go

@@ -8,14 +8,15 @@ import (
 
 	"github.com/go-logr/logr"
 
+	"k8s.io/utils/ptr"
+
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	apitypes "k8s.io/apimachinery/pkg/types"
 
 	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"
 
@@ -24,11 +25,10 @@ import (
 
 	virtv1 "kubevirt.io/api/core/v1"
 
+	"github.com/kubevirt/ipam-extensions/pkg/claims"
 	"github.com/kubevirt/ipam-extensions/pkg/config"
 )
 
-const kubevirtVMFinalizer = "kubevirt.io/persistent-ipam"
-
 // VirtualMachineInstanceReconciler reconciles a VirtualMachineInstance object
 type VirtualMachineInstanceReconciler struct {
 	client.Client
@@ -56,46 +56,42 @@ func (r *VirtualMachineInstanceReconciler) Reconcile(
 	defer cancel()
 	err := r.Client.Get(contextWithTimeout, request.NamespacedName, vmi)
 	if apierrors.IsNotFound(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
+		vmi = nil
 	} else if err != nil {
 		return controllerruntime.Result{}, err
 	}
 
-	var ownerInfo metav1.OwnerReference
-	vm := &virtv1.VirtualMachine{}
-	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 {
-		ownerInfo = metav1.OwnerReference{APIVersion: vm.APIVersion, Kind: vm.Kind, Name: vm.Name, UID: vm.UID}
-	} else {
-		return controllerruntime.Result{}, fmt.Errorf(
-			"error to get VMI %q corresponding VM: %w",
-			request.NamespacedName,
-			err,
-		)
+	vm, err := getOwningVM(ctx, r.Client, request.NamespacedName)
+	if err != nil {
+		return controllerruntime.Result{}, err
+	}
+
+	if shouldCleanFinalizers(vmi, vm) {
+		if err := claims.Cleanup(r.Client, request.NamespacedName); err != nil {
+			return controllerruntime.Result{}, fmt.Errorf("failed removing the IPAMClaims finalizer: %w", err)
+		}
+		return controllerruntime.Result{}, nil
+	}
+
+	if vmi == nil {
+		return controllerruntime.Result{}, nil
 	}
 
 	vmiNetworks, err := r.vmiNetworksClaimingIPAM(ctx, vmi)
 	if err != nil {
 		return controllerruntime.Result{}, err
 	}
 
+	ownerInfo := ownerReferenceFor(vmi, vm)
 	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),
+				Finalizers:      []string{claims.KubevirtVMFinalizer},
+				Labels:          claims.OwnedByVMLabel(vmi.Name),
 			},
 			Spec: ipamclaimsapi.IPAMClaimSpec{
 				Network: netConfigName,
@@ -148,7 +144,7 @@ func onVMIPredicates() predicate.Funcs {
 			return true
 		},
 		UpdateFunc: func(updateEvent event.UpdateEvent) bool {
-			return false
+			return true
 		},
 		GenericFunc: func(event.GenericEvent) bool {
 			return false
@@ -202,29 +198,47 @@ func (r *VirtualMachineInstanceReconciler) vmiNetworksClaimingIPAM(
 	return vmiNets, nil
 }
 
-func (r *VirtualMachineInstanceReconciler) Cleanup(vmiKey apitypes.NamespacedName) error {
-	ipamClaims := &ipamclaimsapi.IPAMClaimList{}
-	listOpts := []client.ListOption{
-		client.InNamespace(vmiKey.Namespace),
-		ownedByVMLabel(vmiKey.Name),
+func shouldCleanFinalizers(vmi *virtv1.VirtualMachineInstance, vm *virtv1.VirtualMachine) bool {
+	if vm != nil {
+		// VMI is gone and VM is marked for deletion
+		return vmi == nil && vm.DeletionTimestamp != nil
+	} else {
+		// VMI is gone or VMI is marked for deletion, virt-launcher is gone
+		return vmi == nil || (vmi.DeletionTimestamp != nil && len(vmi.Status.ActivePods) == 0)
 	}
-	if err := r.Client.List(context.Background(), ipamClaims, listOpts...); err != nil {
-		return fmt.Errorf("could not get list of IPAMClaims owned by VM %q: %w", vmiKey.String(), err)
+}
+
+func ownerReferenceFor(vmi *virtv1.VirtualMachineInstance, vm *virtv1.VirtualMachine) metav1.OwnerReference {
+	var obj client.Object
+	if vm != nil {
+		obj = vm
+	} else {
+		obj = vmi
 	}
 
-	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)
-			}
-		}
+	aPIVersion := obj.GetObjectKind().GroupVersionKind().Group + "/" + obj.GetObjectKind().GroupVersionKind().Version
+	return metav1.OwnerReference{
+		APIVersion:         aPIVersion,
+		Kind:               obj.GetObjectKind().GroupVersionKind().Kind,
+		Name:               obj.GetName(),
+		UID:                obj.GetUID(),
+		Controller:         ptr.To(true),
+		BlockOwnerDeletion: ptr.To(true),
 	}
-	return nil
 }
 
-func ownedByVMLabel(vmiName string) client.MatchingLabels {
-	return map[string]string{
-		virtv1.VirtualMachineLabel: vmiName,
+// Gets the owning VM if any. for simplicity it just try to fetch the VM,
+// even when the VMI exists, instead of parsing ownerReferences and handling differently the nil VMI case.
+func getOwningVM(ctx context.Context, c client.Client, name apitypes.NamespacedName) (*virtv1.VirtualMachine, error) {
+	contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second)
+	defer cancel()
+
+	vm := &virtv1.VirtualMachine{}
+	if err := c.Get(contextWithTimeout, name, vm); err == nil {
+		return vm, nil
+	} else if apierrors.IsNotFound(err) {
+		return nil, nil
+	} else {
+		return nil, fmt.Errorf("failed getting VM %q: %w", name, err)
 	}
 }