From 0240ee85631d360e0acb328bcb30489059377aec Mon Sep 17 00:00:00 2001
From: travisn <tnielsen@redhat.com>
Date: Tue, 30 Jan 2024 15:55:11 -0700
Subject: [PATCH] core: skip reconcile if override configmap is empty

If the configmap rook-config-override is empty,
there is no need to trigger the reconcile to update
the ceph daemons. This configmap update is causing
unnecessary reconciles periodically in some clusters
even when it is empty.

Signed-off-by: travisn <tnielsen@redhat.com>
---
 pkg/operator/ceph/controller/predicate.go     | 30 ++++++++++++------
 .../ceph/controller/predicate_test.go         | 31 ++++++++++++++++---
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/pkg/operator/ceph/controller/predicate.go b/pkg/operator/ceph/controller/predicate.go
index 843a8234e836..631c68120c48 100644
--- a/pkg/operator/ceph/controller/predicate.go
+++ b/pkg/operator/ceph/controller/predicate.go
@@ -579,17 +579,17 @@ func WatchPredicateForNonCRDObject(owner runtime.Object, scheme *runtime.Scheme)
 				logger.Debugf("object %q matched on update", objectName)
 
 				// CONFIGMAP WHITELIST
-				// Only reconcile on rook-config-override CM changes
-				isCMTConfigOverride := isCMTConfigOverride(e.ObjectNew)
-				if isCMTConfigOverride {
-					logger.Debugf("do reconcile when the cm is %s", k8sutil.ConfigOverrideName)
+				// Only reconcile on rook-config-override CM changes if the configmap changed
+				shouldReconcileCM := shouldReconcileCM(e.ObjectOld, e.ObjectNew)
+				if shouldReconcileCM {
+					logger.Infof("reconcile due to updated configmap %s", k8sutil.ConfigOverrideName)
 					return true
 				}
 
 				// If the resource is a ConfigMap we don't reconcile
 				_, ok := e.ObjectNew.(*corev1.ConfigMap)
 				if ok {
-					logger.Debugf("do not reconcile on configmap that is not %q", k8sutil.ConfigOverrideName)
+					logger.Debugf("do not reconcile on configmap %q", objectName)
 					return false
 				}
 
@@ -747,15 +747,27 @@ func isDeployment(obj runtime.Object, appName string) bool {
 	return false
 }
 
-func isCMTConfigOverride(obj runtime.Object) bool {
+func shouldReconcileCM(objOld runtime.Object, objNew runtime.Object) bool {
 	// If not a ConfigMap, let's not reconcile
-	cm, ok := obj.(*corev1.ConfigMap)
+	cmNew, ok := objNew.(*corev1.ConfigMap)
 	if !ok {
 		return false
 	}
 
-	objectName := cm.GetName()
-	return objectName == k8sutil.ConfigOverrideName
+	// If not a ConfigMap, let's not reconcile
+	cmOld, ok := objOld.(*corev1.ConfigMap)
+	if !ok {
+		return false
+	}
+
+	objectName := cmNew.GetName()
+	if objectName != k8sutil.ConfigOverrideName {
+		return false
+	}
+	if !reflect.DeepEqual(cmNew.Data, cmOld.Data) {
+		return true
+	}
+	return false
 }
 
 func isCMToIgnoreOnDelete(obj runtime.Object) bool {
diff --git a/pkg/operator/ceph/controller/predicate_test.go b/pkg/operator/ceph/controller/predicate_test.go
index 227a09050a4b..63565f154543 100644
--- a/pkg/operator/ceph/controller/predicate_test.go
+++ b/pkg/operator/ceph/controller/predicate_test.go
@@ -164,16 +164,39 @@ func TestIsCanary(t *testing.T) {
 
 func TestIsCMToIgnoreOnUpdate(t *testing.T) {
 	blockPool := &cephv1.CephBlockPool{}
-	assert.False(t, isCMTConfigOverride(blockPool))
+	reconcile := shouldReconcileCM(blockPool, blockPool)
+	assert.False(t, reconcile)
 
 	cm := &corev1.ConfigMap{}
-	assert.False(t, isCMTConfigOverride(cm))
+	reconcile = shouldReconcileCM(cm, cm)
+	assert.False(t, reconcile)
 
 	cm.Name = "rook-ceph-mon-endpoints"
-	assert.False(t, isCMTConfigOverride(cm))
+	reconcile = shouldReconcileCM(cm, cm)
+	assert.False(t, reconcile)
 
+	// Valid name, but cm completely empty
 	cm.Name = "rook-config-override"
-	assert.True(t, isCMTConfigOverride(cm))
+	oldCM := &corev1.ConfigMap{}
+	oldCM.Name = cm.Name
+	reconcile = shouldReconcileCM(oldCM, cm)
+	assert.False(t, reconcile)
+
+	// Both have empty config value
+	cm.Data = map[string]string{"config": ""}
+	oldCM.Data = map[string]string{"config": ""}
+	reconcile = shouldReconcileCM(oldCM, cm)
+	assert.False(t, reconcile)
+
+	// Something added to the CM
+	cm.Data = map[string]string{"config": "somevalue"}
+	reconcile = shouldReconcileCM(oldCM, cm)
+	assert.True(t, reconcile)
+
+	// A value changed in the CM
+	oldCM.Data = map[string]string{"config": "diffvalue"}
+	reconcile = shouldReconcileCM(oldCM, cm)
+	assert.True(t, reconcile)
 }
 
 func TestIsCMToIgnoreOnDelete(t *testing.T) {