From bccc84efa2c88644505d2763398e99d99cb19f8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Han?= Date: Fri, 29 Oct 2021 09:30:08 +0200 Subject: [PATCH] mgr: do not set the balancer mode on pacific MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Pacific, Ceph's default is "upmap", so we should let it be like this. This lets the user change the mode is desired. On Octopus though, Rook continues to force the mode to "upmap". Closes: https://github.com/rook/rook/issues/9062 Signed-off-by: Sébastien Han --- pkg/operator/ceph/cluster/mgr/mgr.go | 14 +++-- pkg/operator/ceph/cluster/mgr/mgr_test.go | 67 +++++++++++++++++++++-- 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/pkg/operator/ceph/cluster/mgr/mgr.go b/pkg/operator/ceph/cluster/mgr/mgr.go index 061e4f34913c..518f87109736 100644 --- a/pkg/operator/ceph/cluster/mgr/mgr.go +++ b/pkg/operator/ceph/cluster/mgr/mgr.go @@ -361,14 +361,18 @@ func (c *Cluster) enableCrashModule() error { func (c *Cluster) enableBalancerModule() error { // The order MATTERS, always configure this module first, then turn it on - // This sets min compat client to luminous and the balancer module mode - err := cephclient.ConfigureBalancerModule(c.context, c.clusterInfo, balancerModuleMode) - if err != nil { - return errors.Wrapf(err, "failed to configure module %q", balancerModuleName) + // This enables the balancer module mode only in versions older than Pacific + // This let's the user change the default mode if desired + if !c.clusterInfo.CephVersion.IsAtLeastPacific() { + // This sets min compat client to luminous and the balancer module mode + err := cephclient.ConfigureBalancerModule(c.context, c.clusterInfo, balancerModuleMode) + if err != nil { + return errors.Wrapf(err, "failed to configure module %q", balancerModuleName) + } } // This turns "on" the balancer - err = cephclient.MgrEnableModule(c.context, c.clusterInfo, balancerModuleName, false) + err := cephclient.MgrEnableModule(c.context, c.clusterInfo, balancerModuleName, false) if err != nil { return errors.Wrapf(err, "failed to turn on mgr %q module", balancerModuleName) } diff --git a/pkg/operator/ceph/cluster/mgr/mgr_test.go b/pkg/operator/ceph/cluster/mgr/mgr_test.go index c0f122c65865..d4ad1ddc372f 100644 --- a/pkg/operator/ceph/cluster/mgr/mgr_test.go +++ b/pkg/operator/ceph/cluster/mgr/mgr_test.go @@ -24,14 +24,13 @@ import ( "testing" "time" + "github.com/pkg/errors" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1" "github.com/rook/rook/pkg/client/clientset/versioned/scheme" "github.com/rook/rook/pkg/clusterd" cephclient "github.com/rook/rook/pkg/daemon/ceph/client" cephver "github.com/rook/rook/pkg/operator/ceph/version" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - - monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" testopk8s "github.com/rook/rook/pkg/operator/k8sutil/test" testop "github.com/rook/rook/pkg/operator/test" exectest "github.com/rook/rook/pkg/util/exec/test" @@ -40,8 +39,9 @@ import ( apps "k8s.io/api/apps/v1" policyv1 "k8s.io/api/policy/v1" policyv1beta1 "k8s.io/api/policy/v1beta1" - "k8s.io/apimachinery/pkg/api/errors" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestStartMgr(t *testing.T) { @@ -172,7 +172,7 @@ func validateServices(t *testing.T, c *Cluster) { assert.Equal(t, ds.Spec.Ports[0].Port, int32(c.spec.Dashboard.Port)) } } else { - assert.True(t, errors.IsNotFound(err)) + assert.True(t, kerrors.IsNotFound(err)) } } @@ -227,7 +227,7 @@ func TestMgrSidecarReconcile(t *testing.T) { assert.True(t, calledMgrStat) assert.False(t, calledMgrDump) _, err = c.context.Clientset.CoreV1().Services(c.clusterInfo.Namespace).Get(context.TODO(), "rook-ceph-mgr", metav1.GetOptions{}) - assert.True(t, errors.IsNotFound(err)) + assert.True(t, kerrors.IsNotFound(err)) // nothing is updated when the requested mgr is not the active mgr activeMgr = "b" @@ -378,3 +378,58 @@ func TestApplyMonitoringLabels(t *testing.T) { applyMonitoringLabels(c, sm) assert.Nil(t, sm.Spec.Endpoints[0].RelabelConfigs) } + +func TestCluster_enableBalancerModule(t *testing.T) { + c := &Cluster{ + context: &clusterd.Context{Executor: &exectest.MockExecutor{}, Clientset: testop.New(t, 3)}, + clusterInfo: cephclient.AdminClusterInfo("mycluster"), + } + + t.Run("on octopus we configure the balancer AND enable the upmap mode", func(t *testing.T) { + c.clusterInfo.CephVersion = cephver.Octopus + executor := &exectest.MockExecutor{ + MockExecuteCommandWithOutput: func(command string, args ...string) (string, error) { + logger.Infof("Command: %s %v", command, args) + if command == "ceph" { + if args[0] == "osd" && args[1] == "set-require-min-compat-client" { + return "", nil + } + if args[0] == "balancer" && args[1] == "mode" { + return "", nil + } + if args[0] == "balancer" && args[1] == "on" { + return "", nil + } + } + return "", errors.New("unknown command") + }, + } + c.context.Executor = executor + err := c.enableBalancerModule() + assert.NoError(t, err) + }) + + t.Run("on pacific we configure the balancer ONLY and don't set a mode", func(t *testing.T) { + c.clusterInfo.CephVersion = cephver.Pacific + executor := &exectest.MockExecutor{ + MockExecuteCommandWithOutput: func(command string, args ...string) (string, error) { + logger.Infof("Command: %s %v", command, args) + if command == "ceph" { + if args[0] == "osd" && args[1] == "set-require-min-compat-client" { + return "", nil + } + if args[0] == "balancer" && args[1] == "mode" { + return "", errors.New("balancer mode must not be set") + } + if args[0] == "balancer" && args[1] == "on" { + return "", nil + } + } + return "", errors.New("unknown command") + }, + } + c.context.Executor = executor + err := c.enableBalancerModule() + assert.NoError(t, err) + }) +}