From c995104c159d61c420b4978c79c81e662d80ffb7 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Fri, 24 Sep 2021 15:03:55 -0600 Subject: [PATCH] rgw: always update period, even if realm exists Rook should always update the RGW object store's period, even if the zone/zonegroup/realm haven't changed. This is because the 'radosgw-admin period update' command can fail, and we must retry it in that case. Signed-off-by: Blaine Gardner --- pkg/operator/ceph/object/controller_test.go | 64 ++++++++++++++++----- pkg/operator/ceph/object/objectstore.go | 16 ++---- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/pkg/operator/ceph/object/controller_test.go b/pkg/operator/ceph/object/controller_test.go index 8bc9282c39c11..cb7e9e9457d93 100644 --- a/pkg/operator/ceph/object/controller_test.go +++ b/pkg/operator/ceph/object/controller_test.go @@ -442,24 +442,29 @@ func TestCephObjectStoreController(t *testing.T) { {"poolnum":9,"poolname":"my-store.rgw.buckets.data"} ]`, nil } + t.Logf("returning default success for command: %s %v", command, args) return "", nil }, MockExecuteCommandWithTimeout: func(timeout time.Duration, command string, args ...string) (string, error) { - if args[0] == "realm" && args[1] == "list" { - return realmListJSON, nil - } - if args[0] == "realm" && args[1] == "get" { - return realmGetJSON, nil - } - if args[0] == "zonegroup" && args[1] == "get" { - return zoneGroupGetJSON, nil - } - if args[0] == "zone" && args[1] == "get" { - return zoneGetJSON, nil - } - if args[0] == "user" { - return userCreateJSON, nil + // this assumes the realm/zone/zonegroup have already been created + if command == "radosgw-admin" { + if args[0] == "realm" && args[1] == "list" { + return realmListJSON, nil + } + if args[0] == "realm" && args[1] == "get" { + return realmGetJSON, nil + } + if args[0] == "zonegroup" && args[1] == "get" { + return zoneGroupGetJSON, nil + } + if args[0] == "zone" && args[1] == "get" { + return zoneGetJSON, nil + } + if args[0] == "user" { + return userCreateJSON, nil + } } + t.Logf("returning default success for command: %s %v", command, args) return "", nil }, } @@ -500,6 +505,37 @@ func TestCephObjectStoreController(t *testing.T) { assert.Equal(t, "http://rook-ceph-rgw-my-store.rook-ceph.svc:80", objectStore.Status.Info["endpoint"], objectStore) }) + t.Run("regression check - period update command should always be called", func(t *testing.T) { + r := setupEnvironmentWithReadyCephCluster() + + // do a little hack to get the mock executor that was set up in the setup function, and make + // a wrapper around it that will let us verify that the period update command got issued + periodUpdateCount := 0 + mockExec, ok := r.context.Executor.(*exectest.MockExecutor) + if !ok { + t.Fatalf("failed to set up test mock executor") + } + origMockExecWithTimeout := mockExec.MockExecuteCommandWithTimeout + mockExec.MockExecuteCommandWithTimeout = func(timeout time.Duration, command string, arg ...string) (string, error) { + // count how many times period is updated + if command == "radosgw-admin" && arg[0] == "period" && arg[1] == "update" { + periodUpdateCount++ + } + if command == "radosgw-admin" && arg[0] == "zone" && arg[1] == "create" { + t.Fatal("this test assumes that the zone (and zonegroup and realm) already exist and should not be created, " + + "but 'create' was called! The test setup must have changed! " + + "Please fix the setup or this test to compensate!") + } + // pass through all commands to the original function + return origMockExecWithTimeout(timeout, command, arg...) + } + + res, err := r.Reconcile(ctx, req) + assert.NoError(t, err) + assert.False(t, res.Requeue) + assert.Equal(t, periodUpdateCount, 1) + }) + } func TestCephObjectStoreControllerMultisite(t *testing.T) { diff --git a/pkg/operator/ceph/object/objectstore.go b/pkg/operator/ceph/object/objectstore.go index 1a4c9c0874779..7967b5956377b 100644 --- a/pkg/operator/ceph/object/objectstore.go +++ b/pkg/operator/ceph/object/objectstore.go @@ -362,13 +362,11 @@ func createMultisite(objContext *Context, endpointArg string) error { realmArg := fmt.Sprintf("--rgw-realm=%s", objContext.Realm) zoneGroupArg := fmt.Sprintf("--rgw-zonegroup=%s", objContext.ZoneGroup) - updatePeriod := false // create the realm if it doesn't exist yet output, err := RunAdminCommandNoMultisite(objContext, true, "realm", "get", realmArg) if err != nil { // ENOENT means “No such file or directory” if code, err := exec.ExtractExitCode(err); err == nil && code == int(syscall.ENOENT) { - updatePeriod = true output, err = RunAdminCommandNoMultisite(objContext, false, "realm", "create", realmArg) if err != nil { return errorOrIsNotFound(err, "failed to create ceph realm %q, for reason %q", objContext.ZoneGroup, output) @@ -384,7 +382,6 @@ func createMultisite(objContext *Context, endpointArg string) error { if err != nil { // ENOENT means “No such file or directory” if code, err := exec.ExtractExitCode(err); err == nil && code == int(syscall.ENOENT) { - updatePeriod = true output, err = RunAdminCommandNoMultisite(objContext, false, "zonegroup", "create", "--master", realmArg, zoneGroupArg, endpointArg) if err != nil { return errorOrIsNotFound(err, "failed to create ceph zone group %q, for reason %q", objContext.ZoneGroup, output) @@ -400,7 +397,6 @@ func createMultisite(objContext *Context, endpointArg string) error { if err != nil { // ENOENT means “No such file or directory” if code, err := exec.ExtractExitCode(err); err == nil && code == int(syscall.ENOENT) { - updatePeriod = true output, err = runAdminCommand(objContext, false, "zone", "create", "--master", endpointArg) if err != nil { return errorOrIsNotFound(err, "failed to create ceph zone %q, for reason %q", objContext.Zone, output) @@ -411,14 +407,12 @@ func createMultisite(objContext *Context, endpointArg string) error { } } - if updatePeriod { - // the period will help notify other zones of changes if there are multi-zones - _, err := runAdminCommand(objContext, false, "period", "update", "--commit") - if err != nil { - return errorOrIsNotFound(err, "failed to update period") - } - logger.Debugf("updated period for realm %v", objContext.Realm) + // the period will help notify other zones of changes if there are multi-zones + _, err = runAdminCommand(objContext, false, "period", "update", "--commit") + if err != nil { + return errorOrIsNotFound(err, "failed to update period") } + logger.Debugf("updated period for realm %v", objContext.Realm) logger.Infof("Multisite for object-store: realm=%s, zonegroup=%s, zone=%s", objContext.Realm, objContext.ZoneGroup, objContext.Zone)