From 92faabce4d14d14b2097d002f4cf557ff6be513a Mon Sep 17 00:00:00 2001 From: Sudhanshu Ranjan Date: Tue, 30 Apr 2024 12:01:38 +0530 Subject: [PATCH] Address review comments --- api/v1/utils.go | 1 + controllers/poddistruptionbudget.go | 26 ++++++++ ...erospike-operator-manager-clusterrole.yaml | 1 + test/poddisruptionbudget_test.go | 64 +++++++++++++++++-- 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/api/v1/utils.go b/api/v1/utils.go index ed5c65040..653145710 100644 --- a/api/v1/utils.go +++ b/api/v1/utils.go @@ -65,6 +65,7 @@ const ( AerospikeInitContainerDefaultRegistryNamespace = "aerospike" AerospikeInitContainerDefaultRepoAndTag = "aerospike-kubernetes-init:2.2.0-dev3" AerospikeAppLabel = "app" + AerospikeAppLabelValue = "aerospike-cluster" AerospikeCustomResourceLabel = "aerospike.com/cr" AerospikeRackIDLabel = "aerospike.com/rack-id" AerospikeAPIVersionLabel = "aerospike.com/api-version" diff --git a/controllers/poddistruptionbudget.go b/controllers/poddistruptionbudget.go index c81cfb739..9e2a055d5 100644 --- a/controllers/poddistruptionbudget.go +++ b/controllers/poddistruptionbudget.go @@ -49,6 +49,11 @@ func (r *SingleClusterReconciler) deletePDB() error { return err } + if !isPDBCreatedByOperator(pdb) { + r.Log.Info("PodDisruptionBudget is not created/owned by operator. Skipping delete") + return nil + } + // Delete the PodDisruptionBudget return r.Client.Delete(context.TODO(), pdb) } @@ -125,6 +130,18 @@ func (r *SingleClusterReconciler) createOrUpdatePDB() error { utils.NamespacedName(r.aeroCluster.Namespace, r.aeroCluster.Name), ) + // This will ensure that cluster is not deployed with PDB created by user + // cluster deploy call itself will fail. + // If PDB is not created by operator then no need to even match the spec + if !isPDBCreatedByOperator(pdb) { + r.Log.Info("PodDisruptionBudget is not created/owned by operator. Skipping update") + + return fmt.Errorf( + "failed to update PodDisruptionBudget: %v", + fmt.Errorf("PodDisruptionBudget is not created/owned by operator"), + ) + } + if pdb.Spec.MaxUnavailable.String() != r.aeroCluster.Spec.MaxUnavailable.String() { pdb.Spec.MaxUnavailable = r.aeroCluster.Spec.MaxUnavailable @@ -143,3 +160,12 @@ func (r *SingleClusterReconciler) createOrUpdatePDB() error { return nil } + +func isPDBCreatedByOperator(pdb *v1.PodDisruptionBudget) bool { + val, ok := pdb.GetLabels()[asdbv1.AerospikeAppLabel] + if ok && val == asdbv1.AerospikeAppLabelValue { + return true + } + + return false +} 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 adf721767..05053cfbc 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 @@ -122,6 +122,7 @@ rules: - poddisruptionbudgets verbs: - create + - delete - get - patch - update diff --git a/test/poddisruptionbudget_test.go b/test/poddisruptionbudget_test.go index f8ccb3f1c..2f45b00f5 100644 --- a/test/poddisruptionbudget_test.go +++ b/test/poddisruptionbudget_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/gomega" policyv1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" @@ -19,7 +20,7 @@ var _ = Describe( ctx := context.TODO() aeroCluster := &asdbv1.AerospikeCluster{} maxUnavailable := intstr.FromInt(0) - defaultMaxUpavailable := intstr.FromInt(1) + defaultMaxUnavailable := intstr.FromInt(1) clusterNamespacedName := getNamespacedName("pdb-test-cluster", namespace) BeforeEach(func() { @@ -36,7 +37,7 @@ var _ = Describe( It("Validate create PDB with default maxUnavailable", func() { err := deployCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - validatePDB(ctx, aeroCluster, defaultMaxUpavailable.IntValue()) + validatePDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) }) It("Validate create PDB with specified maxUnavailable", func() { @@ -49,7 +50,7 @@ var _ = Describe( It("Validate update PDB", func() { err := deployCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - validatePDB(ctx, aeroCluster, defaultMaxUpavailable.IntValue()) + validatePDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) Expect(err).ToNot(HaveOccurred()) @@ -79,7 +80,7 @@ var _ = Describe( By("Create cluster with PDB enabled") err := deployCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - validatePDB(ctx, aeroCluster, 1) + validatePDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) By("Update disablePDB to true") aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) @@ -102,7 +103,34 @@ var _ = Describe( aeroCluster.Spec.DisablePDB = ptr.To(false) err = updateCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - validatePDB(ctx, aeroCluster, 1) + validatePDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) + }) + + It("Validate that non-operator created PDB is not created", func() { + err := createPDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) + Expect(err).ToNot(HaveOccurred()) + + // Create cluster should fail as PDB is not created by operator + err = deployCluster(k8sClient, ctx, aeroCluster) + Expect(err).To(HaveOccurred()) + + err = deletePDB(ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + + // Create cluster should pass as PDB is deleted + err = deployCluster(k8sClient, ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + validatePDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) + }) + + It("Validate that cluster is deployed with disabledPDB even if non-operator created PDB is present", func() { + err := createPDB(ctx, aeroCluster, defaultMaxUnavailable.IntValue()) + Expect(err).ToNot(HaveOccurred()) + + // Create cluster with disabledPDB + aeroCluster.Spec.DisablePDB = ptr.To(true) + err = deployCluster(k8sClient, ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) }) }) @@ -124,6 +152,7 @@ var _ = Describe( err := deployCluster(k8sClient, ctx, aeroCluster) Expect(err).To(HaveOccurred()) }) + It("Should fail if maxUnavailable is given but disablePDB is true", func() { aeroCluster.Spec.DisablePDB = ptr.To(true) value := intstr.FromInt(1) @@ -159,3 +188,28 @@ func getPDB(ctx context.Context, aerocluster *asdbv1.AerospikeCluster) (*policyv return pdb, err } + +func createPDB(ctx context.Context, aerocluster *asdbv1.AerospikeCluster, maxUnavailable int) error { + pdb := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: aerocluster.Name, + Namespace: aerocluster.Namespace, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(maxUnavailable)}, + }, + } + + return k8sClient.Create(ctx, pdb) +} + +func deletePDB(ctx context.Context, aerocluster *asdbv1.AerospikeCluster) error { + pdb := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: aerocluster.Name, + Namespace: aerocluster.Namespace, + }, + } + + return k8sClient.Delete(ctx, pdb) +}