rgw: update period if period does not exist

Rook should update the RGW object store's period if the period doesn't
yet exist. This protects us from the case where the
'radosgw-admin period update --commit` command fails and the
CephObjectStore controller reconciles again.

Signed-off-by: Blaine Gardner <blaine.gardner@redhat.com>

pkg/operator/ceph/object/objectstore.go

@@ -151,7 +151,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
 }
@@ -373,9 +373,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)))
 		}
 	}
 
@@ -389,9 +389,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)
 		}
 	}
 
@@ -405,19 +405,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)

pkg/operator/ceph/object/objectstore_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"syscall"
 	"testing"
 	"time"
 
@@ -291,3 +292,291 @@ 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,
+				failCreateRealm:     true,
+				failCreateZoneGroup: true,
+				failCreateZone:      true,
+				failUpdatePeriod:    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)
+			}
+		})
+	}
+}