From 90bec3120367f71ce9a64b5a121702ed359133f0 Mon Sep 17 00:00:00 2001 From: travisn Date: Wed, 14 Feb 2024 16:46:22 -0700 Subject: [PATCH] pool: skip crush rule update when not needed The crush rule will be updated when the failure domain changes. This update can be skipped when the expected crush rule is already configured. Otherwise, the log has a confusing message that makes it appear that the crush rule was updated when in fact it was not. Signed-off-by: travisn --- pkg/daemon/ceph/client/pool.go | 19 ++++++++++++------- pkg/daemon/ceph/client/pool_test.go | 7 +++++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/daemon/ceph/client/pool.go b/pkg/daemon/ceph/client/pool.go index 57eca948f..317d316fb 100644 --- a/pkg/daemon/ceph/client/pool.go +++ b/pkg/daemon/ceph/client/pool.go @@ -510,18 +510,23 @@ func updatePoolCrushRule(context *clusterd.Context, clusterInfo *ClusterInfo, cl return nil } - if currentFailureDomain != pool.FailureDomain { - logger.Infof("creating a new crush rule for changed failure domain on crush rule %q", details.CrushRule) - } - if currentDeviceClass != pool.DeviceClass { - logger.Infof("creating a new crush rule for changed deviceClass on crush rule %q", details.CrushRule) - } - // Use a crush rule name that is unique to the desired failure domain crushRuleName := fmt.Sprintf("%s_%s", pool.Name, pool.FailureDomain) if pool.DeviceClass != "" { crushRuleName = fmt.Sprintf("%s_%s_%s", pool.Name, pool.FailureDomain, pool.DeviceClass) } + if crushRuleName == details.CrushRule { + logger.Debugf("crush rule already set to %q for pool %q (failureDomain=%s, deviceClass=%s)", crushRuleName, pool.Name, currentFailureDomain, currentDeviceClass) + return nil + } + + if currentFailureDomain != pool.FailureDomain { + logger.Infof("creating a new crush rule for changed failure domain (%q-->%q) on crush rule %q", currentFailureDomain, pool.FailureDomain, details.CrushRule) + } + if currentDeviceClass != pool.DeviceClass { + logger.Infof("creating a new crush rule for changed deviceClass (%q-->%q) on crush rule %q", currentDeviceClass, pool.DeviceClass, details.CrushRule) + } + logger.Infof("updating pool %q failure domain from %q to %q with new crush rule %q", pool.Name, currentFailureDomain, pool.FailureDomain, crushRuleName) logger.Infof("crush rule %q will no longer be used by pool %q", details.CrushRule, pool.Name) diff --git a/pkg/daemon/ceph/client/pool_test.go b/pkg/daemon/ceph/client/pool_test.go index 0dda84de6..b2fa1add2 100644 --- a/pkg/daemon/ceph/client/pool_test.go +++ b/pkg/daemon/ceph/client/pool_test.go @@ -249,6 +249,8 @@ func testCreateReplicaPool(t *testing.T, failureDomain, crushRoot, deviceClass, func TestUpdateFailureDomain(t *testing.T) { var newCrushRule string currentFailureDomain := "rack" + currentDeviceClass := "default" + testCrushRuleName := "test_rule" executor := &exectest.MockExecutor{} context := &clusterd.Context{Executor: executor} executor.MockExecuteCommandWithOutput = func(command string, args ...string) (string, error) { @@ -256,7 +258,7 @@ func TestUpdateFailureDomain(t *testing.T) { if args[1] == "pool" { if args[2] == "get" { assert.Equal(t, "mypool", args[3]) - return `{"crush_rule": "test_rule"}`, nil + return fmt.Sprintf(`{"crush_rule": "%s"}`, testCrushRuleName), nil } if args[2] == "set" { assert.Equal(t, "mypool", args[3]) @@ -267,7 +269,7 @@ func TestUpdateFailureDomain(t *testing.T) { } if args[1] == "crush" { if args[2] == "rule" && args[3] == "dump" { - return fmt.Sprintf(`{"steps": [{"foo":"bar"},{"type":"%s"}]}`, currentFailureDomain), nil + return fmt.Sprintf(`{"steps": [{"item_name":"%s"},{"type":"%s"}]}`, currentDeviceClass, currentFailureDomain), nil } newCrushRule = "foo" return "", nil @@ -296,6 +298,7 @@ func TestUpdateFailureDomain(t *testing.T) { Replicated: cephv1.ReplicatedSpec{Size: 3}, }, } + testCrushRuleName = "mypool_rack" clusterSpec := &cephv1.ClusterSpec{Storage: cephv1.StorageScopeSpec{}} err := updatePoolCrushRule(context, AdminTestClusterInfo("mycluster"), clusterSpec, p) assert.NoError(t, err)