Merge pull request #102 from razo7/no-va-deletion

Remove volumeAttachment deletion

controllers/fenceagentsremediation_controller.go

@@ -25,6 +25,7 @@ import (
 	"github.com/go-logr/logr"
 	commonAnnotations "github.com/medik8s/common/pkg/annotations"
 	commonConditions "github.com/medik8s/common/pkg/conditions"
+	commonResources "github.com/medik8s/common/pkg/resources"
 
 	apiErrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -224,7 +225,7 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct
 		!meta.IsStatusConditionTrue(far.Status.Conditions, commonConditions.SucceededType) {
 		// Fence agent action succeeded, and now we try to remove workloads (pods and their volume attachments)
 		r.Log.Info("Manual workload deletion", "Fence Agent", far.Spec.Agent, "Node Name", req.Name)
-		if err := utils.DeleteResources(ctx, r.Client, req.Name); err != nil {
+		if err := commonResources.DeletePods(ctx, r.Client, req.Name); err != nil {
 			r.Log.Error(err, "Manual workload deletion has failed", "CR's Name", req.Name)
 			return emptyResult, err
 		}

controllers/fenceagentsremediation_controller_test.go

@@ -162,9 +162,7 @@ var _ = Describe("FAR Controller", func() {
 				By("Having a finalizer if we have a remediation taint")
 				Expect(controllerutil.ContainsFinalizer(underTestFAR, v1alpha1.FARFinalizer)).To(BeTrue())
 
-				By("Not having any VAs nor the test pod")
-				testVADeletion(vaName1, resourceDeletionWasTriggered)
-				testVADeletion(vaName2, resourceDeletionWasTriggered)
+				By("Not having any test pod")
 				testPodDeletion(testPodName, resourceDeletionWasTriggered)
 
 				By("Verifying correct conditions for successfull remediation")
@@ -195,10 +193,8 @@ var _ = Describe("FAR Controller", func() {
 				By("Not having remediation taint")
 				Expect(utils.TaintExists(node.Spec.Taints, &farNoExecuteTaint)).To(BeFalse())
 
-				By("Still having all the VAs and one test pod")
+				By("Still having one test pod")
 				resourceDeletionWasTriggered = false
-				testVADeletion(vaName1, resourceDeletionWasTriggered)
-				testVADeletion(vaName2, resourceDeletionWasTriggered)
 				testPodDeletion(testPodName, resourceDeletionWasTriggered)
 
 				By("Verifying correct conditions for unsuccessfull remediation")
@@ -314,34 +310,6 @@ func cliCommandsEquality(far *v1alpha1.FenceAgentsRemediation) (bool, error) {
 	return isEqualStringLists(mocksExecuter.command, expectedCommand), nil
 }
 
-// TODO: Think about using Generics for the next two functions
-
-// testVADeletion tests whether the volume attachment no longer exist for successful FAR CR
-// and consistently check if the volume attachment exist and was not deleted
-func testVADeletion(vaName string, resourceDeletionWasTriggered bool) {
-	vaKey := client.ObjectKey{
-		Namespace: defaultNamespace,
-		Name:      vaName,
-	}
-	if resourceDeletionWasTriggered {
-		EventuallyWithOffset(1, func() bool {
-			va := &storagev1.VolumeAttachment{}
-			err := k8sClient.Get(context.Background(), vaKey, va)
-			return apierrors.IsNotFound(err)
-
-		}, timeoutDeletion, pollInterval).Should(BeTrue())
-		log.Info("Volume attachment is no longer exist", "va", vaName)
-	} else {
-		ConsistentlyWithOffset(1, func() bool {
-			va := &storagev1.VolumeAttachment{}
-			err := k8sClient.Get(context.Background(), vaKey, va)
-			return apierrors.IsNotFound(err)
-
-		}, timeoutDeletion, pollInterval).Should(BeFalse())
-		log.Info("Volume attachment exist", "va", vaName)
-	}
-}
-
 // testPodDeletion tests whether the pod no longer exist for successful FAR CR
 // and consistently check if the pod exist and was not deleted
 func testPodDeletion(podName string, resourceDeletionWasTriggered bool) {

go.mod

@@ -4,7 +4,7 @@ go 1.20
 
 require (
 	github.com/go-logr/logr v1.2.4
-	github.com/medik8s/common v1.7.0
+	github.com/medik8s/common v1.9.0
 	github.com/onsi/ginkgo/v2 v2.11.0
 	github.com/onsi/gomega v1.27.10
 	github.com/openshift/api v0.0.0-20230621174358-ea40115b9fa6

go.sum

@@ -76,8 +76,8 @@ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0
 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
 github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 h1:jWpvCLoY8Z/e3VKvlsiIGKtc+UG6U5vzxaoagmhXfyg=
 github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0/go.mod h1:QUyp042oQthUoa9bqDv0ER0wrtXnBruoNd7aNjkbP+k=
-github.com/medik8s/common v1.7.0 h1:JwimhWigPTAszGG2jYJmh+uVHq2nFUPRRljw7ZhlQhg=
-github.com/medik8s/common v1.7.0/go.mod h1:ZT/3hfMXJLmZEcqmxRWB5LGC8Wl+qKGGQ4zM8hOE7PY=
+github.com/medik8s/common v1.9.0 h1:nMMffmj+e0l0xRB0RfpsbdDpW9upXU2MFeGGADJlwyE=
+github.com/medik8s/common v1.9.0/go.mod h1:ZT/3hfMXJLmZEcqmxRWB5LGC8Wl+qKGGQ4zM8hOE7PY=
 github.com/moby/spdystream v0.2.0 h1:cjW1zVyyoiM0T7b6UoySUFqzXMoqRckQtXwGPiBhOM8=
 github.com/moby/spdystream v0.2.0/go.mod h1:f7i0iNDQJ059oMTcWxx8MA/zKFIuD/lY+0GqbN2Wy8c=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=

test/e2e/far_e2e_test.go

@@ -12,7 +12,6 @@ import (
 	. "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
-	storagev1 "k8s.io/api/storage/v1"
 	apiErrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -35,7 +34,6 @@ const (
 	nodeIdentifierPrefixAWS  = "--plug"
 	nodeIdentifierPrefixIPMI = "--ipport"
 	containerName            = "manager"
-	testVolumeAttachment     = "test-va"
 	testContainerName        = "test-container"
 	testPodName              = "test-pod"
 
@@ -83,7 +81,6 @@ var _ = Describe("FAR E2e", func() {
 		var (
 			nodes, filteredNodes                *corev1.NodeList
 			nodeName                            string
-			va                                  *storagev1.VolumeAttachment
 			pod                                 *corev1.Pod
 			creationTimePod, nodeBootTimeBefore time.Time
 			err                                 error
@@ -127,8 +124,7 @@ var _ = Describe("FAR E2e", func() {
 			Expect(k8sClient.Create(context.Background(), pod)).To(Succeed())
 			log.Info("Tested pod has been created", "pod", testPodName)
 			creationTimePod = metav1.Now().Time
-			va = createVA(nodeName)
-			DeferCleanup(cleanupTestedResources, va, pod)
+			DeferCleanup(cleanupTestedResources, pod)
 
 			// set the node as "unhealthy" by disabling kubelet
 			makeNodeUnready(selectedNode)
@@ -138,10 +134,10 @@ var _ = Describe("FAR E2e", func() {
 		})
 		When("running FAR to reboot two nodes", func() {
 			It("should successfully remediate the first node", func() {
-				checkRemediation(nodeName, nodeBootTimeBefore, creationTimePod, va, pod)
+				checkRemediation(nodeName, nodeBootTimeBefore, creationTimePod, pod)
 			})
 			It("should successfully remediate the second node", func() {
-				checkRemediation(nodeName, nodeBootTimeBefore, creationTimePod, va, pod)
+				checkRemediation(nodeName, nodeBootTimeBefore, creationTimePod, pod)
 			})
 		})
 	})
@@ -243,28 +239,6 @@ func randomizeWorkerNode(nodes *corev1.NodeList) *corev1.Node {
 	return &nodes.Items[r.Intn(len(nodes.Items))]
 }
 
-// createVA creates dummy volume attachment for testing the resource deletion
-func createVA(nodeName string) *storagev1.VolumeAttachment {
-	pv := "test-pv"
-	va := &storagev1.VolumeAttachment{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      testVolumeAttachment,
-			Namespace: testNsName,
-		},
-		Spec: storagev1.VolumeAttachmentSpec{
-			Attacher: pv,
-			Source: storagev1.VolumeAttachmentSource{
-				PersistentVolumeName: &pv,
-			},
-			NodeName: nodeName,
-		},
-	}
-
-	ExpectWithOffset(1, k8sClient.Create(context.Background(), va)).To(Succeed())
-	log.Info("Volume attachment has been created", "va", va.Name)
-	return va
-}
-
 // createFAR assigns the input to FenceAgentsRemediation object, creates CR, and returns the CR object
 func createFAR(nodeName string, agent string, sharedParameters map[v1alpha1.ParameterName]string, nodeParameters map[v1alpha1.ParameterName]map[v1alpha1.NodeName]string) *v1alpha1.FenceAgentsRemediation {
 	far := &v1alpha1.FenceAgentsRemediation{
@@ -291,13 +265,7 @@ func deleteFAR(far *v1alpha1.FenceAgentsRemediation) {
 }
 
 // cleanupTestedResources deletes an old pod and old va if it was not deleted from FAR CR
-func cleanupTestedResources(va *storagev1.VolumeAttachment, pod *corev1.Pod) {
-	newVa := &storagev1.VolumeAttachment{}
-	if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(va), newVa); err == nil {
-		Expect(k8sClient.Delete(context.Background(), newVa)).To(Succeed())
-		log.Info("cleanup: Volume attachment has not been deleted by remediation", "va name", va.Name)
-	}
-
+func cleanupTestedResources(pod *corev1.Pod) {
 	newPod := &corev1.Pod{}
 	if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(pod), newPod); err == nil {
 		Expect(k8sClient.Delete(context.Background(), newPod)).To(Succeed())
@@ -403,17 +371,6 @@ func wasNodeRebooted(nodeName string, nodeBootTimeBefore time.Time) {
 	log.Info("successful reboot", "node", nodeName, "offset between last boot", nodeBootTimeAfter.Sub(nodeBootTimeBefore), "new boot time", nodeBootTimeAfter)
 }
 
-// checkVaDeleted verifies if the va has already been deleted due to resource deletion
-func checkVaDeleted(va *storagev1.VolumeAttachment) {
-	EventuallyWithOffset(1, func() bool {
-		newVa := &storagev1.VolumeAttachment{}
-		err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(va), newVa)
-		return apiErrors.IsNotFound(err)
-
-	}, timeoutDeletion, pollDeletion).Should(BeTrue())
-	log.Info("Volume Attachment has already been deleted", "va name", va.Name)
-}
-
 // checkPodDeleted vefifies if the pod has already been deleted due to resource deletion
 func checkPodDeleted(pod *corev1.Pod) {
 	ConsistentlyWithOffset(1, func() bool {
@@ -441,7 +398,7 @@ func verifyStatusCondition(nodeName, conditionType string, conditionStatus *meta
 }
 
 // checkRemediation verify whether the node was remediated
-func checkRemediation(nodeName string, nodeBootTimeBefore time.Time, oldPodCreationTime time.Time, va *storagev1.VolumeAttachment, pod *corev1.Pod) {
+func checkRemediation(nodeName string, nodeBootTimeBefore time.Time, oldPodCreationTime time.Time, pod *corev1.Pod) {
 	By("Check if FAR NoExecute taint was added")
 	wasFarTaintAdded(nodeName)
 
@@ -452,9 +409,6 @@ func checkRemediation(nodeName string, nodeBootTimeBefore time.Time, oldPodCreat
 	By("Getting new node's boot time")
 	wasNodeRebooted(nodeName, nodeBootTimeBefore)
 
-	By("checking if old VA has been deleted")
-	checkVaDeleted(va)
-
 	By("checking if old pod has been deleted")
 	checkPodDeleted(pod)
 

vendor/modules.txt

@@ -92,11 +92,12 @@ github.com/mailru/easyjson/jwriter
 # github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0
 ## explicit; go 1.19
 github.com/matttproud/golang_protobuf_extensions/v2/pbutil
-# github.com/medik8s/common v1.7.0
+# github.com/medik8s/common v1.9.0
 ## explicit; go 1.20
 github.com/medik8s/common/pkg/annotations
 github.com/medik8s/common/pkg/conditions
 github.com/medik8s/common/pkg/labels
+github.com/medik8s/common/pkg/resources
 # github.com/moby/spdystream v0.2.0
 ## explicit; go 1.13
 github.com/moby/spdystream