From aa61ddb57e4e5e1d5f29d5d89beb690494bef3b9 Mon Sep 17 00:00:00 2001 From: Travis Nielsen Date: Wed, 25 Jun 2025 14:38:33 -0600 Subject: [PATCH] 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 --- pkg/operator/ceph/cluster/mon/health.go | 30 ++++++++++++++++---- pkg/operator/ceph/cluster/mon/health_test.go | 28 ++++++++++++++++++ pkg/operator/ceph/cluster/mon/mon.go | 8 +++--- pkg/operator/ceph/cluster/mon/mon_test.go | 3 +- 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/pkg/operator/ceph/cluster/mon/health.go b/pkg/operator/ceph/cluster/mon/health.go index fe1042ae8..44142ac55 100644 --- a/pkg/operator/ceph/cluster/mon/health.go +++ b/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) diff --git a/pkg/operator/ceph/cluster/mon/health_test.go b/pkg/operator/ceph/cluster/mon/health_test.go index 10196d129..3e462a225 100644 --- a/pkg/operator/ceph/cluster/mon/health_test.go +++ b/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) diff --git a/pkg/operator/ceph/cluster/mon/mon.go b/pkg/operator/ceph/cluster/mon/mon.go index b16941627..bc43bb8d2 100644 --- a/pkg/operator/ceph/cluster/mon/mon.go +++ b/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 } diff --git a/pkg/operator/ceph/cluster/mon/mon_test.go b/pkg/operator/ceph/cluster/mon/mon_test.go index 98b6d8d77..6484d2211 100644 --- a/pkg/operator/ceph/cluster/mon/mon_test.go +++ b/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{}, } }