diff --git a/pkg/operator/ceph/object/objectstore.go b/pkg/operator/ceph/object/objectstore.go index 32f1070a75d9..a32f5bd4b5bb 100644 --- a/pkg/operator/ceph/object/objectstore.go +++ b/pkg/operator/ceph/object/objectstore.go @@ -152,7 +152,7 @@ func removeObjectStoreFromMultisite(objContext *Context, spec cephv1.ObjectStore if err != nil { return errors.Wrap(err, "failed to update period after removing an endpoint from the zone") } - logger.Infof("successfully updated period for realm %v after removal of object-store %v", objContext.Realm, objContext.Name) + logger.Infof("successfully updated period for realm %q after removal of object-store %q", objContext.Realm, objContext.Name) return nil } @@ -374,9 +374,9 @@ func createMultisite(objContext *Context, endpointArg string) error { if err != nil { return errorOrIsNotFound(err, "failed to create ceph realm %q, for reason %q", objContext.ZoneGroup, output) } - logger.Debugf("created realm %v", objContext.Realm) + logger.Debugf("created realm %q", objContext.Realm) } else { - return errorOrIsNotFound(err, "radosgw-admin realm get failed with code %d, for reason %q. %v", strconv.Itoa(code), output, string(kerrors.ReasonForError(err))) + return errorOrIsNotFound(err, "'radosgw-admin realm get' failed with code %d, for reason %q. %v", strconv.Itoa(code), output, string(kerrors.ReasonForError(err))) } } @@ -390,9 +390,9 @@ func createMultisite(objContext *Context, endpointArg string) error { if err != nil { return errorOrIsNotFound(err, "failed to create ceph zone group %q, for reason %q", objContext.ZoneGroup, output) } - logger.Debugf("created zone group %v", objContext.ZoneGroup) + logger.Debugf("created zone group %q", objContext.ZoneGroup) } else { - return errorOrIsNotFound(err, "radosgw-admin zonegroup get failed with code %d, for reason %q", strconv.Itoa(code), output) + return errorOrIsNotFound(err, "'radosgw-admin zonegroup get' failed with code %d, for reason %q", strconv.Itoa(code), output) } } @@ -406,19 +406,32 @@ func createMultisite(objContext *Context, endpointArg string) error { if err != nil { return errorOrIsNotFound(err, "failed to create ceph zone %q, for reason %q", objContext.Zone, output) } - logger.Debugf("created zone %v", objContext.Zone) + logger.Debugf("created zone %q", objContext.Zone) } else { - return errorOrIsNotFound(err, "radosgw-admin zone get failed with code %d, for reason %q", strconv.Itoa(code), output) + return errorOrIsNotFound(err, "'radosgw-admin zone get' failed with code %d, for reason %q", strconv.Itoa(code), output) + } + } + + // check if the period exists + output, err = runAdminCommand(objContext, false, "period", "get") + if err != nil { + code, err := exec.ExtractExitCode(err) + // ENOENT means “No such file or directory” + if err == nil && code == int(syscall.ENOENT) { + // period does not exist and so needs to be created + updatePeriod = true + } else { + return errorOrIsNotFound(err, "'radosgw-admin period get' failed with code %d, for reason %q", strconv.Itoa(code), output) } } if updatePeriod { // the period will help notify other zones of changes if there are multi-zones - _, err := runAdminCommand(objContext, false, "period", "update", "--commit") + _, 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.Debugf("updated period for realm %q", objContext.Realm) } logger.Infof("Multisite for object-store: realm=%s, zonegroup=%s, zone=%s", objContext.Realm, objContext.ZoneGroup, objContext.Zone) diff --git a/pkg/operator/ceph/object/objectstore_test.go b/pkg/operator/ceph/object/objectstore_test.go index 2bc03cfe05d3..932bb3633c8a 100644 --- a/pkg/operator/ceph/object/objectstore_test.go +++ b/pkg/operator/ceph/object/objectstore_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "os" + "syscall" "testing" "time" @@ -304,3 +305,287 @@ func TestDashboard(t *testing.T) { assert.True(t, checkdashboard) disableRGWDashboard(objContext) } + +// import TestMockExecHelperProcess +func TestMockExecHelperProcess(t *testing.T) { + exectest.TestMockExecHelperProcess(t) +} + +func Test_createMultisite(t *testing.T) { + // control the return values from calling get/create/update on resources + type commandReturns struct { + realmExists bool + zoneGroupExists bool + zoneExists bool + periodExists bool + failCreateRealm bool + failCreateZoneGroup bool + failCreateZone bool + failUpdatePeriod bool + } + + // control whether we should expect certain 'get' calls + type expectCommands struct { + getRealm bool + createRealm bool + getZoneGroup bool + createZoneGroup bool + getZone bool + createZone bool + getPeriod bool + updatePeriod bool + } + + // vars used for testing if calls were made + var ( + calledGetRealm = false + calledGetZoneGroup = false + calledGetZone = false + calledGetPeriod = false + calledCreateRealm = false + calledCreateZoneGroup = false + calledCreateZone = false + calledUpdatePeriod = false + ) + + enoentIfNotExist := func(resourceExists bool) (string, error) { + if !resourceExists { + return "", exectest.MockExecCommandReturns(t, "", "", int(syscall.ENOENT)) + } + return "{}", nil // get wants json, and {} is the most basic json + } + + errorIfFail := func(shouldFail bool) (string, error) { + if shouldFail { + return "", exectest.MockExecCommandReturns(t, "", "basic error", 1) + } + return "", nil + } + + setupTest := func(env commandReturns) *exectest.MockExecutor { + // reset output testing vars + calledGetRealm = false + calledCreateRealm = false + calledGetZoneGroup = false + calledCreateZoneGroup = false + calledGetZone = false + calledCreateZone = false + calledGetPeriod = false + calledUpdatePeriod = false + + return &exectest.MockExecutor{ + MockExecuteCommandWithTimeout: func(timeout time.Duration, command string, arg ...string) (string, error) { + if command == "radosgw-admin" { + switch arg[0] { + case "realm": + switch arg[1] { + case "get": + calledGetRealm = true + return enoentIfNotExist(env.realmExists) + case "create": + calledCreateRealm = true + return errorIfFail(env.failCreateRealm) + } + case "zonegroup": + switch arg[1] { + case "get": + calledGetZoneGroup = true + return enoentIfNotExist(env.zoneGroupExists) + case "create": + calledCreateZoneGroup = true + return errorIfFail(env.failCreateZoneGroup) + } + case "zone": + switch arg[1] { + case "get": + calledGetZone = true + return enoentIfNotExist(env.zoneExists) + case "create": + calledCreateZone = true + return errorIfFail(env.failCreateZone) + } + case "period": + switch arg[1] { + case "get": + calledGetPeriod = true + return enoentIfNotExist(env.periodExists) + case "update": + calledUpdatePeriod = true + return errorIfFail(env.failUpdatePeriod) + } + } + } + t.Fatalf("unhandled command: %s %v", command, arg) + return "", nil + }, + } + } + + expectNoErr := false // want no error + expectErr := true // want an error + + tests := []struct { + name string + commandReturns commandReturns + expectCommands expectCommands + wantErr bool + }{ + {"create realm, zonegroup, and zone; period update", + commandReturns{ + // nothing exists, and all should succeed + }, + expectCommands{ + getRealm: true, + createRealm: true, + getZoneGroup: true, + createZoneGroup: true, + getZone: true, + createZone: true, + getPeriod: true, + updatePeriod: true, + }, + expectNoErr}, + {"fail creating realm", + commandReturns{ + failCreateRealm: true, + }, + expectCommands{ + getRealm: true, + createRealm: true, + // when we fail to create realm, we should not continue + }, + expectErr}, + {"fail creating zonegroup", + commandReturns{ + failCreateZoneGroup: true, + }, + expectCommands{ + getRealm: true, + createRealm: true, + getZoneGroup: true, + createZoneGroup: true, + // when we fail to create zonegroup, we should not continue + }, + expectErr}, + {"fail creating zone", + commandReturns{ + failCreateZone: true, + }, + expectCommands{ + getRealm: true, + createRealm: true, + getZoneGroup: true, + createZoneGroup: true, + getZone: true, + createZone: true, + // when we fail to create zone, we should not continue + }, + expectErr}, + {"fail period update", + commandReturns{ + failUpdatePeriod: true, + }, + expectCommands{ + getRealm: true, + createRealm: true, + getZoneGroup: true, + createZoneGroup: true, + getZone: true, + createZone: true, + getPeriod: true, + updatePeriod: true, + }, + expectErr}, + {"realm exists; create zonegroup and zone; period update", + commandReturns{ + realmExists: true, + }, + expectCommands{ + getRealm: true, + createRealm: false, + getZoneGroup: true, + createZoneGroup: true, + getZone: true, + createZone: true, + getPeriod: true, + updatePeriod: true, + }, + expectNoErr}, + {"realm and zonegroup exist; create zone; period update", + commandReturns{ + realmExists: true, + zoneGroupExists: true, + }, + expectCommands{ + getRealm: true, + createRealm: false, + getZoneGroup: true, + createZoneGroup: false, + getZone: true, + createZone: true, + getPeriod: true, + updatePeriod: true, + }, + expectNoErr}, + {"realm, zonegroup, and zone exist; period update", + commandReturns{ + realmExists: true, + zoneGroupExists: true, + zoneExists: true, + }, + expectCommands{ + getRealm: true, + createRealm: false, + getZoneGroup: true, + createZoneGroup: false, + getZone: true, + createZone: false, + getPeriod: true, + updatePeriod: true, + }, + expectNoErr}, + {"realm, zonegroup, zone, and period all exist", + commandReturns{ + realmExists: true, + zoneGroupExists: true, + zoneExists: true, + periodExists: true, + }, + expectCommands{ + getRealm: true, + createRealm: false, + getZoneGroup: true, + createZoneGroup: false, + getZone: true, + createZone: false, + getPeriod: true, + updatePeriod: false, + }, + expectNoErr}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + executor := setupTest(tt.commandReturns) + ctx := &clusterd.Context{ + Executor: executor, + } + objContext := NewContext(ctx, &client.ClusterInfo{Namespace: "my-cluster"}, "my-store") + + // assumption: endpointArg is sufficiently tested by integration tests + err := createMultisite(objContext, "") + assert.Equal(t, tt.expectCommands.getRealm, calledGetRealm) + assert.Equal(t, tt.expectCommands.createRealm, calledCreateRealm) + assert.Equal(t, tt.expectCommands.getZoneGroup, calledGetZoneGroup) + assert.Equal(t, tt.expectCommands.createZoneGroup, calledCreateZoneGroup) + assert.Equal(t, tt.expectCommands.getZone, calledGetZone) + assert.Equal(t, tt.expectCommands.createZone, calledCreateZone) + assert.Equal(t, tt.expectCommands.getPeriod, calledGetPeriod) + assert.Equal(t, tt.expectCommands.updatePeriod, calledUpdatePeriod) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/util/exec/exec_test.go b/pkg/util/exec/exec_test.go index e99a70b1ab5b..4bcc9f38d734 100644 --- a/pkg/util/exec/exec_test.go +++ b/pkg/util/exec/exec_test.go @@ -17,13 +17,11 @@ limitations under the License. package exec import ( - "fmt" - "os" "os/exec" - "strconv" "testing" "github.com/pkg/errors" + exectest "github.com/rook/rook/pkg/util/exec/test" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kexec "k8s.io/utils/exec" @@ -51,12 +49,16 @@ func Test_assertErrorType(t *testing.T) { } } +// import TestMockExecHelperProcess +func TestMockExecHelperProcess(t *testing.T) { + exectest.TestMockExecHelperProcess(t) +} + func TestExtractExitCode(t *testing.T) { mockExecExitError := func(retcode int) *exec.ExitError { // we can't create an exec.ExitError directly, but we can get one by running a command that fails // use go's type assertion to be sure we are returning exactly *exec.ExitError - cmd := mockExecCommandReturns("stdout", "stderr", retcode) - err := cmd.Run() + err := exectest.MockExecCommandReturns(t, "stdout", "stderr", retcode) ee, ok := err.(*exec.ExitError) if !ok { @@ -108,33 +110,3 @@ func TestExtractExitCode(t *testing.T) { }) } } - -// Mock an exec command where we really only care about the return values -// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go -func mockExecCommandReturns(stdout, stderr string, retcode int) *exec.Cmd { - cmd := exec.Command(os.Args[0], "-test.run=TestExecHelperProcess") //nolint:gosec //Rook controls the input to the exec arguments - cmd.Env = append(os.Environ(), - "GO_WANT_HELPER_PROCESS=1", - fmt.Sprintf("GO_HELPER_PROCESS_STDOUT=%s", stdout), - fmt.Sprintf("GO_HELPER_PROCESS_STDERR=%s", stderr), - fmt.Sprintf("GO_HELPER_PROCESS_RETCODE=%d", retcode), - ) - return cmd -} - -// TestHelperProcess isn't a real test. It's used as a helper process. -// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go -func TestExecHelperProcess(*testing.T) { - if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { - return - } - - // test should set these in its environment to control the output of the test commands - fmt.Fprint(os.Stdout, os.Getenv("GO_HELPER_PROCESS_STDOUT")) - fmt.Fprint(os.Stderr, os.Getenv("GO_HELPER_PROCESS_STDERR")) - rc, err := strconv.Atoi(os.Getenv("GO_HELPER_PROCESS_RETCODE")) - if err != nil { - panic(err) - } - os.Exit(rc) -} diff --git a/pkg/util/exec/test/mockexec.go b/pkg/util/exec/test/mockexec.go index f2d5d2989536..b7f1815ad3fc 100644 --- a/pkg/util/exec/test/mockexec.go +++ b/pkg/util/exec/test/mockexec.go @@ -17,7 +17,11 @@ limitations under the License. package test import ( + "fmt" + "os" "os/exec" + "strconv" + "testing" "time" ) @@ -76,3 +80,47 @@ func (e *MockExecutor) ExecuteCommandWithCombinedOutput(command string, arg ...s return "", nil } + +// Mock an executed command with the desired return values. +// STDERR is returned *before* STDOUT. +// +// This will return an error if the given exit code is nonzero. The error return is the primary +// benefit of using this method. +// +// In order for this to work in a `*_test.go` file, you MUST import TestMockExecHelperProcess +// exactly as shown below: +// import exectest "github.com/rook/rook/pkg/util/exec/test" +// // import TestMockExecHelperProcess +// func TestMockExecHelperProcess(t *testing.T) { +// exectest.TestMockExecHelperProcess(t) +// } +// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go +func MockExecCommandReturns(t *testing.T, stdout, stderr string, retcode int) error { + cmd := exec.Command(os.Args[0], "-test.run=TestMockExecHelperProcess") //nolint:gosec //Rook controls the input to the exec arguments + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + fmt.Sprintf("GO_HELPER_PROCESS_STDOUT=%s", stdout), + fmt.Sprintf("GO_HELPER_PROCESS_STDERR=%s", stderr), + fmt.Sprintf("GO_HELPER_PROCESS_RETCODE=%d", retcode), + ) + err := cmd.Run() + return err +} + +// TestHelperProcess isn't a real test. It's used as a helper process for MockExecCommandReturns to +// simulate output from a command. Notably, this can return a realistic os/exec error. +// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go +func TestMockExecHelperProcess(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { + return + } + + // test should set these in its environment to control the output of the test commands + fmt.Fprint(os.Stderr, os.Getenv("GO_HELPER_PROCESS_STDERR")) // return stderr before stdout + fmt.Fprint(os.Stdout, os.Getenv("GO_HELPER_PROCESS_STDOUT")) + rc, err := strconv.Atoi(os.Getenv("GO_HELPER_PROCESS_RETCODE")) + if err != nil { + panic(err) + } + os.Exit(rc) +}