From 783daa5355330549c6348ee7224c38fe235dd463 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Sun, 5 Nov 2023 23:06:48 +0530 Subject: [PATCH] addressing comments --- api/v1/aerospikecluster_types.go | 2 +- .../asdb.aerospike.com_aerospikeclusters.yaml | 16 +++-- controllers/pvc.go | 2 +- test/drain_test.go | 68 +++++++++++++++---- 4 files changed, 68 insertions(+), 20 deletions(-) diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index 838aed736..9c1b23712 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -555,7 +555,7 @@ type AerospikeStorageSpec struct { //nolint:govet // for readability // CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. CleanupThreads int `json:"cleanupThreads,omitempty"` - // CleanLocalPVC is a flag which allows operator to delete local PVC in case of kubernetes node crash + // CleanLocalPVC is a flag which allows operator to delete local PVC in case of kubernetes node (drain or delete) CleanLocalPVC bool `json:"cleanLocalPVC,omitempty"` // LocalStorageClasses contains list of storage classes which provisions local volumes. diff --git a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml index 0c205a310..9229ad628 100644 --- a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml +++ b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml @@ -5655,7 +5655,8 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator - to delete local PVC in case of kubernetes node crash + to delete local PVC in case of kubernetes node (drain + or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number @@ -7353,7 +7354,8 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator - to delete local PVC in case of kubernetes node crash + to delete local PVC in case of kubernetes node (drain + or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number @@ -8067,7 +8069,7 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator to - delete local PVC in case of kubernetes node crash + delete local PVC in case of kubernetes node (drain or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup @@ -14362,7 +14364,8 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator - to delete local PVC in case of kubernetes node crash + to delete local PVC in case of kubernetes node (drain + or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number @@ -16060,7 +16063,8 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator - to delete local PVC in case of kubernetes node crash + to delete local PVC in case of kubernetes node (drain + or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number @@ -16823,7 +16827,7 @@ spec: type: object cleanLocalPVC: description: CleanLocalPVC is a flag which allows operator to - delete local PVC in case of kubernetes node crash + delete local PVC in case of kubernetes node (drain or delete) type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup diff --git a/controllers/pvc.go b/controllers/pvc.go index faff82a2b..f1b15da13 100644 --- a/controllers/pvc.go +++ b/controllers/pvc.go @@ -118,7 +118,7 @@ func (r *SingleClusterReconciler) deleteLocalPVCs(pod *corev1.Pod, localStorageC for idx := range pvcItems { pvcStorageClass := pvcItems[idx].Spec.StorageClassName if pvcStorageClass == nil { - r.Log.Info("PVC does not have storageclass set, no need to delete PVC", pvcItems[idx].Name) + r.Log.Info("PVC does not have storageClass set, no need to delete PVC", "pvcName", pvcItems[idx].Name) continue } diff --git a/test/drain_test.go b/test/drain_test.go index 76375d66e..904b58bb0 100644 --- a/test/drain_test.go +++ b/test/drain_test.go @@ -10,6 +10,9 @@ import ( "golang.org/x/net/context" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/api/v1" ) var _ = Describe( @@ -21,12 +24,26 @@ var _ = Describe( clusterNamespacedName := getNamespacedName( clusterName, namespace, ) - aeroCluster := createDummyAerospikeCluster( - clusterNamespacedName, 2, + + AfterEach( + func() { + aeroCluster := &asdbv1.AerospikeCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterNamespacedName.Name, + Namespace: clusterNamespacedName.Namespace, + }, + } + err := deleteCluster(k8sClient, ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + }, ) It( + "Should delete local PVC if pod is unschedulable", func() { + aeroCluster := createDummyAerospikeCluster( + clusterNamespacedName, 2, + ) aeroCluster.Spec.Storage.LocalStorageClasses = []string{storageClass} aeroCluster.Spec.Storage.CleanLocalPVC = true err := deployCluster(k8sClient, ctx, aeroCluster) @@ -40,10 +57,33 @@ var _ = Describe( err = updateClusterWithTO(k8sClient, ctx, aeroCluster, 1*time.Minute) Expect(err).To(HaveOccurred()) - err = checkPVCStatus(ctx, "ns-"+clusterName+"-0-1") + pvcDeleted := false + pvcDeleted, err = isPVCDeleted(ctx, "ns-"+clusterName+"-0-1") + Expect(err).ToNot(HaveOccurred()) + Expect(pvcDeleted).To(Equal(true)) + }, + ) + It( + "Should not delete local PVC if cleanLocalPVC flag is not set", func() { + aeroCluster := createDummyAerospikeCluster( + clusterNamespacedName, 2, + ) + aeroCluster.Spec.Storage.LocalStorageClasses = []string{storageClass} + err := deployCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - _ = deleteCluster(k8sClient, ctx, aeroCluster) + aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + aeroCluster.Spec.PodSpec.AerospikeContainerSpec.Resources = unschedulableResource() + + err = updateClusterWithTO(k8sClient, ctx, aeroCluster, 1*time.Minute) + Expect(err).To(HaveOccurred()) + + pvcDeleted := false + pvcDeleted, err = isPVCDeleted(ctx, "ns-"+clusterName+"-0-1") + Expect(err).ToNot(HaveOccurred()) + Expect(pvcDeleted).To(Equal(false)) }, ) }, @@ -51,7 +91,7 @@ var _ = Describe( }, ) -func checkPVCStatus(ctx context.Context, pvcName string) error { +func isPVCDeleted(ctx context.Context, pvcName string) (bool, error) { pvc := &corev1.PersistentVolumeClaim{} pvcNamespacesName := getNamespacedName( pvcName, namespace, @@ -59,16 +99,20 @@ func checkPVCStatus(ctx context.Context, pvcName string) error { err := k8sClient.Get(ctx, pvcNamespacesName, pvc) if err != nil { - if !errors.IsNotFound(err) { - return err + if errors.IsNotFound(err) { + return true, nil } - return nil + return false, err } - if pvc.Status.Phase == "Pending" { - return nil + //nolint:exhaustive // rest of the cases handled by default + switch pvc.Status.Phase { + case corev1.ClaimPending: + return true, nil + case corev1.ClaimBound: + return false, nil + default: + return false, fmt.Errorf("PVC status is not expected %s", pvc.Name) } - - return fmt.Errorf("PVC status is not expected %s", pvc.Name) }