From c5dff9857dcb2dcef550247656c2f8d7e092c3aa Mon Sep 17 00:00:00 2001 From: Michael Burman Date: Wed, 24 Jul 2024 16:19:10 +0300 Subject: [PATCH] Revert changes in a3ce8bc93492282484a97208286d27fde58eb023 but add tests to ensure the field is working properly --- .../v1alpha1/k8ssandracluster_types.go | 2 +- .../v1alpha1/k8ssandracluster_types_test.go | 21 +++++++++++++++ controllers/k8ssandra/datacenters.go | 11 +++----- controllers/k8ssandra/datacenters_test.go | 27 ++++++++++++++++++- .../k8ssandra/k8ssandracluster_controller.go | 8 +++--- 5 files changed, 54 insertions(+), 15 deletions(-) diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go index f60306743..7e44c5cdf 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go @@ -566,5 +566,5 @@ func (kc *K8ssandraCluster) GetClusterIdHash() string { } func (k *K8ssandraCluster) GenerationChanged() bool { - return k.Status.ObservedGeneration != 0 && k.Status.ObservedGeneration < k.Generation + return k.Status.ObservedGeneration < k.Generation } diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go index 3ab6bafd7..154620021 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go @@ -3,6 +3,7 @@ package v1alpha1 import ( "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/yaml" @@ -194,3 +195,23 @@ metadata: assert.Equal(t, "nodePortSvcLabelValue1", dc.Meta.ServiceConfig.NodePortService.Labels["nodePortSvcLabel1"]) assert.Equal(t, "nodePortSvcAnnotationValue1", dc.Meta.ServiceConfig.NodePortService.Annotations["nodePortSvcAnnotation1"]) } + +func TestGenerationChanged(t *testing.T) { + assert := assert.New(t) + kc := &K8ssandraCluster{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, + Spec: K8ssandraClusterSpec{}, + } + + kc.Status = K8ssandraClusterStatus{ + ObservedGeneration: 0, + } + + assert.True(kc.GenerationChanged()) + kc.Status.ObservedGeneration = 2 + assert.False(kc.GenerationChanged()) + kc.ObjectMeta.Generation = 3 + assert.True(kc.GenerationChanged()) +} diff --git a/controllers/k8ssandra/datacenters.go b/controllers/k8ssandra/datacenters.go index d7f60e4cd..b96d4fa07 100644 --- a/controllers/k8ssandra/datacenters.go +++ b/controllers/k8ssandra/datacenters.go @@ -9,7 +9,6 @@ import ( "github.com/Masterminds/semver/v3" "github.com/go-logr/logr" - "github.com/google/go-cmp/cmp" cassdcapi "github.com/k8ssandra/cass-operator/apis/cassandra/v1beta1" cassctlapi "github.com/k8ssandra/cass-operator/apis/control/v1alpha1" ktaskapi "github.com/k8ssandra/k8ssandra-operator/apis/control/v1alpha1" @@ -34,8 +33,7 @@ const ( rebuildNodesLabel = "k8ssandra.io/rebuild-nodes" ) -func AllowUpdate(kc *api.K8ssandraCluster, logger logr.Logger) bool { - logger.Info(fmt.Sprintf("Generation: %d, ObservedGeneration: %d", kc.Generation, kc.Status.ObservedGeneration)) +func AllowUpdate(kc *api.K8ssandraCluster) bool { return kc.GenerationChanged() || metav1.HasAnnotation(kc.ObjectMeta, api.AutomatedUpdateAnnotation) } @@ -152,10 +150,7 @@ func (r *K8ssandraClusterReconciler) reconcileDatacenters(ctx context.Context, k r.setStatusForDatacenter(kc, actualDc) - dcDiff := cmp.Diff(actualDc, desiredDc) - logger.Info(fmt.Sprintf("ObservedGeneration: %d, Generation: %d, Datacenter: %v, dcDiff: %s", actualDc.Status.ObservedGeneration, actualDc.GetGeneration(), dcKey, dcDiff)) - - if !annotations.CompareHashAnnotations(actualDc, desiredDc) && !AllowUpdate(kc, logger) { + if !annotations.CompareHashAnnotations(actualDc, desiredDc) && !AllowUpdate(kc) { logger.Info("Datacenter requires an update, but we're not allowed to do it", "CassandraDatacenter", dcKey) // We're not allowed to update, but need to patch := client.MergeFrom(kc.DeepCopy()) @@ -253,7 +248,7 @@ func (r *K8ssandraClusterReconciler) reconcileDatacenters(ctx context.Context, k } } - if AllowUpdate(kc, logger) { + if AllowUpdate(kc) { dcsRequiringUpdate := make([]string, 0, len(actualDcs)) for _, dc := range actualDcs { if dc.Status.GetConditionStatus(cassdcapi.DatacenterRequiresUpdate) == corev1.ConditionTrue { diff --git a/controllers/k8ssandra/datacenters_test.go b/controllers/k8ssandra/datacenters_test.go index 7f13a92c2..dd6e35b2b 100644 --- a/controllers/k8ssandra/datacenters_test.go +++ b/controllers/k8ssandra/datacenters_test.go @@ -1,6 +1,8 @@ package k8ssandra import ( + "testing" + "github.com/Masterminds/semver/v3" cassdcapi "github.com/k8ssandra/cass-operator/apis/cassandra/v1beta1" api "github.com/k8ssandra/k8ssandra-operator/apis/k8ssandra/v1alpha1" @@ -8,7 +10,6 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "testing" ) var ( @@ -263,3 +264,27 @@ func TestGetSourceDatacenterName_Conflict(t *testing.T) { } } + +func TestAllowUpdate(t *testing.T) { + assert := assert.New(t) + kc := &api.K8ssandraCluster{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, + Spec: api.K8ssandraClusterSpec{}, + } + + kc.Status = api.K8ssandraClusterStatus{ + ObservedGeneration: 0, + } + + assert.True(AllowUpdate(kc)) + kc.Status.ObservedGeneration = 1 + assert.True(AllowUpdate(kc)) + kc.Status.ObservedGeneration = 2 + assert.False(AllowUpdate(kc)) + metav1.SetMetaDataAnnotation(&kc.ObjectMeta, api.AutomatedUpdateAnnotation, string(api.AllowUpdateOnce)) + assert.True(AllowUpdate(kc)) + metav1.SetMetaDataAnnotation(&kc.ObjectMeta, api.AutomatedUpdateAnnotation, string(api.AllowUpdateAlways)) + assert.True(AllowUpdate(kc)) +} diff --git a/controllers/k8ssandra/k8ssandracluster_controller.go b/controllers/k8ssandra/k8ssandracluster_controller.go index b948a8800..dd3372d6d 100644 --- a/controllers/k8ssandra/k8ssandracluster_controller.go +++ b/controllers/k8ssandra/k8ssandracluster_controller.go @@ -18,7 +18,6 @@ package k8ssandra import ( "context" - "fmt" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" @@ -157,7 +156,7 @@ func (r *K8ssandraClusterReconciler) reconcile(ctx context.Context, kc *api.K8ss return recResult.Output() } - if res := updateStatus(ctx, r.Client, kc, kcLogger); res.Completed() { + if res := updateStatus(ctx, r.Client, kc); res.Completed() { return res.Output() } @@ -186,8 +185,8 @@ func (r *K8ssandraClusterReconciler) afterCassandraReconciled(ctx context.Contex return result.Continue() } -func updateStatus(ctx context.Context, r client.Client, kc *api.K8ssandraCluster, kcLogger logr.Logger) result.ReconcileResult { - if AllowUpdate(kc, kcLogger) { +func updateStatus(ctx context.Context, r client.Client, kc *api.K8ssandraCluster) result.ReconcileResult { + if AllowUpdate(kc) { if metav1.HasAnnotation(kc.ObjectMeta, api.AutomatedUpdateAnnotation) { if kc.Annotations[api.AutomatedUpdateAnnotation] == string(api.AllowUpdateOnce) { delete(kc.ObjectMeta.Annotations, api.AutomatedUpdateAnnotation) @@ -199,7 +198,6 @@ func updateStatus(ctx context.Context, r client.Client, kc *api.K8ssandraCluster kc.Status.SetConditionStatus(api.ClusterRequiresUpdate, corev1.ConditionFalse) } - kcLogger.Info(fmt.Sprintf("Updating observed generation to %d", kc.Generation)) kc.Status.ObservedGeneration = kc.Generation if err := r.Status().Update(ctx, kc); err != nil { return result.Error(err)