Repository: kubernetes-sigs/cluster-api-provider-kubevirt (go)

Issue:
Kubevirt cluster and machine controllers shouldn't report patch errors when resources already deleted
**What steps did you take and what happened:**

1. create a cluster and wait for it to be created
1. delete the cluster
1. Looking at the logs of the `capk-controller-manager-<hash>-<hash>` manager container, the following errors might be reported (unless the deletion took longer to delete the resource, but this showing fairly consistently for me):

   ```plain
   (...)
   E0720 21:36:17.931136       1 controller.go:317] controller/kubevirtmachine "msg"="Reconciler error" "error"="failed to patch KubevirtMachine: kubevirtmachines.infrastructure.cluster.x-k8s.io \"pja-multi-control-plane-qnrhm\" not found" "name"="pja-multi-control-plane-qnrhm" "namespace"="capk" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="KubevirtMachine"
   (...)
   E0720 21:36:23.236566       1 controller.go:317] controller/kubevirtcluster "msg"="Reconciler error" "error"="kubevirtclusters.infrastructure.cluster.x-k8s.io \"pja-multi\" not found" "name"="pja-multi" "namespace"="capk" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="KubevirtCluster"
   ```

Looking at the code, this is due to calls to the patch the resources that might occur after the resources have been deleted and the error handling doesn't differentiate between `NotFound` and other errors. 

* For `KubevirtCluster`, this is due to the [deferred call to the patch function](https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/blob/41c77a793ccbcaa21a05d9b76fd2d4f02c4934f5/controllers/kubevirtcluster_controller.go#L116) that is called even when the reconciliation result is the deletion of the resource.
* For `KubevirtMachine`, this is due to the call to the [patch function](https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/blob/41c77a793ccbcaa21a05d9b76fd2d4f02c4934f5/controllers/kubevirtmachine_controller.go#L439) at the end of the `reconcileDelete`.

**What did you expect to happen:**

Calling the patches are still valid as the deletion might actually take longer to be done by K8s, but, in the case where the latter was quick and the resources are not longer found, then either the not-found errors should be silenced, or they should be reported at a lower level (info) with the indication that this is an expected case.


**Environment:**

- Cluster-api version: **v1beta1**
- Cluster-api-provider-kubevirt version: **v1alpha1**
- Kubernetes version: (use `kubectl version`): **v1.20.15**
- KubeVirt version: **v1**
- OS (e.g. from `/etc/os-release`): **CentOS 7**

/kind bug


Pull request:
Avoid reporting not-found errors when patching resources under deletion.
**What this PR does / why we need it**:

When reconciling for deletion, the KubevirtCluster and KubevirtMachine controllers both attempt to patch, as the last step of the reconciliation, the resource in order to indicate to the user that it is being deleted. It is possible that, by the time these patches are called, the resource is already deleted; which result of the patch failing with a `NotFound` error that is then reported at the error level in the log.

Because this is an expected case, this change silences `NotFound` errors raised by these two final patches. 

**Which issue this PR fixes**:

fixes #152

**Special notes for your reviewer**:

Note that the patch methods return an `k8s.io/apimachinery/pkg/util/errors#Aggregate` error in this case; which will not be recognized by the `apierrors.IsNotFound` method. So, I am extracting the error using the `Reduce` method to extract the aggregated error, if there is only one and the error passed to the method is an aggregate.

Note also that, for the KubevirtCluster controller, the change will also apply when a normal reconciliation happens. I believe that this is acceptable as the existence of the resource has been access before and this change should not obfuscate unexpected failures.

**Release notes**:

```release-note
NONE
```


Patch:
diff --git a/controllers/kubevirtcluster_controller.go b/controllers/kubevirtcluster_controller.go
index 8877571..0ce3a1e 100644
--- a/controllers/kubevirtcluster_controller.go
+++ b/controllers/kubevirtcluster_controller.go
@@ -25,6 +25,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -115,9 +116,11 @@ func (r *KubevirtClusterReconciler) Reconcile(goctx gocontext.Context, req ctrl.
 	// Always attempt to Patch the KubevirtCluster object and status after each reconciliation.
 	defer func() {
 		if err := clusterContext.PatchKubevirtCluster(patchHelper); err != nil {
-			clusterContext.Logger.Error(err, "failed to patch KubevirtCluster")
-			if rerr == nil {
-				rerr = err
+			if !apierrors.IsNotFound(utilerrors.Reduce(err)) {
+				clusterContext.Logger.Error(err, "failed to patch KubevirtCluster")
+				if rerr == nil {
+					rerr = err
+				}
 			}
 		}
 	}()
diff --git a/controllers/kubevirtmachine_controller.go b/controllers/kubevirtmachine_controller.go
index 8315d45..b51c7b6 100644
--- a/controllers/kubevirtmachine_controller.go
+++ b/controllers/kubevirtmachine_controller.go
@@ -28,6 +28,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	kubevirtv1 "kubevirt.io/api/core/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	capierrors "sigs.k8s.io/cluster-api/errors"
@@ -424,6 +425,7 @@ func (r *KubevirtMachineReconciler) reconcileDelete(ctx *context.MachineContext)
 	if err != nil {
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, errors.Wrap(err, "failed to create helper for externalMachine access")
 	}
+
 	if externalMachine.Exists() {
 		if err := externalMachine.Delete(); err != nil {
 			return ctrl.Result{RequeueAfter: 10 * time.Second}, errors.Wrap(err, "failed to delete VM")
@@ -433,11 +435,13 @@ func (r *KubevirtMachineReconciler) reconcileDelete(ctx *context.MachineContext)
 	// Machine is deleted so remove the finalizer.
 	controllerutil.RemoveFinalizer(ctx.KubevirtMachine, infrav1.MachineFinalizer)
 
-	// Set the VMProvisionedCondition reporting delete is started, and issue a patch in order to make
-	// this visible to the users.
+	// Set the VMProvisionedCondition reporting delete is started, and attempt to issue a patch in
+	// order to make this visible to the users.
 	conditions.MarkFalse(ctx.KubevirtMachine, infrav1.VMProvisionedCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
 	if err := ctx.PatchKubevirtMachine(patchHelper); err != nil {
-		return ctrl.Result{}, errors.Wrap(err, "failed to patch KubevirtMachine")
+		if !apierrors.IsNotFound(utilerrors.Reduce(err)) {
+			return ctrl.Result{}, errors.Wrap(err, "failed to patch KubevirtMachine")
+		}
 	}
 
 	return ctrl.Result{}, nil


Test patch:
diff --git a/controllers/kubevirtcluster_controller_test.go b/controllers/kubevirtcluster_controller_test.go
index d9ba1c9..8f7906c 100644
--- a/controllers/kubevirtcluster_controller_test.go
+++ b/controllers/kubevirtcluster_controller_test.go
@@ -153,7 +153,7 @@ var _ = Describe("Reconcile", func() {
 
 		AfterEach(func() {})
 
-		It("should throw an error when reconciling deleted clusters.", func() {
+		It("should succeed with the kubevirt cluster being deleted.", func() {
 			objects := []client.Object{
 				cluster,
 				kubevirtCluster,
@@ -161,13 +161,20 @@ var _ = Describe("Reconcile", func() {
 			setupClient(objects)
 			infraClusterMock.EXPECT().GenerateInfraClusterClient(gomock.Any(), gomock.Any(), gomock.Any()).Return(fakeClient, kubevirtCluster.Namespace, nil)
 
-			_, err := kubevirtClusterReconciler.Reconcile(fakeContext, Request{
-				NamespacedName: client.ObjectKey{
-					Namespace: kubevirtCluster.Namespace,
-					Name:      kubevirtCluster.Name,
-				},
+			namespacedName := client.ObjectKey{
+				Namespace: kubevirtCluster.Namespace,
+				Name:      kubevirtCluster.Name,
+			}
+
+			result, err := kubevirtClusterReconciler.Reconcile(fakeContext, Request{
+				NamespacedName: namespacedName,
 			})
-			//test-kubevirt-cluster not found.
+			Expect(err).ShouldNot(HaveOccurred())
+			Expect(result.RequeueAfter).To(BeZero())
+			Expect(result.Requeue).To(BeFalse())
+
+			kvc := &infrav1.KubevirtCluster{}
+			err = fakeClient.Get(fakeContext, namespacedName, kvc)
 			Expect(err).Should(HaveOccurred())
 		})
 	})