Repository: rook/rook
Pull request #10144 (2022-04-22 17:57:12)

csi: add/remove mon IP from csi config (backport #10135)
This is an automatic backport of pull request #10135 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/cluster/cluster_external.go b/pkg/operator/ceph/cluster/cluster_external.go
index bc960a7ae..8f67c7da8 100644
--- a/pkg/operator/ceph/cluster/cluster_external.go
+++ b/pkg/operator/ceph/cluster/cluster_external.go
@@ -114,7 +114,7 @@ func (c *ClusterController) configureExternalCephCluster(cluster *cluster) error
 	}
 
 	// Save CSI configmap
-	err = csi.SaveClusterConfig(c.context.Clientset, c.namespacedName.Namespace, cluster.ClusterInfo, &csi.CsiClusterConfigEntry{Monitors: csi.MonEndpoints(cluster.ClusterInfo.Monitors)})
+	err = csi.SaveClusterConfig(c.context.Clientset, c.namespacedName.Namespace, cluster.ClusterInfo, &csi.CsiClusterConfigEntry{Namespace: cluster.ClusterInfo.Namespace, Monitors: csi.MonEndpoints(cluster.ClusterInfo.Monitors)})
 	if err != nil {
 		return errors.Wrap(err, "failed to update csi cluster config")
 	}
diff --git a/pkg/operator/ceph/cluster/mon/mon.go b/pkg/operator/ceph/cluster/mon/mon.go
index 6c460c690..efc9a4096 100644
--- a/pkg/operator/ceph/cluster/mon/mon.go
+++ b/pkg/operator/ceph/cluster/mon/mon.go
@@ -1050,7 +1050,7 @@ func (c *Cluster) saveMonConfig() error {
 		return errors.Wrap(err, "failed to write connection config for new mons")
 	}
 
-	if err := csi.SaveClusterConfig(c.context.Clientset, c.Namespace, c.ClusterInfo, &csi.CsiClusterConfigEntry{Monitors: csi.MonEndpoints(c.ClusterInfo.Monitors)}); err != nil {
+	if err := csi.SaveClusterConfig(c.context.Clientset, c.Namespace, c.ClusterInfo, &csi.CsiClusterConfigEntry{Namespace: c.ClusterInfo.Namespace, Monitors: csi.MonEndpoints(c.ClusterInfo.Monitors)}); err != nil {
 		return errors.Wrap(err, "failed to update csi cluster config")
 	}
 
diff --git a/pkg/operator/ceph/csi/cluster_config.go b/pkg/operator/ceph/csi/cluster_config.go
index 7e665dbb8..6f241acb2 100644
--- a/pkg/operator/ceph/csi/cluster_config.go
+++ b/pkg/operator/ceph/csi/cluster_config.go
@@ -40,6 +40,7 @@ var (
 type CsiClusterConfigEntry struct {
 	ClusterID      string         `json:"clusterID"`
 	Monitors       []string       `json:"monitors"`
+	Namespace      string         `json:"namespace"`
 	CephFS         *CsiCephFSSpec `json:"cephFS,omitempty"`
 	RadosNamespace string         `json:"radosNamespace,omitempty"`
 }
@@ -112,6 +113,16 @@ func updateCsiClusterConfig(curr, clusterKey string, newCsiClusterConfigEntry *C
 	// a lock is acquired for the update operation. So concurrent updates (rare event) will block and
 	// wait for the other update to complete. Monitors and Subvolumegroup will be updated
 	// independently and won't collide.
+	if newCsiClusterConfigEntry != nil {
+		for i, centry := range cc {
+			// If the clusterID belongs to the same cluster, update the entry.
+			// update default clusterID's entry
+			if clusterKey == centry.Namespace {
+				centry.Monitors = newCsiClusterConfigEntry.Monitors
+				cc[i] = centry
+			}
+		}
+	}
 	for i, centry := range cc {
 		if centry.ClusterID == clusterKey {
 			// If the new entry is nil, this means the entry is being deleted so remove it from the list
@@ -137,6 +148,7 @@ func updateCsiClusterConfig(curr, clusterKey string, newCsiClusterConfigEntry *C
 		// will fail with a dangling pointer
 		if newCsiClusterConfigEntry != nil {
 			centry.ClusterID = clusterKey
+			centry.Namespace = newCsiClusterConfigEntry.Namespace
 			centry.Monitors = newCsiClusterConfigEntry.Monitors
 			// Add a condition not to fill with empty values
 			if newCsiClusterConfigEntry.CephFS != nil && newCsiClusterConfigEntry.CephFS.SubvolumeGroup != "" {
diff --git a/pkg/operator/ceph/file/subvolumegroup/controller.go b/pkg/operator/ceph/file/subvolumegroup/controller.go
index 2855ce3ef..fe3f1403f 100644
--- a/pkg/operator/ceph/file/subvolumegroup/controller.go
+++ b/pkg/operator/ceph/file/subvolumegroup/controller.go
@@ -265,7 +265,8 @@ func (r *ReconcileCephFilesystemSubVolumeGroup) updateClusterConfig(cephFilesyst
 	// If the mon endpoints change, the mon health check go routine will take care of updating the
 	// config map, so no special care is needed in this controller
 	csiClusterConfigEntry := csi.CsiClusterConfigEntry{
-		Monitors: csi.MonEndpoints(r.clusterInfo.Monitors),
+		Namespace: r.clusterInfo.Namespace,
+		Monitors:  csi.MonEndpoints(r.clusterInfo.Monitors),
 		CephFS: &csi.CsiCephFSSpec{
 			SubvolumeGroup: cephFilesystemSubVolumeGroup.Name,
 		},
diff --git a/pkg/operator/ceph/pool/radosnamespace/controller.go b/pkg/operator/ceph/pool/radosnamespace/controller.go
index c4bf66360..866a3b682 100644
--- a/pkg/operator/ceph/pool/radosnamespace/controller.go
+++ b/pkg/operator/ceph/pool/radosnamespace/controller.go
@@ -257,6 +257,7 @@ func (r *ReconcileCephBlockPoolRadosNamespace) updateClusterConfig(cephBlockPool
 	// If the mon endpoints change, the mon health check go routine will take care of updating the
 	// config map, so no special care is needed in this controller
 	csiClusterConfigEntry := csi.CsiClusterConfigEntry{
+		Namespace:      r.clusterInfo.Namespace,
 		Monitors:       csi.MonEndpoints(r.clusterInfo.Monitors),
 		RadosNamespace: cephBlockPoolRadosNamespace.Name,
 	}


Test patch:
diff --git a/pkg/operator/ceph/csi/cluster_config_test.go b/pkg/operator/ceph/csi/cluster_config_test.go
index 08c9e1cad..df974bae6 100644
--- a/pkg/operator/ceph/csi/cluster_config_test.go
+++ b/pkg/operator/ceph/csi/cluster_config_test.go
@@ -24,35 +24,39 @@ import (
 
 func TestUpdateCsiClusterConfig(t *testing.T) {
 	csiClusterConfigEntry := CsiClusterConfigEntry{
-		Monitors: []string{"1.2.3.4:5000"},
+		Namespace: "rook-ceph-1",
+		Monitors:  []string{"1.2.3.4:5000"},
 	}
 	csiClusterConfigEntry2 := CsiClusterConfigEntry{
-		Monitors: []string{"20.1.1.1:5000", "20.1.1.2:5000", "20.1.1.3:5000"},
+		Namespace: "rook-ceph-2",
+		Monitors:  []string{"20.1.1.1:5000", "20.1.1.2:5000", "20.1.1.3:5000"},
 	}
 	csiClusterConfigEntry3 := CsiClusterConfigEntry{
-		Monitors: []string{"10.1.1.1:5000", "10.1.1.2:5000", "10.1.1.3:5000"},
+		Namespace: "rook-ceph-3",
+		Monitors:  []string{"10.1.1.1:5000", "10.1.1.2:5000", "10.1.1.3:5000"},
 		CephFS: &CsiCephFSSpec{
 			SubvolumeGroup: "mygroup",
 		},
 	}
+
 	var s string
 	var err error
 
 	t.Run("add a simple mons list", func(t *testing.T) {
-		s, err = updateCsiClusterConfig("[]", "alpha", &csiClusterConfigEntry)
+		s, err = updateCsiClusterConfig("[]", "rook-ceph-1", &csiClusterConfigEntry)
 		assert.NoError(t, err)
 		assert.Equal(t, s,
-			`[{"clusterID":"alpha","monitors":["1.2.3.4:5000"]}]`)
+			`[{"clusterID":"rook-ceph-1","monitors":["1.2.3.4:5000"],"namespace":"rook-ceph-1"}]`)
 	})
 
 	t.Run("add a 2nd mon to the current cluster", func(t *testing.T) {
 		csiClusterConfigEntry.Monitors = append(csiClusterConfigEntry.Monitors, "10.11.12.13:5000")
-		s, err = updateCsiClusterConfig(s, "alpha", &csiClusterConfigEntry)
+		s, err = updateCsiClusterConfig(s, "rook-ceph-1", &csiClusterConfigEntry)
 		assert.NoError(t, err)
 		cc, err := parseCsiClusterConfig(s)
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(cc))
-		assert.Equal(t, "alpha", cc[0].ClusterID)
+		assert.Equal(t, "rook-ceph-1", cc[0].ClusterID)
 		assert.Contains(t, cc[0].Monitors, "1.2.3.4:5000")
 		assert.Contains(t, cc[0].Monitors, "10.11.12.13:5000")
 		assert.Equal(t, 2, len(cc[0].Monitors))
@@ -64,14 +68,18 @@ func TestUpdateCsiClusterConfig(t *testing.T) {
 		cc, err := parseCsiClusterConfig(s)
 		assert.NoError(t, err)
 		assert.Equal(t, 2, len(cc))
-		assert.Equal(t, "alpha", cc[0].ClusterID)
+		assert.Equal(t, "rook-ceph-1", cc[0].ClusterID)
 		assert.Contains(t, cc[0].Monitors, "1.2.3.4:5000")
 		assert.Contains(t, cc[0].Monitors, "10.11.12.13:5000")
+		// check 1st cluster contains any of the 3 mons from 2nd cluster
+		assert.NotContains(t, cc[0].Monitors, "20.1.1.1:5000")
 		assert.Equal(t, 2, len(cc[0].Monitors))
 		assert.Equal(t, "beta", cc[1].ClusterID)
 		assert.Contains(t, cc[1].Monitors, "20.1.1.1:5000")
 		assert.Contains(t, cc[1].Monitors, "20.1.1.2:5000")
 		assert.Contains(t, cc[1].Monitors, "20.1.1.3:5000")
+		// check 2nd cluster contains any of the mons from 1st cluster
+		assert.NotContains(t, cc[1].Monitors, "10.11.12.13:5000")
 		assert.Equal(t, 3, len(cc[1].Monitors))
 	})
 
@@ -84,7 +92,7 @@ func TestUpdateCsiClusterConfig(t *testing.T) {
 		cc, err := parseCsiClusterConfig(s)
 		assert.NoError(t, err)
 		assert.Equal(t, 2, len(cc))
-		assert.Equal(t, "alpha", cc[0].ClusterID)
+		assert.Equal(t, "rook-ceph-1", cc[0].ClusterID)
 		assert.Contains(t, cc[0].Monitors, "1.2.3.4:5000")
 		assert.Contains(t, cc[0].Monitors, "10.11.12.13:5000")
 		assert.Equal(t, 2, len(cc[0].Monitors))
@@ -100,7 +108,7 @@ func TestUpdateCsiClusterConfig(t *testing.T) {
 		cc, err := parseCsiClusterConfig(s)
 		assert.NoError(t, err)
 		assert.Equal(t, 3, len(cc))
-		assert.Equal(t, "alpha", cc[0].ClusterID)
+		assert.Equal(t, "rook-ceph-1", cc[0].ClusterID)
 		assert.Equal(t, 2, len(cc[0].Monitors))
 		assert.Equal(t, "beta", cc[1].ClusterID)
 		assert.Equal(t, "baba", cc[2].ClusterID)
@@ -157,4 +165,155 @@ func TestUpdateCsiClusterConfig(t *testing.T) {
 		_, err = updateCsiClusterConfig("qqq", "beta", &csiClusterConfigEntry2)
 		assert.Error(t, err)
 	})
+
+	t.Run("test mon IP's update across all clusterID's belong to same cluster", func(t *testing.T) {
+		clusterIDofCluster1 := "rook-ceph"
+		subvolGrpNameofCluster1 := "subvol-group"
+		radosNSofCluster1 := "rados-ns"
+
+		csiCluster1ConfigEntry := CsiClusterConfigEntry{
+			Namespace: clusterIDofCluster1,
+			Monitors:  []string{"1.2.3.4:5000"},
+		}
+		s, err := updateCsiClusterConfig("[]", clusterIDofCluster1, &csiCluster1ConfigEntry)
+		assert.NoError(t, err)
+		assert.Equal(t, s,
+			`[{"clusterID":"rook-ceph","monitors":["1.2.3.4:5000"],"namespace":"rook-ceph"}]`)
+		// add subvolumegroup to same cluster
+		subVolCsiCluster1Config := CsiClusterConfigEntry{
+			Namespace: clusterIDofCluster1,
+			Monitors:  csiCluster1ConfigEntry.Monitors,
+			CephFS: &CsiCephFSSpec{
+				SubvolumeGroup: subvolGrpNameofCluster1,
+			},
+		}
+		s, err = updateCsiClusterConfig(s, subvolGrpNameofCluster1, &subVolCsiCluster1Config)
+		assert.NoError(t, err)
+		cc, err := parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 2, len(cc), cc)
+		assert.Equal(t, 1, len(cc[1].Monitors))
+		assert.Equal(t, subvolGrpNameofCluster1, cc[1].CephFS.SubvolumeGroup, cc)
+
+		// add rados to same cluster
+		radosNsCsiCluster1Config := CsiClusterConfigEntry{
+			Namespace:      clusterIDofCluster1,
+			Monitors:       csiCluster1ConfigEntry.Monitors,
+			RadosNamespace: radosNSofCluster1,
+		}
+		s, err = updateCsiClusterConfig(s, radosNSofCluster1, &radosNsCsiCluster1Config)
+		assert.NoError(t, err)
+		cc, err = parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 3, len(cc), cc)
+		assert.Equal(t, 1, len(cc[2].Monitors))
+		assert.Equal(t, radosNSofCluster1, cc[2].RadosNamespace, cc)
+
+		// update mon IP's and check is it updating for all clusterID's
+		csiCluster1ConfigEntry.Monitors = append(csiCluster1ConfigEntry.Monitors, "1.2.3.10:5000")
+		s, err = updateCsiClusterConfig(s, clusterIDofCluster1, &csiCluster1ConfigEntry)
+		assert.NoError(t, err)
+		cc, err = parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 3, len(cc), cc)
+		// check mon entries are updated in all configmap
+		for _, i := range cc {
+			assert.Equal(t, 2, len(i.Monitors))
+		}
+
+		clusterIDofCluster2 := "rook-ceph-2"
+		subvolGrpNameofCluster2 := "subvol-group-2"
+		radosNSofCluster2 := "rados-ns-2"
+
+		cluster2Mons := []string{"192.168.0.2:5000"}
+		csiCluster2ConfigEntry := CsiClusterConfigEntry{
+			Namespace: clusterIDofCluster2,
+			Monitors:  cluster2Mons,
+		}
+		subVolCsiCluster2Config := CsiClusterConfigEntry{
+			Namespace: clusterIDofCluster2,
+			Monitors:  cluster2Mons,
+			CephFS: &CsiCephFSSpec{
+				SubvolumeGroup: subvolGrpNameofCluster2,
+			},
+		}
+		radosNsCsiCluster2Config := CsiClusterConfigEntry{
+			Namespace:      clusterIDofCluster2,
+			Monitors:       cluster2Mons,
+			RadosNamespace: radosNSofCluster2,
+		}
+		s, err = updateCsiClusterConfig(s, clusterIDofCluster2, &csiCluster2ConfigEntry)
+		assert.NoError(t, err)
+		s, err = updateCsiClusterConfig(s, subvolGrpNameofCluster2, &subVolCsiCluster2Config)
+		assert.NoError(t, err)
+		s, err = updateCsiClusterConfig(s, radosNSofCluster2, &radosNsCsiCluster2Config)
+		assert.NoError(t, err)
+		cc, err = parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 6, len(cc), cc)
+		// check mon entries of 1st cluster amd mon overlapping with 2nd cluster
+		for i := 0; i < 3; i++ {
+			assert.Equal(t, 2, len(cc[i].Monitors))
+			assert.False(t, contains(cc[i].Monitors, cluster2Mons))
+
+		}
+		// check mon entries of 2nd cluster
+		for i := 3; i < 6; i++ {
+			assert.Equal(t, 1, len(cc[i].Monitors))
+			assert.False(t, contains(cc[i].Monitors, csiCluster1ConfigEntry.Monitors))
+		}
+		// update mon on 2nd cluster and check is it updating for all clusterID's
+		// of 2nd cluster
+		csiCluster2ConfigEntry.Monitors = append(csiCluster2ConfigEntry.Monitors, "192.168.0.3:5000")
+		s, err = updateCsiClusterConfig(s, clusterIDofCluster2, &csiCluster2ConfigEntry)
+		assert.NoError(t, err)
+		cc, err = parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 6, len(cc), cc)
+		// check mon entries are updated
+		for _, i := range cc {
+			assert.Equal(t, 2, len(i.Monitors))
+		}
+		// check for overlapping
+		// check mon entries of 1st cluster amd mon overlapping with 2nd cluster
+		for i := 0; i < 3; i++ {
+			assert.False(t, contains(cc[i].Monitors, cluster2Mons))
+		}
+		// check mon entries of 2nd cluster
+		for i := 3; i < 6; i++ {
+			assert.False(t, contains(cc[i].Monitors, csiCluster1ConfigEntry.Monitors))
+		}
+		// Remove one mon from 2nd cluster and check is it updating for all
+		// clusterID's of 2nd cluster
+		i := 1
+		csiCluster2ConfigEntry.Monitors = append(csiCluster2ConfigEntry.Monitors[:i], csiCluster2ConfigEntry.Monitors[i+1:]...)
+		s, err = updateCsiClusterConfig(s, clusterIDofCluster2, &csiCluster2ConfigEntry)
+		assert.NoError(t, err)
+		cc, err = parseCsiClusterConfig(s)
+		assert.NoError(t, err)
+		assert.Equal(t, 6, len(cc), cc)
+		// check for overlapping
+		// check mon entries of 1st cluster amd mon overlapping with 2nd cluster
+		for i := 0; i < 3; i++ {
+			assert.False(t, contains(cc[i].Monitors, cluster2Mons))
+			assert.Equal(t, 2, len(cc[i].Monitors))
+		}
+		// check mon entries of 2nd cluster
+		for i := 3; i < 6; i++ {
+			assert.False(t, contains(cc[i].Monitors, csiCluster1ConfigEntry.Monitors))
+			assert.Equal(t, 1, len(cc[i].Monitors))
+		}
+	})
+}
+
+func contains(src, dest []string) bool {
+	for _, s := range src {
+		for _, d := range dest {
+			if s == d {
+				return true
+			}
+		}
+	}
+
+	return false
 }