diff --git a/pkg/vmnetworkscontroller/controller.go b/pkg/vmnetworkscontroller/controller.go index f17304a5..8cb46663 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(ctx, 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 @@ -88,9 +91,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, @@ -145,7 +146,7 @@ func onVMPredicates() predicate.Funcs { return true }, DeleteFunc: func(event.DeleteEvent) bool { - return false + return true }, UpdateFunc: func(updateEvent event.UpdateEvent) bool { return false @@ -201,3 +202,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 {