Repository: rook/rook
Pull request #9617 (2022-01-20 16:53:41)

core: reconcile operator configuration with env var (backport #9613)
This is an automatic backport of pull request #9613 done by [Mergify](https://mergify.com).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the [documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>



Patch:
diff --git a/pkg/operator/ceph/controller/predicate.go b/pkg/operator/ceph/controller/predicate.go
index 77eeac7ec..7c2ed2011 100644
--- a/pkg/operator/ceph/controller/predicate.go
+++ b/pkg/operator/ceph/controller/predicate.go
@@ -745,6 +745,8 @@ func DuplicateCephClusters(ctx context.Context, c client.Client, object client.O
 		return true
 	}
 
+	logger.Debugf("found %d ceph clusters in namespace %q", len(cephClusterList.Items), object.GetNamespace())
+
 	// This check is needed when the operator is down and a cluster was created
 	if len(cephClusterList.Items) > 1 {
 		// Since multiple predicate are using this function we don't want all of them to log the
diff --git a/pkg/operator/ceph/csi/controller.go b/pkg/operator/ceph/csi/controller.go
index 8fd48c264..a905f1ddd 100644
--- a/pkg/operator/ceph/csi/controller.go
+++ b/pkg/operator/ceph/csi/controller.go
@@ -54,7 +54,7 @@ type ReconcileCSI struct {
 // Add creates a new Ceph CSI Controller and adds it to the Manager. The Manager will set fields on the Controller
 // and Start it when the Manager is Started.
 func Add(mgr manager.Manager, context *clusterd.Context, opManagerContext context.Context, opConfig opcontroller.OperatorConfig) error {
-	return add(opManagerContext, mgr, newReconciler(mgr, context, opManagerContext, opConfig))
+	return add(opManagerContext, mgr, newReconciler(mgr, context, opManagerContext, opConfig), opConfig)
 }
 
 // newReconciler returns a new reconcile.Reconciler
@@ -67,7 +67,7 @@ func newReconciler(mgr manager.Manager, context *clusterd.Context, opManagerCont
 	}
 }
 
-func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error {
+func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler, opConfig opcontroller.OperatorConfig) error {
 	// Create a new controller
 	c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: r})
 	if err != nil {
@@ -77,14 +77,14 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error
 
 	// Watch for ConfigMap (operator config)
 	err = c.Watch(&source.Kind{
-		Type: &v1.ConfigMap{TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: v1.SchemeGroupVersion.String()}}}, &handler.EnqueueRequestForObject{}, predicateController(ctx, mgr.GetClient()))
+		Type: &v1.ConfigMap{TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: v1.SchemeGroupVersion.String()}}}, &handler.EnqueueRequestForObject{}, predicateController(ctx, mgr.GetClient(), opConfig.OperatorNamespace))
 	if err != nil {
 		return err
 	}
 
 	// Watch for CephCluster
 	err = c.Watch(&source.Kind{
-		Type: &cephv1.CephCluster{TypeMeta: metav1.TypeMeta{Kind: "CephCluster", APIVersion: v1.SchemeGroupVersion.String()}}}, &handler.EnqueueRequestForObject{}, predicateController(ctx, mgr.GetClient()))
+		Type: &cephv1.CephCluster{TypeMeta: metav1.TypeMeta{Kind: "CephCluster", APIVersion: v1.SchemeGroupVersion.String()}}}, &handler.EnqueueRequestForObject{}, predicateController(ctx, mgr.GetClient(), opConfig.OperatorNamespace))
 	if err != nil {
 		return err
 	}
diff --git a/pkg/operator/ceph/csi/predicate.go b/pkg/operator/ceph/csi/predicate.go
index c6b3c539d..2742475e9 100644
--- a/pkg/operator/ceph/csi/predicate.go
+++ b/pkg/operator/ceph/csi/predicate.go
@@ -25,14 +25,16 @@ import (
 	"github.com/rook/rook/pkg/operator/ceph/controller"
 	opcontroller "github.com/rook/rook/pkg/operator/ceph/controller"
 	v1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
 // predicateController is the predicate function to trigger reconcile on operator configuration cm change
-func predicateController(ctx context.Context, c client.Client) predicate.Funcs {
+func predicateController(ctx context.Context, c client.Client, opNamespace string) predicate.Funcs {
 	return predicate.Funcs{
 		CreateFunc: func(e event.CreateEvent) bool {
 			// if the operator configuration file is created we want to reconcile
@@ -48,12 +50,24 @@ func predicateController(ctx context.Context, c client.Client) predicate.Funcs {
 				if controller.DuplicateCephClusters(ctx, c, e.Object, false) {
 					return false
 				}
+
+				// We still have users that don't use the ConfigMap to configure the
+				// operator/csi and still rely on environment variables from the operator pod's
+				// spec. So we still want to reconcile the controller if the ConfigMap cannot be found.
+				err := c.Get(ctx, types.NamespacedName{Name: opcontroller.OperatorSettingConfigMapName, Namespace: opNamespace}, &v1.ConfigMap{})
+				if err != nil && kerrors.IsNotFound(err) {
+					logger.Debugf("could not find operator configuration ConfigMap, will reconcile the csi controller")
+					return true
+				}
+
 				// This allows us to avoid a double reconcile of the CSI controller if this is not
 				// the first generation of the CephCluster. So only return true if this is the very
 				// first instance of the CephCluster
 				// Corner case is when the cluster is created but the operator is down AND the cm
 				// does not exist... However, these days the operator config map is part of the
 				// operator.yaml so it's probably acceptable?
+				// This does not account for the case where the cephcluster is already deployed and
+				// the upgrade the operator or restart it. However, the CM check above should catch that
 				return cephCluster.Generation == 1
 			}
 
@@ -76,6 +90,11 @@ func predicateController(ctx context.Context, c client.Client) predicate.Funcs {
 		},
 
 		DeleteFunc: func(e event.DeleteEvent) bool {
+			// if the operator configuration file is deleted we want to reconcile to apply the
+			// configuration based on environment variables present in the operator's pod spec
+			if cm, ok := e.Object.(*v1.ConfigMap); ok {
+				return cm.Name == opcontroller.OperatorSettingConfigMapName
+			}
 			return false
 		},
 


Test patch:
diff --git a/pkg/operator/ceph/csi/predicate_test.go b/pkg/operator/ceph/csi/predicate_test.go
index af718e63d..95f86820d 100644
--- a/pkg/operator/ceph/csi/predicate_test.go
+++ b/pkg/operator/ceph/csi/predicate_test.go
@@ -17,9 +17,18 @@ limitations under the License.
 package csi
 
 import (
+	"context"
 	"testing"
 
+	"github.com/coreos/pkg/capnslog"
+	cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
+	"github.com/rook/rook/pkg/client/clientset/versioned/scheme"
 	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
 func Test_findCSIChange(t *testing.T) {
@@ -123,3 +132,119 @@ func Test_findCSIChange(t *testing.T) {
 		assert.True(t, b)
 	})
 }
+
+func Test_predicateController(t *testing.T) {
+	capnslog.SetGlobalLogLevel(capnslog.DEBUG)
+	var (
+		client    client.WithWatch
+		namespace v1.Namespace
+		p         predicate.Funcs
+		c         event.CreateEvent
+		d         event.DeleteEvent
+		u         event.UpdateEvent
+		cm        v1.ConfigMap
+		cm2       v1.ConfigMap
+		cluster   cephv1.CephCluster
+		cluster2  cephv1.CephCluster
+	)
+
+	s := scheme.Scheme
+	s.AddKnownTypes(cephv1.SchemeGroupVersion, &v1.ConfigMap{}, &v1.ConfigMapList{}, &cephv1.CephCluster{}, &cephv1.CephClusterList{})
+	client = fake.NewClientBuilder().WithScheme(s).Build()
+
+	t.Run("create event is not the one we are looking for", func(t *testing.T) {
+		c = event.CreateEvent{Object: &namespace}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Create(c))
+	})
+
+	t.Run("create event is a CM but not the operator config", func(t *testing.T) {
+		c = event.CreateEvent{Object: &cm}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Create(c))
+	})
+
+	t.Run("delete event is a CM and it's not the operator config", func(t *testing.T) {
+		d = event.DeleteEvent{Object: &cm}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Delete(d))
+	})
+
+	t.Run("create event is a CM and it's the operator config", func(t *testing.T) {
+		cm.Name = "rook-ceph-operator-config"
+		c = event.CreateEvent{Object: &cm}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.True(t, p.Create(c))
+	})
+
+	t.Run("delete event is a CM and it's the operator config", func(t *testing.T) {
+		d = event.DeleteEvent{Object: &cm}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.True(t, p.Delete(d))
+	})
+
+	t.Run("update event is a CM and it's the operator config but nothing changed", func(t *testing.T) {
+		u = event.UpdateEvent{ObjectOld: &cm, ObjectNew: &cm}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Update(u))
+	})
+
+	t.Run("update event is a CM and the content changed but with incorrect setting", func(t *testing.T) {
+		cm.Data = map[string]string{"foo": "bar"}
+		cm2.Name = "rook-ceph-operator-config"
+		cm2.Data = map[string]string{"foo": "ooo"}
+		u = event.UpdateEvent{ObjectOld: &cm, ObjectNew: &cm2}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Update(u))
+	})
+
+	t.Run("update event is a CM and the content changed with correct setting", func(t *testing.T) {
+		cm.Data = map[string]string{"foo": "bar"}
+		cm2.Name = "rook-ceph-operator-config"
+		cm2.Data = map[string]string{"CSI_PROVISIONER_REPLICAS": "2"}
+		u = event.UpdateEvent{ObjectOld: &cm, ObjectNew: &cm2}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.True(t, p.Update(u))
+	})
+
+	t.Run("create event is a CephCluster and it's the first instance and a cm is present", func(t *testing.T) {
+		cm.Namespace = "rook-ceph"
+		err := client.Create(context.TODO(), &cm)
+		assert.NoError(t, err)
+		cluster.Generation = 1
+		c = event.CreateEvent{Object: &cluster}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.True(t, p.Create(c))
+	})
+
+	t.Run("create event is a CephCluster and it's not the first instance", func(t *testing.T) {
+		cluster.Generation = 2
+		c = event.CreateEvent{Object: &cluster}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Create(c))
+	})
+
+	t.Run("create event is a CephCluster and it's not the first instance but no cm so we reconcile", func(t *testing.T) {
+		err := client.Delete(context.TODO(), &cm)
+		assert.NoError(t, err)
+		cluster.Generation = 2
+		c = event.CreateEvent{Object: &cluster}
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.True(t, p.Create(c))
+	})
+
+	t.Run("create event is a CephCluster and a cluster already exists - not reconciling", func(t *testing.T) {
+		cluster.Name = "ceph1"
+		cluster2.Name = "ceph2"
+		cluster.Namespace = "rook-ceph"
+		cluster2.Namespace = "rook-ceph"
+		err := client.Create(context.TODO(), &cluster)
+		assert.NoError(t, err)
+		err = client.Create(context.TODO(), &cluster2)
+		assert.NoError(t, err)
+		c = event.CreateEvent{Object: &cluster2}
+		assert.Equal(t, "rook-ceph", c.Object.GetNamespace())
+		p = predicateController(context.TODO(), client, "rook-ceph")
+		assert.False(t, p.Create(c))
+	})
+}