From e2cadab6fe10e7d2c66a9f2981737477cc9302db Mon Sep 17 00:00:00 2001
From: Travis Nielsen <tnielsen@redhat.com>
Date: Fri, 4 Oct 2024 14:23:05 -0600
Subject: [PATCH] mon: do not remove extra mon in middle of failover

If the mon failover is in progress, ensure the removal
of an extra mon deployment is skipped since that code
path only has one mon in the list for the mon that was
just newly started. The extra mon was erroneously removing
a random mon in that case, followed immediately by the mon
failover completing and removing the expected failed mon,
and potentially causing mon quroum loss.

Signed-off-by: Travis Nielsen <tnielsen@redhat.com>
---
 pkg/operator/ceph/cluster/mon/mon.go      |  9 +++++++++
 pkg/operator/ceph/cluster/mon/mon_test.go | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/pkg/operator/ceph/cluster/mon/mon.go b/pkg/operator/ceph/cluster/mon/mon.go
index 17258a532..8375163e8 100644
--- a/pkg/operator/ceph/cluster/mon/mon.go
+++ b/pkg/operator/ceph/cluster/mon/mon.go
@@ -1068,7 +1068,16 @@ func (c *Cluster) startDeployments(mons []*monConfig, requireAllInQuorum bool) e
 }
 
 func (c *Cluster) checkForExtraMonResources(mons []*monConfig, deployments []apps.Deployment) string {
+	// If there are fewer mon deployments than the desired count, no need to remove an extra.
 	if len(deployments) <= c.spec.Mon.Count || len(deployments) <= len(mons) {
+		logger.Debug("no extra mon deployments to remove")
+		return ""
+	}
+	// If there are fewer mons in the list than expected, either new mons are being created for
+	// a new cluster, or a mon failover is in progress and the list of mons only
+	// includes the single mon that was just started
+	if len(mons) < c.spec.Mon.Count {
+		logger.Debug("new cluster or mon failover in progress, not checking for extra mon deployments")
 		return ""
 	}
 
diff --git a/pkg/operator/ceph/cluster/mon/mon_test.go b/pkg/operator/ceph/cluster/mon/mon_test.go
index e5918745a..b4c5e0f5b 100644
--- a/pkg/operator/ceph/cluster/mon/mon_test.go
+++ b/pkg/operator/ceph/cluster/mon/mon_test.go
@@ -718,6 +718,24 @@ func TestRemoveExtraMonDeployments(t *testing.T) {
 	c.spec.Mon.Count = 3
 	removed = c.checkForExtraMonResources(mons, deployments)
 	assert.Equal(t, "", removed)
+
+	// Do not remove a mon when it was during failover and only a single mon is in the list, even if extra deployments exist
+	mons = []*monConfig{
+		{ResourceName: "rook-ceph-mon-d", DaemonName: "d"},
+	}
+	deployments = append(deployments, apps.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "rook-ceph-mon-c",
+			Labels: map[string]string{"ceph_daemon_id": "c"},
+		}})
+	deployments = append(deployments, apps.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "rook-ceph-mon-d",
+			Labels: map[string]string{"ceph_daemon_id": "d"},
+		}})
+	c.spec.Mon.Count = 3
+	removed = c.checkForExtraMonResources(mons, deployments)
+	assert.Equal(t, "", removed)
 }
 
 func TestStretchMonVolumeClaimTemplate(t *testing.T) {