From 12962e48b230bd4d4f1b95ccc27462d42bc4f158 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Fri, 3 Nov 2023 11:39:47 +0530 Subject: [PATCH] Removing persistentvolume dependency while cleaning local pvc. --- api/v1/aerospikecluster_types.go | 8 +- api/v1/zz_generated.deepcopy.go | 5 ++ .../asdb.aerospike.com_aerospikeclusters.yaml | 64 +++++++++++++++- config/rbac/role.yaml | 7 +- controllers/pod.go | 4 +- controllers/pvc.go | 24 ++---- controllers/statefulset.go | 33 ++------- ...erospike-operator-manager-clusterrole.yaml | 1 - pkg/utils/pod.go | 4 +- test/cluster_helper.go | 5 +- test/drain_test.go | 74 +++++++++++++++++++ 11 files changed, 166 insertions(+), 63 deletions(-) create mode 100644 test/drain_test.go diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index 673b6b962..838aed736 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -73,8 +73,6 @@ type AerospikeClusterSpec struct { //nolint:govet // for readability SeedsFinderServices SeedsFinderServices `json:"seedsFinderServices,omitempty"` // RosterNodeBlockList is a list of blocked nodeIDs from roster in a strong-consistency setup RosterNodeBlockList []string `json:"rosterNodeBlockList,omitempty"` - // CleanLocalPVC is a flag which allows operator to delete local PVC in case of kubernetes node crash - CleanLocalPVC bool `json:"cleanLocalPVC,omitempty"` } type SeedsFinderServices struct { @@ -557,6 +555,12 @@ 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 bool `json:"cleanLocalPVC,omitempty"` + + // LocalStorageClasses contains list of storage classes which provisions local volumes. + LocalStorageClasses []string `json:"localStorageClasses,omitempty"` + // Volumes list to attach to created pods. // +patchMergeKey=name // +patchStrategy=merge diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index a68fa7d32..9fd51cfb7 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -652,6 +652,11 @@ func (in *AerospikeStorageSpec) DeepCopyInto(out *AerospikeStorageSpec) { *out = *in in.FileSystemVolumePolicy.DeepCopyInto(&out.FileSystemVolumePolicy) in.BlockVolumePolicy.DeepCopyInto(&out.BlockVolumePolicy) + if in.LocalStorageClasses != nil { + in, out := &in.LocalStorageClasses, &out.LocalStorageClasses + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Volumes != nil { in, out := &in.Volumes, &out.Volumes *out = make([]VolumeSpec, len(*in)) diff --git a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml index b4bbce24c..0c205a310 100644 --- a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml +++ b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml @@ -278,10 +278,6 @@ spec: - customInterface type: string type: object - cleanLocalPVC: - description: CleanLocalPVC is a flag which allows operator to delete - local PVC in case of kubernetes node crash - type: boolean image: description: Aerospike server image type: string @@ -5657,6 +5653,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator + to delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -5713,6 +5713,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage + classes which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: @@ -7345,6 +7351,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator + to delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -7401,6 +7411,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage + classes which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: @@ -8049,6 +8065,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator to + delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -8105,6 +8125,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage classes + which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: @@ -14334,6 +14360,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator + to delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -14390,6 +14420,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage + classes which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: @@ -16022,6 +16058,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator + to delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -16078,6 +16118,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage + classes which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: @@ -16775,6 +16821,10 @@ spec: - deleteFiles type: string type: object + cleanLocalPVC: + description: CleanLocalPVC is a flag which allows operator to + delete local PVC in case of kubernetes node crash + type: boolean cleanupThreads: description: CleanupThreads contains maximum number of cleanup threads(dd or blkdiscard) per init container. @@ -16831,6 +16881,12 @@ spec: - deleteFiles type: string type: object + localStorageClasses: + description: LocalStorageClasses contains list of storage classes + which provisions local volumes. + items: + type: string + type: array volumes: description: Volumes list to attach to created pods. items: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index cee5326ca..515b566cd 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -79,6 +79,12 @@ rules: - patch - update - watch +- apiGroups: + - "" + resources: + - persistentvolumes + verbs: + - get - apiGroups: - "" resources: @@ -98,7 +104,6 @@ rules: - "" resources: - secrets - - persistentvolumes verbs: - get - apiGroups: diff --git a/controllers/pod.go b/controllers/pod.go index 8a3f9b836..0b2229c46 100644 --- a/controllers/pod.go +++ b/controllers/pod.go @@ -261,8 +261,8 @@ func (r *SingleClusterReconciler) restartPods( } } - if r.aeroCluster.Spec.CleanLocalPVC { - if err := r.deleteLocalPVCs(pod); err != nil { + if rackState.Rack.Storage.CleanLocalPVC { + if err := r.deleteLocalPVCs(pod, rackState.Rack.Storage.LocalStorageClasses); err != nil { return reconcileError(err) } } diff --git a/controllers/pvc.go b/controllers/pvc.go index 1508343c1..faff82a2b 100644 --- a/controllers/pvc.go +++ b/controllers/pvc.go @@ -6,9 +6,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/api/v1" @@ -105,7 +103,7 @@ func (r *SingleClusterReconciler) removePVCsAsync( return deletedPVCs, nil } -func (r *SingleClusterReconciler) deleteLocalPVCs(pod *corev1.Pod) error { +func (r *SingleClusterReconciler) deleteLocalPVCs(pod *corev1.Pod, localStorageClasses []string) error { if pod.Status.Phase == corev1.PodPending && utils.IsPodNeedsToMigrate(pod) { rackID, err := utils.GetRackIDFromPodName(pod.Name) if err != nil { @@ -118,26 +116,14 @@ func (r *SingleClusterReconciler) deleteLocalPVCs(pod *corev1.Pod) error { } for idx := range pvcItems { - pv := &corev1.PersistentVolume{} - - volumeName := pvcItems[idx].Spec.VolumeName - if volumeName == "" { - r.Log.Info("PVC is not bounded with any volume, no need to delete PVC", pvcItems[idx].Name) + 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) continue } - pvName := types.NamespacedName{Name: volumeName} - if err := r.Client.Get(context.TODO(), pvName, pv); err != nil { - if errors.IsNotFound(err) { - r.Log.Info("Volume bounded with PVC not found, no need to delete PVC", pvcItems[idx].Name) - continue - } - - return err - } - - if pv.Spec.Local != nil { + if utils.ContainsString(localStorageClasses, *pvcStorageClass) { if err := r.Client.Delete(context.TODO(), &pvcItems[idx]); err != nil { return fmt.Errorf( "could not delete pvc %s: %v", pvcItems[idx].Name, err, diff --git a/controllers/statefulset.go b/controllers/statefulset.go index 6f9a18636..38ffb77ad 100644 --- a/controllers/statefulset.go +++ b/controllers/statefulset.go @@ -604,18 +604,16 @@ func (r *SingleClusterReconciler) updateSTS( return err } - // Updating statefulSet object only if there is a difference in spec. - if reflect.DeepEqual(found.Spec, statefulSet.Spec) { - r.Log.Info("Skipping StatefulSet update, no change in spec") - - return nil - } // Save the updated stateful set. found.Spec = statefulSet.Spec return r.Client.Update(context.TODO(), found, updateOption) }) if err != nil { - return err + return fmt.Errorf( + "failed to update StatefulSet %s: %v", + statefulSet.Name, + err, + ) } r.Log.V(1).Info( @@ -785,7 +783,6 @@ func (r *SingleClusterReconciler) updateSTSNonPVStorage( // Add volume in statefulSet template k8sVolume := createVolumeForVolumeAttachment(volume) - st.Spec.Template.Spec.Volumes = append( st.Spec.Template.Spec.Volumes, k8sVolume, ) @@ -1200,8 +1197,6 @@ func getDefaultAerospikeInitContainerVolumeMounts() []corev1.VolumeMount { func getDefaultSTSVolumes( aeroCluster *asdbv1.AerospikeCluster, rackState *RackState, ) []corev1.Volume { - defaultMode := corev1.SecretVolumeSourceDefaultMode - return []corev1.Volume{ { Name: confDirName, @@ -1218,7 +1213,6 @@ func getDefaultSTSVolumes( aeroCluster, rackState.Rack.ID, ).Name, }, - DefaultMode: &defaultMode, }, }, }, @@ -1355,19 +1349,6 @@ func createPVCForVolumeAttachment( } func createVolumeForVolumeAttachment(volume *asdbv1.VolumeSpec) corev1.Volume { - perm := corev1.SecretVolumeSourceDefaultMode - - switch { - case volume.Source.Secret != nil: - if volume.Source.Secret.DefaultMode == nil { - volume.Source.Secret.DefaultMode = &perm - } - case volume.Source.ConfigMap != nil: - if volume.Source.ConfigMap.DefaultMode == nil { - volume.Source.ConfigMap.DefaultMode = &perm - } - } - return corev1.Volume{ Name: volume.Name, // Add all type of source, @@ -1517,10 +1498,6 @@ func getSTSContainerPort( containerPort.HostPort = containerPort.ContainerPort } - if containerPort.Protocol == "" { - containerPort.Protocol = corev1.ProtocolTCP - } - ports = append(ports, containerPort) } diff --git a/helm-charts/aerospike-kubernetes-operator/templates/aerospike-operator-manager-clusterrole.yaml b/helm-charts/aerospike-kubernetes-operator/templates/aerospike-operator-manager-clusterrole.yaml index 3f590e368..c379e7f0e 100644 --- a/helm-charts/aerospike-kubernetes-operator/templates/aerospike-operator-manager-clusterrole.yaml +++ b/helm-charts/aerospike-kubernetes-operator/templates/aerospike-operator-manager-clusterrole.yaml @@ -102,7 +102,6 @@ rules: - "" resources: - secrets - - persistentvolumes verbs: - get - apiGroups: diff --git a/pkg/utils/pod.go b/pkg/utils/pod.go index a057c718d..b2c8e45f6 100644 --- a/pkg/utils/pod.go +++ b/pkg/utils/pod.go @@ -224,8 +224,8 @@ func IsPodReasonUnschedulable(pod *corev1.Pod) bool { func IsPodNeedsToMigrate(pod *corev1.Pod) bool { for _, condition := range pod.Status.Conditions { if condition.Type == corev1.PodScheduled && (condition.Reason == corev1.PodReasonUnschedulable || - condition.Reason == corev1.PodReasonSchedulerError && - strings.Contains(condition.Message, "nodeinfo not found for node name")) { + (condition.Reason == corev1.PodReasonSchedulerError && + strings.Contains(condition.Message, "nodeinfo not found for node name"))) { return true } } diff --git a/test/cluster_helper.go b/test/cluster_helper.go index 241dda027..945b871a9 100644 --- a/test/cluster_helper.go +++ b/test/cluster_helper.go @@ -1373,14 +1373,11 @@ func getStorageVolumeForAerospike(name, path string) asdbv1.VolumeSpec { } func getStorageVolumeForSecret() asdbv1.VolumeSpec { - perm := corev1.SecretVolumeSourceDefaultMode - return asdbv1.VolumeSpec{ Name: aerospikeConfigSecret, Source: asdbv1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ - SecretName: tlsSecretName, - DefaultMode: &perm, + SecretName: tlsSecretName, }, }, Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ diff --git a/test/drain_test.go b/test/drain_test.go new file mode 100644 index 000000000..76375d66e --- /dev/null +++ b/test/drain_test.go @@ -0,0 +1,74 @@ +package test + +import ( + goctx "context" + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "golang.org/x/net/context" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" +) + +var _ = Describe( + "NodeDrain", func() { + ctx := goctx.TODO() + Context( + "Clean local PVC", func() { + clusterName := "fake-local-volume-cluster" + clusterNamespacedName := getNamespacedName( + clusterName, namespace, + ) + aeroCluster := createDummyAerospikeCluster( + clusterNamespacedName, 2, + ) + + It( + "Should delete local PVC if pod is unschedulable", func() { + aeroCluster.Spec.Storage.LocalStorageClasses = []string{storageClass} + aeroCluster.Spec.Storage.CleanLocalPVC = true + err := deployCluster(k8sClient, ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + + 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()) + + err = checkPVCStatus(ctx, "ns-"+clusterName+"-0-1") + Expect(err).ToNot(HaveOccurred()) + + _ = deleteCluster(k8sClient, ctx, aeroCluster) + }, + ) + }, + ) + }, +) + +func checkPVCStatus(ctx context.Context, pvcName string) error { + pvc := &corev1.PersistentVolumeClaim{} + pvcNamespacesName := getNamespacedName( + pvcName, namespace, + ) + + err := k8sClient.Get(ctx, pvcNamespacesName, pvc) + if err != nil { + if !errors.IsNotFound(err) { + return err + } + + return nil + } + + if pvc.Status.Phase == "Pending" { + return nil + } + + return fmt.Errorf("PVC status is not expected %s", pvc.Name) +}