From fe3623997abe3a7d7d15b382562116b3e15d183e Mon Sep 17 00:00:00 2001 From: Peter Goron Date: Fri, 18 Aug 2023 11:00:19 +0200 Subject: [PATCH] pool: fix multiple bugs on setPgNumMinMaxProperties (#12) unit test coverage extended Co-authored-by: Peter Goron --- pkg/daemon/ceph/client/pool.go | 102 ++++++---- pkg/daemon/ceph/client/pool_test.go | 257 ++++++++++++++++++++++-- pkg/operator/ceph/object/objectstore.go | 9 +- 3 files changed, 307 insertions(+), 61 deletions(-) diff --git a/pkg/daemon/ceph/client/pool.go b/pkg/daemon/ceph/client/pool.go index be3b5224e666..d67f57f08111 100644 --- a/pkg/daemon/ceph/client/pool.go +++ b/pkg/daemon/ceph/client/pool.go @@ -35,8 +35,13 @@ const ( reallyConfirmFlag = "--yes-i-really-really-mean-it" targetSizeRatioProperty = "target_size_ratio" CompressionModeProperty = "compression_mode" + PgNumProperty = "pg_num" + PgpNumProperty = "pgp_num" + PgNumMinProperty = "pg_num_min" + PgNumMaxProperty = "pg_num_max" PgAutoscaleModeProperty = "pg_autoscale_mode" PgAutoscaleModeOn = "on" + PgAutoscaleModeOff = "off" ) type CephStoragePoolSummary struct { @@ -191,6 +196,23 @@ func CreatePoolWithPGs(context *clusterd.Context, clusterInfo *ClusterInfo, clus if pool.Name == "" { return errors.New("pool name must be specified") } + + // honor pg_num_min if provided by user + if pgNumMinStr, found := pool.Parameters[PgNumMinProperty]; found { + pgNumMin, err := strconv.Atoi(pgNumMinStr) + if err != nil { + return errors.Errorf("failed to parse pg_num_min parameter of pool %q. %v", pool.Name, err) + } + pgNum, err := strconv.Atoi(pgCount) + if err != nil { + return errors.Errorf("failed to parse pgCount of pool %q. %v", pool.Name, err) + } + + if pgNum < pgNumMin { + pgCount = strconv.Itoa(pgNumMin) + } + } + if pool.IsReplicated() { return createReplicatedPoolForApp(context, clusterInfo, clusterSpec, pool, pgCount, appName) } @@ -285,14 +307,18 @@ func givePoolAppTag(context *clusterd.Context, clusterInfo *ClusterInfo, poolNam return nil } -func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInfo, pool cephv1.NamedPoolSpec, poolDetails *CephStoragePoolDetails) error { - needToRestoreAutoScaler := ("on" == poolDetails.PgAutoScaleMode) +func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInfo, pool cephv1.NamedPoolSpec) error { + poolDetails, err := GetPoolDetails(context, clusterInfo, pool.Name) + if err != nil { + return err + } + + needToRestoreAutoScaler := (PgAutoscaleModeOn == poolDetails.PgAutoScaleMode) curPgNum := poolDetails.PgNum curPgNumMin := -1 curPgNumMax := -1 - wantedPgNumMin := -1 - wantedPgNumMax := -1 - var err error + var wantedPgNumMin int + var wantedPgNumMax int if poolDetails.PgNumMin != nil { curPgNumMin = (*poolDetails.PgNumMin) @@ -302,27 +328,29 @@ func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInf } // parse pg_num_min and ensure consistency with pg_num_max if already set - if wantedPgNumMinStr, found := pool.Parameters["pg_num_min"]; found { + if wantedPgNumMinStr, found := pool.Parameters[PgNumMinProperty]; found { wantedPgNumMin, err = strconv.Atoi(wantedPgNumMinStr) if err != nil { logger.Errorf("failed to parse pg_num_min parameter of pool %q. %v", pool.Name, err) wantedPgNumMin = curPgNumMin } - if curPgNumMax != -1 && wantedPgNumMin > curPgNumMax { - return errors.Errorf("wanted pg_num_min (%d) can't be greater than current pg_num_max (%d) for pool %q", wantedPgNumMin, curPgNumMax, pool.Name) - } + } else { + wantedPgNumMin = curPgNumMin } // parse pg_num_max and ensure consistency with pg_num_min if already set - if wantedPgNumMaxStr, found := pool.Parameters["pg_num_max"]; found { + if wantedPgNumMaxStr, found := pool.Parameters[PgNumMaxProperty]; found { wantedPgNumMax, err = strconv.Atoi(wantedPgNumMaxStr) if err != nil { logger.Errorf("failed to parse pg_num_max parameter of pool %q. %v", pool.Name, err) wantedPgNumMax = curPgNumMax } - if curPgNumMin != -1 && wantedPgNumMax < curPgNumMin { - return errors.Errorf("wanted pg_num_max (%d) can't be less than current pg_num_min (%d) for pool %q", wantedPgNumMax, curPgNumMin, pool.Name) - } + } else { + wantedPgNumMax = curPgNumMax + } + + if wantedPgNumMin != -1 && wantedPgNumMax != -1 && wantedPgNumMin > wantedPgNumMax { + return errors.Errorf("pg_num_min (%d) can't be greater than pg_num_max (%d) for pool %q", wantedPgNumMin, wantedPgNumMax, pool.Name) } if wantedPgNumMin == curPgNumMin && wantedPgNumMax == curPgNumMax { @@ -337,8 +365,8 @@ func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInf // restore pg autoscaler if disabled during adjustment of pg_num_min & pg_num_max defer func() { - if poolDetails.PgAutoScaleMode == "off" && needToRestoreAutoScaler { - SetPoolProperty(context, clusterInfo, pool.Name, "pg_autoscale_mode", "on") + if poolDetails.PgAutoScaleMode == PgAutoscaleModeOff && needToRestoreAutoScaler { + SetPoolProperty(context, clusterInfo, pool.Name, PgAutoscaleModeProperty, PgAutoscaleModeOn) } }() @@ -346,68 +374,61 @@ func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInf if wantedPgNumMin > int(curPgNum) { // number of PG is below expected pg_num_min, we must increase pg(p)_num before adjusting pg_num_min // pg_autoscale_mode need to be disabled during the operation to make process reliable - if poolDetails.PgAutoScaleMode == "on" { - err = SetPoolProperty(context, clusterInfo, pool.Name, "pg_autoscale_mode", "off") + if poolDetails.PgAutoScaleMode == PgAutoscaleModeOn { + err = SetPoolProperty(context, clusterInfo, pool.Name, PgAutoscaleModeProperty, PgAutoscaleModeOff) if err != nil { return errors.Wrapf(err, "failed to disable pg autoscaler on pool %q before applying pg_num_min", pool.Name) } - poolDetails.PgAutoScaleMode = "off" + poolDetails.PgAutoScaleMode = PgAutoscaleModeOff } - err = SetPoolProperty(context, clusterInfo, pool.Name, "pg_num", strconv.Itoa(wantedPgNumMin)) + err = SetPoolProperty(context, clusterInfo, pool.Name, PgNumProperty, strconv.Itoa(wantedPgNumMin)) if err != nil { return errors.Wrapf(err, "failed to align pg_num to pg_num_min for pool %q", pool.Name) } curPgNum = uint(wantedPgNumMin) - poolDetails.PgNum = curPgNum - err = SetPoolProperty(context, clusterInfo, pool.Name, "pgp_num", strconv.Itoa(wantedPgNumMin)) + err = SetPoolProperty(context, clusterInfo, pool.Name, PgpNumProperty, strconv.Itoa(wantedPgNumMin)) if err != nil { return errors.Wrapf(err, "failed to align pgp_num to pg_num_min for pool %q", pool.Name) } - poolDetails.PgpNum = uint(wantedPgNumMin) } - err := SetPoolProperty(context, clusterInfo, pool.Name, "pg_num_min", strconv.Itoa(wantedPgNumMin)) + err := SetPoolProperty(context, clusterInfo, pool.Name, PgNumMinProperty, strconv.Itoa(wantedPgNumMin)) if err != nil { return errors.Wrapf(err, "failed to set pg_num_min quota for pool %q", pool.Name) } curPgNumMin = wantedPgNumMin - poolDetails.PgNumMin = &curPgNumMin } if wantedPgNumMax != curPgNumMax { if wantedPgNumMax < int(curPgNum) { // number of PG is above expected pg_num_max, we must decrease pg(p)_num before adjusting pg_num_max // pg_autoscale_mode need to be disabled during the operation to make process reliable - if poolDetails.PgAutoScaleMode == "on" { - err = SetPoolProperty(context, clusterInfo, pool.Name, "pg_autoscale_mode", "off") + if poolDetails.PgAutoScaleMode == PgAutoscaleModeOn { + err = SetPoolProperty(context, clusterInfo, pool.Name, PgAutoscaleModeProperty, PgAutoscaleModeOff) if err != nil { return errors.Wrapf(err, "failed to disable pg autoscaler on pool %q before applying pg_num_max", pool.Name) } - poolDetails.PgAutoScaleMode = "off" + poolDetails.PgAutoScaleMode = PgAutoscaleModeOff } - err = SetPoolProperty(context, clusterInfo, pool.Name, "pg_num", strconv.Itoa(wantedPgNumMax)) + err = SetPoolProperty(context, clusterInfo, pool.Name, PgNumProperty, strconv.Itoa(wantedPgNumMax)) if err != nil { return errors.Wrapf(err, "failed to align pg_num to pg_num_max for pool %q", pool.Name) } curPgNum = uint(wantedPgNumMax) - poolDetails.PgNum = curPgNum - err = SetPoolProperty(context, clusterInfo, pool.Name, "pgp_num", strconv.Itoa(wantedPgNumMax)) + err = SetPoolProperty(context, clusterInfo, pool.Name, PgpNumProperty, strconv.Itoa(wantedPgNumMax)) if err != nil { return errors.Wrapf(err, "failed to align pgp_num to pg_num_min for pool %q", pool.Name) } - poolDetails.PgpNum = uint(wantedPgNumMax) } - err := SetPoolProperty(context, clusterInfo, pool.Name, "pg_num_max", strconv.Itoa(wantedPgNumMax)) + err := SetPoolProperty(context, clusterInfo, pool.Name, PgNumMaxProperty, strconv.Itoa(wantedPgNumMax)) if err != nil { return errors.Wrapf(err, "failed to set pg_num_max quota for pool %q", pool.Name) } - curPgNumMin = wantedPgNumMax - poolDetails.PgNumMin = &curPgNumMin } return nil @@ -415,9 +436,9 @@ func setPgNumMinMaxProperties(context *clusterd.Context, clusterInfo *ClusterInf func isASpecificallyManagedPoolProperty(propName string) bool { switch propName { - case "pg_num_min": + case PgNumMinProperty: return true - case "pg_num_max": + case PgNumMaxProperty: return true } return false @@ -548,13 +569,8 @@ func createECPoolForApp(context *clusterd.Context, clusterInfo *ClusterInfo, ecP } } - poolDetails, err := GetPoolDetails(context, clusterInfo, pool.Name) - if err != nil { - return errors.Wrapf(err, "failed to get pool details for pool %s", pool.Name) - } - // update pg_num_min/pg_num_max properties - if err := setPgNumMinMaxProperties(context, clusterInfo, pool, &poolDetails); err != nil { + if err := setPgNumMinMaxProperties(context, clusterInfo, pool); err != nil { return err } @@ -622,7 +638,7 @@ func createReplicatedPoolForApp(context *clusterd.Context, clusterInfo *ClusterI } // update pg_num_min/pg_num_max properties - if err := setPgNumMinMaxProperties(context, clusterInfo, pool, &poolDetails); err != nil { + if err := setPgNumMinMaxProperties(context, clusterInfo, pool); err != nil { return err } diff --git a/pkg/daemon/ceph/client/pool_test.go b/pkg/daemon/ceph/client/pool_test.go index 52dbe1df00c0..9317712ba33c 100644 --- a/pkg/daemon/ceph/client/pool_test.go +++ b/pkg/daemon/ceph/client/pool_test.go @@ -16,10 +16,12 @@ limitations under the License. package client import ( + "encoding/json" "fmt" "os/exec" "reflect" "strconv" + "strings" "testing" "github.com/pkg/errors" @@ -212,6 +214,7 @@ func TestCreateReplicaPoolWithPgNumMin(t *testing.T) { func testCreateReplicaPool(t *testing.T, failureDomain, crushRoot, deviceClass, compressionMode string, parameters map[string]string) { crushRuleCreated := false + poolCreated := false compressionModeCreated := false pgNumMinSet, pgNumSet, pgpNumSet := false, false, false expectedPgNumMin, expectedPgNumMinSet := parameters["pg_num_min"] @@ -226,8 +229,17 @@ func testCreateReplicaPool(t *testing.T, failureDomain, crushRoot, deviceClass, assert.Equal(t, "replicated", args[5]) assert.Equal(t, "--size", args[7]) assert.Equal(t, "12345", args[8]) + poolCreated = true return "", nil } + if args[2] == "get" { + assert.Equal(t, "mypool", args[3]) + if !poolCreated { + return "", errors.New("pool doesn't exist") + } else { + return "{\"pool\":\"mypool\",\"pool_id\":5,\"size\":3,\"min_size\":2,\"pg_num\":8,\"pgp_num\":8,\"crush_rule\":\"mypool\",\"hashpspool\":true,\"nodelete\":false,\"nopgchange\":false,\"nosizechange\":false,\"write_fadvise_dontneed\":false,\"noscrub\":false,\"nodeep-scrub\":false,\"use_gmt_hitset\":true,\"fast_read\":0,\"recovery_priority\":5,\"pg_autoscale_mode\":\"on\",\"pg_autoscale_bias\":4,\"bulk\":false}", nil + } + } if args[2] == "set" { assert.Equal(t, "mypool", args[3]) if args[4] == "size" { @@ -264,26 +276,36 @@ func testCreateReplicaPool(t *testing.T, failureDomain, crushRoot, deviceClass, } } if args[1] == "crush" { - crushRuleCreated = true assert.Equal(t, "rule", args[2]) - assert.Equal(t, "create-replicated", args[3]) - assert.Equal(t, "mypool", args[4]) - if crushRoot == "" { - assert.Equal(t, "cluster-crush-root", args[5]) - } else { - assert.Equal(t, crushRoot, args[5]) - } - if failureDomain == "" { - assert.Equal(t, "host", args[6]) - } else { - assert.Equal(t, failureDomain, args[6]) + if args[3] == "create-replicated" { + crushRuleCreated = true + assert.Equal(t, "create-replicated", args[3]) + assert.Equal(t, "mypool", args[4]) + if crushRoot == "" { + assert.Equal(t, "cluster-crush-root", args[5]) + } else { + assert.Equal(t, crushRoot, args[5]) + } + if failureDomain == "" { + assert.Equal(t, "host", args[6]) + } else { + assert.Equal(t, failureDomain, args[6]) + } + if deviceClass == "" { + assert.False(t, testIsStringInSlice("hdd", args)) + } else { + assert.Equal(t, deviceClass, args[7]) + } + return "", nil } - if deviceClass == "" { - assert.False(t, testIsStringInSlice("hdd", args)) - } else { - assert.Equal(t, deviceClass, args[7]) + if args[3] == "dump" { + assert.Equal(t, "mypool", args[4]) + if crushRuleCreated { + return `{"rule_id":15,"rule_name":"mypool","ruleset":15,"type":1,"min_size":1,"max_size":10,"steps":[{"op":"take","item":-2,"item_name":"default"},{"op":"chooseleaf_firstn","num":0,"type":"` + failureDomain + `"},{"op":"emit"}]}`, nil + } else { + return "", errors.New("crush rule doesn't exist") + } } - return "", nil } return "", errors.Errorf("unexpected ceph command %q", args) } @@ -679,3 +701,204 @@ func hasCrushtool() bool { _, err := exec.LookPath("crushtool") return err == nil } + +func TestSetPgNumMinMaxPropertiesDoesNothing(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{} + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinMaxPropertiesDontChangeExistingMinMaxSettings(t *testing.T) { + pgNumMin := 8 + pgNumMax := 32 + got, _ := testSetPgNumMinMaxProperties(map[string]string{}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgNumMin: &pgNumMin, + PgNumMax: &pgNumMax, + PgAutoScaleMode: "on", + }) + wanted := []string{} + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinLowerThanPgNum(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_min": "4"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{ + "osd pool set mypool pg_num_min 4", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinHigherThanPgNumWithAutoScalingOn(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_min": "16"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{ + "osd pool set mypool pg_autoscale_mode off", + "osd pool set mypool pg_num 16", + "osd pool set mypool pgp_num 16", + "osd pool set mypool pg_num_min 16", + "osd pool set mypool pg_autoscale_mode on", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinHigherThanPgNumWithAutoScalingOff(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_min": "16"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "off", + }) + wanted := []string{ + "osd pool set mypool pg_num 16", + "osd pool set mypool pgp_num 16", + "osd pool set mypool pg_num_min 16", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMaxHigherThanPgNum(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_max": "16"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{ + "osd pool set mypool pg_num_max 16", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMaxLowerThanPgNumWithAutoScalingOn(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_max": "4"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{ + "osd pool set mypool pg_autoscale_mode off", + "osd pool set mypool pg_num 4", + "osd pool set mypool pgp_num 4", + "osd pool set mypool pg_num_max 4", + "osd pool set mypool pg_autoscale_mode on", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMaxLowerThanPgNumWithAutoScalingOff(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_max": "4"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "off", + }) + wanted := []string{ + "osd pool set mypool pg_num 4", + "osd pool set mypool pgp_num 4", + "osd pool set mypool pg_num_max 4", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinMaxWithAutoScalingOn(t *testing.T) { + got, _ := testSetPgNumMinMaxProperties(map[string]string{"pg_num_min": "16", "pg_num_max": "32"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{ + "osd pool set mypool pg_autoscale_mode off", + "osd pool set mypool pg_num 16", + "osd pool set mypool pgp_num 16", + "osd pool set mypool pg_num_min 16", + "osd pool set mypool pg_num_max 32", + "osd pool set mypool pg_autoscale_mode on", + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func TestSetPgNumMinMaxPropertiesCheckConsistency(t *testing.T) { + got, err := testSetPgNumMinMaxProperties(map[string]string{"pg_num_min": "32", "pg_num_max": "16"}, &CephStoragePoolDetails{ + PgNum: 8, + PgpNum: 8, + PgAutoScaleMode: "on", + }) + wanted := []string{} + + if err == nil { + t.Errorf("SetPgNumMinMaxProperties shouldn't have return an error") + } + + wantedError := "pg_num_min (32) can't be greater than pg_num_max (16) for pool \"mypool\"" + if wantedError != err.Error() { + t.Errorf("got %v want %v", err.Error(), wantedError) + } + + if !reflect.DeepEqual(got, wanted) { + t.Errorf("got %v want %v", got, wanted) + } +} + +func testSetPgNumMinMaxProperties(poolParameters map[string]string, poolState *CephStoragePoolDetails) ([]string, error) { + executor := &exectest.MockExecutor{} + context := &clusterd.Context{Executor: executor} + p := cephv1.NamedPoolSpec{ + Name: "mypool", + PoolSpec: cephv1.PoolSpec{ + Parameters: poolParameters, + }, + } + calls := make([]string, 0) + executor.MockExecuteCommandWithOutput = func(command string, args ...string) (string, error) { + if len(args) > 4 { + if args[1] == "pool" && args[2] == "get" { + data, err := json.Marshal(poolState) + if err != nil { + return "", err + } + return string(data), nil + } + } + calls = append(calls, strings.Join(args[0:6], " ")) + return "", nil + } + err := setPgNumMinMaxProperties(context, AdminTestClusterInfo("mycluster"), p) + return calls, err +} diff --git a/pkg/operator/ceph/object/objectstore.go b/pkg/operator/ceph/object/objectstore.go index e43b5581fcb8..4c4255e3d755 100644 --- a/pkg/operator/ceph/object/objectstore.go +++ b/pkg/operator/ceph/object/objectstore.go @@ -816,11 +816,18 @@ func createRGWPool(ctx *Context, clusterSpec *cephv1.ClusterSpec, poolSpec cephv Name: poolName(ctx.Name, requestedName), PoolSpec: poolSpec, } + if err := cephclient.CreatePoolWithPGs(ctx.Context, ctx.clusterInfo, clusterSpec, pool, AppName, pgCount); err != nil { return errors.Wrapf(err, "failed to create pool %q", pool.Name) } + + poolDetails, err := cephclient.GetPoolDetails(ctx.Context, ctx.clusterInfo, pool.Name) + if err != nil { + return errors.Wrapf(err, "failed to get pool %q details", pool.Name) + } + // Set the pg_num_min if not the default so the autoscaler won't immediately increase the pg count - if pgCount != cephclient.DefaultPGCount { + if poolDetails.PgNumMin == nil && pgCount != cephclient.DefaultPGCount { if err := cephclient.SetPoolProperty(ctx.Context, ctx.clusterInfo, pool.Name, "pg_num_min", pgCount); err != nil { return errors.Wrapf(err, "failed to set pg_num_min on pool %q to %q", pool.Name, pgCount) }