mon: failover with host network must use different node

When a mon is failed over, the old mon is left in mon quorum
until the new quorum is confirmed in quorum. When host networking
is enabled, this may result in two mons being in quorum with the
same ip, if the new mon is scheduled on the same node as the
down mon. This results in daemon crashes in the cluster since
they are confused by multiple of the same ip in the mon map.

The solution is to avoid scaling down the bad mon when host networking
is enabled. This will ensure the new mon is not scheduled on the
same node as the previous mon. With host networking, either another
node is necessary or the original mon must come back online to
restore quorum.

Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

pkg/operator/ceph/cluster/mon/health.go

@@ -372,10 +372,10 @@ func (c *Cluster) checkHealth(ctx context.Context) error {
 
 	// failover any mons present in the mon fail over list
 	for _, mon := range c.ClusterInfo.InternalMonitors {
-		if c.monsToFailover.Has(mon.Name) {
+		if _, ok := c.monsToFailover[mon.Name]; ok {
 			logger.Infof("fail over mon %q from the mon fail over list", mon.Name)
 			c.failMon(len(c.ClusterInfo.InternalMonitors), desiredMonCount, mon.Name)
-			c.monsToFailover.Delete(mon.Name)
+			delete(c.monsToFailover, mon.Name)
 			return nil
 		}
 	}
@@ -710,9 +710,11 @@ func (c *Cluster) failoverMon(name string) error {
 	logger.Infof("starting new mon: %+v", m)
 
 	// Scale down the failed mon to allow a new one to start
-	if err := c.updateMonDeploymentReplica(name, false); err != nil {
-		// attempt to continue with the failover even if the bad mon could not be stopped
-		logger.Warningf("failed to stop mon %q for failover. %v", name, err)
+	if c.stopMonDuringFailover(name) {
+		if err := c.updateMonDeploymentReplica(name, false); err != nil {
+			// attempt to continue with the failover even if the bad mon could not be stopped
+			logger.Warningf("failed to stop mon %q for failover. %v", name, err)
+		}
 	}
 
 	// If the mon failover is not successful, revert the failover
@@ -794,6 +796,24 @@ func (c *Cluster) failoverMon(name string) error {
 	return c.removeMon(name)
 }
 
+func (c *Cluster) stopMonDuringFailover(name string) bool {
+	if !c.spec.Network.IsHost() {
+		return true
+	}
+
+	// If the mon is using host networking, we don't want to stop the mon
+	// since we don't want to schedule a new mon with the same host ip.
+	// With host networking, when failing over the mon, we either require
+	// another eligible node without a mon, or we expect the down mon to come back online.
+	// But if the host networking settings are being enabled, we can allow the failover from a
+	// non-host networking mon.
+	if mon, ok := c.monsToFailover[name]; ok && !mon.UseHostNetwork {
+		return true
+	}
+	logger.Infof("skipping stopping mon %q during failover, since with host networking a new mon cannot be started on the same node", name)
+	return false
+}
+
 // make a best effort to remove the mon and all its resources
 func (c *Cluster) removeMon(daemonName string) error {
 	return c.removeMonWithOptionalQuorum(daemonName, true)

pkg/operator/ceph/cluster/mon/health_test.go

@@ -300,6 +300,34 @@ func TestEvictMonOnSameNode(t *testing.T) {
 	assert.Equal(t, 3, c.maxMonID)
 }
 
+func TestHostNetworkFailover(t *testing.T) {
+	ctx := context.TODO()
+	context := &clusterd.Context{}
+	ownerInfo := cephclient.NewMinimumOwnerInfoWithOwnerRef()
+	c := New(ctx, context, "ns", cephv1.ClusterSpec{}, ownerInfo)
+
+	t.Run("should stop mon on default network", func(t *testing.T) {
+		assert.True(t, c.stopMonDuringFailover("a"))
+	})
+
+	t.Run("should not stop mon on host network", func(t *testing.T) {
+		c.spec.Network.Provider = "host"
+		assert.False(t, c.stopMonDuringFailover("a"))
+	})
+
+	t.Run("should stop mon converting to host network", func(t *testing.T) {
+		c.spec.Network.Provider = "host"
+		c.monsToFailover["a"] = &monConfig{UseHostNetwork: false}
+		assert.True(t, c.stopMonDuringFailover("a"))
+	})
+
+	t.Run("should stop mon converting from host network", func(t *testing.T) {
+		c.spec.Network.Provider = ""
+		c.monsToFailover["a"] = &monConfig{UseHostNetwork: true}
+		assert.True(t, c.stopMonDuringFailover("a"))
+	})
+}
+
 func createTestMonPod(t *testing.T, clientset kubernetes.Interface, c *Cluster, name, node string) {
 	m := &monConfig{ResourceName: resourceName(name), DaemonName: name, DataPathMap: &config.DataPathMap{}}
 	d, err := c.makeDeployment(m, false)

pkg/operator/ceph/cluster/mon/mon.go

@@ -132,7 +132,7 @@ type Cluster struct {
 	isUpgrade          bool
 	arbiterMon         string
 	// list of mons to be failed over
-	monsToFailover sets.Set[string]
+	monsToFailover map[string]*monConfig
 }
 
 // monConfig for a single monitor
@@ -180,7 +180,7 @@ func New(ctx context.Context, clusterdContext *clusterd.Context, namespace strin
 		ClusterInfo: &cephclient.ClusterInfo{
 			Context: ctx,
 		},
-		monsToFailover: sets.New[string](),
+		monsToFailover: map[string]*monConfig{},
 	}
 }
 
@@ -1557,13 +1557,13 @@ func (c *Cluster) startMon(m *monConfig, schedule *controller.MonScheduleInfo) e
 	if deploymentExists {
 		// skip update if mon path has changed
 		if hasMonPathChanged(existingDeployment, c.spec.Mon.VolumeClaimTemplate.ToPVC()) {
-			c.monsToFailover.Insert(m.DaemonName)
+			c.monsToFailover[m.DaemonName] = m
 			return nil
 		}
 
 		// skip update if mon fail over is required due to change in hostnetwork settings
 		if isMonIPUpdateRequiredForHostNetwork(m.DaemonName, m.UseHostNetwork, &c.spec.Network) {
-			c.monsToFailover.Insert(m.DaemonName)
+			c.monsToFailover[m.DaemonName] = m
 			return nil
 		}
 

pkg/operator/ceph/cluster/mon/mon_test.go

@@ -45,7 +45,6 @@ import (
 	discoveryv1 "k8s.io/api/discovery/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
 )
 
@@ -124,7 +123,7 @@ func newCluster(context *clusterd.Context, namespace string, allowMultiplePerNod
 			Schedule: map[string]*opcontroller.MonScheduleInfo{},
 		},
 		ownerInfo:      ownerInfo,
-		monsToFailover: sets.New[string](),
+		monsToFailover: map[string]*monConfig{},
 	}
 }