Fixed a backend userState bug, where on a controller restart userStat…

…e was not being preserved.

core/orchestrator_core.go

@@ -263,11 +263,7 @@ func (o *TridentOrchestrator) bootstrapBackends(ctx context.Context) error {
 		if found {
 			newBackend.SetOnline(b.Online)
 
-			if b.UserState != "" {
-				newBackend.SetUserState(b.UserState)
-			} else {
-				newBackend.SetUserState(storage.UserNormal)
-			}
+			newBackend.SetUserState(b.UserState)
 
 			if backendErr != nil {
 				newBackend.SetState(storage.Failed)
@@ -1094,7 +1090,16 @@ func (o *TridentOrchestrator) validateAndCreateBackendFromConfig(
 		}
 	}
 
-	return factory.NewStorageBackendForConfig(ctx, configInJSON, configRef, backendUUID, commonConfig, backendSecret)
+	sb, err := factory.NewStorageBackendForConfig(ctx, configInJSON, configRef, backendUUID, commonConfig, backendSecret)
+
+	if commonConfig.UserState != "" {
+		// If the userState field is present in tbc/backend.json, then update the userBackendState.
+		if err = o.updateUserBackendState(ctx, &sb, commonConfig.UserState, false); err != nil {
+			return nil, err
+		}
+	}
+
+	return sb, err
 }
 
 // UpdateBackend updates an existing backend.
@@ -1258,6 +1263,15 @@ func (o *TridentOrchestrator) updateBackendByBackendUUID(
 		return nil, err
 	}
 
+	// We're updating a backend, there can be two scenarios (related to userState):
+	// 1. userState field is not present in the tbc/backend.json,
+	//       so we should set it to whatever it was before.
+	// 2. userState field is present in the tbc/backend.json, then we've already set it
+	//       when we called validateAndCreateBackendFromConfig() above.
+	if backend.Driver().GetCommonConfig(ctx).UserState == "" {
+		backend.SetUserState(originalBackend.UserState())
+	}
+
 	if err = o.validateBackendUpdate(originalBackend, backend); err != nil {
 		return nil, err
 	}
@@ -1431,49 +1445,79 @@ func (o *TridentOrchestrator) UpdateBackendState(
 	}
 
 	if userBackendState != "" {
-		backendExternal, err = o.updateUserBackendState(ctx, backend, userBackendState)
+		if err = o.updateUserBackendState(ctx, &backend, userBackendState, true); err != nil {
+			return nil, err
+		}
 	}
 	if backendState != "" {
-		backendExternal, err = o.updateBackendState(ctx, backend, backendState)
+		if err = o.updateBackendState(ctx, &backend, backendState); err != nil {
+			return nil, err
+		}
 	}
 
-	return backendExternal, err
+	return backend.ConstructExternal(ctx), o.storeClient.UpdateBackend(ctx, backend)
 }
 
-func (o *TridentOrchestrator) updateUserBackendState(ctx context.Context, backend storage.Backend, userBackendState string) (backendExternal *storage.BackendExternal, err error) {
+func (o *TridentOrchestrator) updateUserBackendState(ctx context.Context, sb *storage.Backend, userBackendState string, isCLI bool) (err error) {
+	backend := *sb
 	Logc(ctx).WithFields(LogFields{
 		"backendName":      backend.Name(),
 		"userBackendState": userBackendState,
 	}).Debug("updateUserBackendState")
 
+	// There are primarily two methods for creating a backend:
+	// 1. Backend is either created via tbc or linked to tbc, then there can be two scenarios:
+	//     A. If the userState field is present in the config section of the tbc,
+	//        then updating via tridentctl is not allowed.
+	//	   B. If the userState field is not present in the config section of the tbc,
+	//	      then updating via tridentctl is allowed.
+	// 2. Backend is created via tridentctl, using backend.json:
+	//     A. It doesn't matter if the userState field is present in the backend.json or not,
+	//        updating via tridentctl is allowed.
+	if isCLI {
+		commonConfig := backend.Driver().GetCommonConfig(ctx)
+		if commonConfig.UserState != "" {
+			if backend.ConfigRef() != "" {
+				return fmt.Errorf("updating via tridentctl is not allowed when `userState` field is set in the tbc of the backend")
+			} else {
+				// If the userState has been updated via tridentctl,
+				//    then in the config section of tbe, userState will be shown empty.
+				// We will only come to this section of the code when a backend is created via tridentctl using backend.json,
+				//    and hasn't been linked to any of the tbc yet.
+				commonConfig.UserState = ""
+			}
+		}
+	}
+
 	userBackendState = strings.ToLower(userBackendState)
 	newUserBackendState := storage.UserBackendState(userBackendState)
 
 	// An extra check to ensure that the user-backend state is valid.
 	if err = newUserBackendState.Validate(); err != nil {
-		return nil, err
+		return fmt.Errorf("invalid user backend state provided: %s, allowed are: `%s`, `%s`", string(newUserBackendState), storage.UserNormal, storage.UserSuspended)
 	}
 
 	// Idempotent check.
 	if backend.UserState() == newUserBackendState {
-		return backend.ConstructExternal(ctx), nil
+		return nil
 	}
 
 	// If the user requested for the backend to be suspended.
 	if newUserBackendState.IsSuspended() {
 		// Backend is only suspended when its current state is either online, offline or failed.
 		if !backend.State().IsOnline() && !backend.State().IsOffline() && !backend.State().IsFailed() {
-			return nil, fmt.Errorf("the backend '%s' is currently not in any of the expected states: offline, online, or failed. Its current state is '%s'", backend.Name(), backend.State())
+			return fmt.Errorf("the backend '%s' is currently not in any of the expected states: offline, online, or failed. Its current state is '%s'", backend.Name(), backend.State())
 		}
 	}
 
 	// Update the user-backend state.
 	backend.SetUserState(newUserBackendState)
 
-	return backend.ConstructExternal(ctx), o.storeClient.UpdateBackend(ctx, backend)
+	return nil
 }
 
-func (o *TridentOrchestrator) updateBackendState(ctx context.Context, backend storage.Backend, backendState string) (backendExternal *storage.BackendExternal, err error) {
+func (o *TridentOrchestrator) updateBackendState(ctx context.Context, sb *storage.Backend, backendState string) (err error) {
+	backend := *sb
 	Logc(ctx).WithFields(LogFields{
 		"backendName":      backend.Name(),
 		"userBackendState": backendState,
@@ -1484,15 +1528,15 @@ func (o *TridentOrchestrator) updateBackendState(ctx context.Context, backend st
 
 	// Limit the command to Failed
 	if !newBackendState.IsFailed() {
-		return nil, fmt.Errorf("unsupported backend state: %s", newBackendState)
+		return fmt.Errorf("unsupported backend state: %s", newBackendState)
 	}
 
 	if !newBackendState.IsOnline() {
 		backend.Terminate(ctx)
 	}
 	backend.SetState(newBackendState)
 
-	return backend.ConstructExternal(ctx), o.storeClient.UpdateBackend(ctx, backend)
+	return nil
 }
 
 func (o *TridentOrchestrator) getBackendUUIDByBackendName(backendName string) (string, error) {

core/orchestrator_core_test.go

@@ -8547,12 +8547,14 @@ func TestUpdateBackendState(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mockBackend := mockstorage.NewMockBackend(mockCtrl)
 	mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
+	fakeStorageDriver := fakedriver.NewFakeStorageDriverWithDebugTraceFlags(nil)
 	o := getOrchestrator(t, false)
 	o.storeClient = mockStoreClient
 	o.bootstrapError = nil
 	o.backends[backendUUID] = mockBackend
 
 	mockBackend.EXPECT().Name().Return("something").AnyTimes()
+	mockBackend.EXPECT().Driver().Return(fakeStorageDriver).AnyTimes()
 	mockBackend.EXPECT().GetDriverName().Return("ontap").AnyTimes()
 	mockBackend.EXPECT().SetState(gomock.Any()).AnyTimes()
 	mockBackend.EXPECT().SetUserState(gomock.Any()).AnyTimes()
@@ -8638,4 +8640,19 @@ func TestUpdateBackendState(t *testing.T) {
 	_, err = o.UpdateBackendState(ctx, "something", "", "suspended")
 	assert.Error(t, err, "should return error, bootstrap error")
 	o.bootstrapError = nil
+
+	// Test 15 - when commonConfig.userState in tbc is set
+	fakeStorageDriver.Config.UserState = "suspended"
+	mockBackend.EXPECT().State().Return(storage.Online).Times(1)
+	mockBackend.EXPECT().ConfigRef().Return("1234").Times(1)
+	_, err = o.UpdateBackendState(ctx, "something", "", "suspended")
+	assert.Errorf(t, err, "should return error, userBackendState is set in commonConfig, so modifying via cli is not allowed")
+
+	// Test 16 - when commonConfig.userState in tbe is set, and there's no tbc linked to this tbe yet.
+	fakeStorageDriver.Config.UserState = "suspended"
+	mockBackend.EXPECT().ConfigRef().Return("").Times(1)
+	mockBackend.EXPECT().State().Return(storage.Online).Times(1)
+	mockBackend.EXPECT().UserState().Return(storage.UserSuspended).Times(1)
+	_, err = o.UpdateBackendState(ctx, "something", "", "suspended")
+	assert.NoError(t, err, "update to userState via tridentctl should be allowed when there's no tbc linked to this tbe yet")
 }

go.mod

@@ -45,7 +45,6 @@ require (
 	github.com/spf13/afero v1.11.0
 	github.com/spf13/cobra v1.8.0
 	github.com/stretchr/testify v1.8.4
-	github.com/tidwall/gjson v1.17.1
 	github.com/vishvananda/netlink v1.1.0
 	github.com/zcalusic/sysinfo v1.0.2
 	go.uber.org/multierr v1.11.0 // github.com/uber-go/multierr
@@ -139,8 +138,6 @@ require (
 	github.com/rivo/uniseg v0.1.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/stretchr/objx v0.5.0 // indirect
-	github.com/tidwall/match v1.1.1 // indirect
-	github.com/tidwall/pretty v1.2.1 // indirect
 	github.com/vishvananda/netns v0.0.0-20191106174202-0a2b9b5464df // indirect
 	go.mongodb.org/mongo-driver v1.13.1 // indirect
 	go.opentelemetry.io/otel v1.19.0 // indirect

go.sum

@@ -327,13 +327,6 @@ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
-github.com/tidwall/gjson v1.17.1 h1:wlYEnwqAHgzmhNUFfw7Xalt2JzQvsMx2Se4PcoFCT/U=
-github.com/tidwall/gjson v1.17.1/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
-github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA=
-github.com/tidwall/match v1.1.1/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM=
-github.com/tidwall/pretty v1.2.0/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
-github.com/tidwall/pretty v1.2.1 h1:qjsOFOWWQl+N3RsoF5/ssm1pHmJJwhjlSbZ51I6wMl4=
-github.com/tidwall/pretty v1.2.1/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
 github.com/vishvananda/netlink v1.1.0 h1:1iyaYNBLmP6L0220aDnYQpo1QEV4t4hJ+xEEhhJH8j0=
 github.com/vishvananda/netlink v1.1.0/go.mod h1:cTgwzPIzzgDAYoQrMm0EdrjRUBkTqKYppBueQtXaqoE=
 github.com/vishvananda/netns v0.0.0-20191106174202-0a2b9b5464df h1:OviZH7qLw/7ZovXvuNyL3XQl8UFofeikI1NW1Gypu7k=

storage/factory/factory.go

@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/ghodss/yaml"
-	"github.com/tidwall/gjson"
 
 	"github.com/netapp/trident/config"
 	. "github.com/netapp/trident/logging"
@@ -79,14 +78,6 @@ func NewStorageBackendForConfig(
 		}
 	}()
 
-	// Retrieving userBackendState that either has been set by the user via an update/creation of the tbc/backend.json
-	// or was previously stored in the config and the control flow is a part of the orchestrator bootstrap process.
-	var userBackendState storage.UserBackendState
-	if userBackendState, err = GetUserBackendState(ctx, configJSON); err != nil {
-		Logc(ctx).WithField("error", err).Error("Failed to get userBackendState from the config, setting it to default i.e. UserNormal")
-	}
-	Logc(ctx).WithField("userBackendState", userBackendState).Trace("Retrieved userBackendState from the config.")
-
 	driverProtocol, err := GetDriverProtocol(commonConfig.StorageDriverName, configJSON)
 	if err != nil {
 		Logc(ctx).WithField("error", err).Error("Failed to get driver protocol.")
@@ -120,7 +111,6 @@ func NewStorageBackendForConfig(
 
 	if err == nil {
 		sb.SetState(storage.Online)
-		sb.SetUserState(userBackendState)
 	}
 	sb.SetBackendUUID(backendUUID)
 	sb.SetConfigRef(configRef)
@@ -202,21 +192,3 @@ func GetDriverProtocol(driverName, configJSON string) (string, error) {
 
 	return "", nil
 }
-
-// GetUserBackendState retrieves the userBackendState that was set by the user
-// in the storage backend config file, either in tbc or backend.json.
-func GetUserBackendState(ctx context.Context, configJSON string) (storage.UserBackendState, error) {
-	// gjon is a Go package that provides a fast and simple way to get values from a json document
-	value := gjson.Get(configJSON, "userBackendState")
-
-	// Converting the value to userBackendState and validating it.
-	// If validation failed i.e., any value other than suspended or normal is provided,
-	// by default userBackendState: UserNormal will be returned.
-	userBackendState := storage.UserBackendState(strings.ToLower(value.String()))
-	if err := userBackendState.Validate(); err != nil {
-		Logc(ctx).WithField("error", err).Error("Validation failed for user backend state.")
-		return storage.UserNormal, err
-	}
-
-	return userBackendState, nil
-}

storage/factory/factory_test.go

@@ -341,103 +341,3 @@ func TestGetDriverProtocol(t *testing.T) {
 	_, err = GetDriverProtocol(config.OntapNASStorageDriverName, `{}`)
 	assert.NoError(t, err, "Failed to get protocol type.")
 }
-
-func TestGetUserBackendState(t *testing.T) {
-	empty := ""
-
-	// Function to create a FakeStorageDriverConfig.
-	configJSONFunc := func(userBackendState string) *drivers.FakeStorageDriverConfig {
-		return &drivers.FakeStorageDriverConfig{
-			CommonStorageDriverConfig: &drivers.CommonStorageDriverConfig{
-				Version:          1,
-				StoragePrefixRaw: json.RawMessage("{}"),
-				StoragePrefix:    &empty,
-				Credentials: map[string]string{
-					"name": "secret1",
-					"type": "secret",
-				},
-			},
-			Username:         "none",
-			Password:         "none",
-			UserBackendState: userBackendState,
-		}
-	}
-
-	// Set of valid tests.
-	validTests := []struct {
-		name   string
-		config *drivers.FakeStorageDriverConfig
-		output storage.UserBackendState
-		error  bool
-	}{
-		{
-			name:   "valid userBackendState userNormal",
-			config: configJSONFunc("normal"),
-			output: storage.UserNormal,
-			error:  false,
-		},
-		{
-			name:   "valid userBackendState userSuspended",
-			config: configJSONFunc("suspended"),
-			output: storage.UserSuspended,
-			error:  false,
-		},
-	}
-
-	for _, tt := range validTests {
-		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			marshaledJSON, err := json.Marshal(tt.config)
-			if err != nil {
-				t.Fatal("Unable to marshal configJSON: ", err)
-			}
-			output, err := GetUserBackendState(context.Background(), string(marshaledJSON))
-			if tt.error {
-				assert.Error(t, err)
-			} else {
-				assert.NoError(t, err)
-			}
-			assert.Equal(t, tt.output, output)
-		})
-	}
-
-	// Set of invalid tests.
-	invalidTests := []struct {
-		name   string
-		config *drivers.FakeStorageDriverConfig
-		output storage.UserBackendState
-		error  bool
-	}{
-		{
-			name:   "invalid userBackendState",
-			config: configJSONFunc("abcdef"),
-			output: storage.UserNormal,
-			error:  true,
-		},
-		{
-			name:   "missing userBackendState",
-			config: configJSONFunc(""),
-			output: storage.UserNormal,
-			error:  true,
-		},
-	}
-
-	for _, tt := range invalidTests {
-		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			marshaledJSON, err := json.Marshal(tt.config)
-			if err != nil {
-				t.Fatal("Unable to marshal configJSON: ", err)
-			}
-			output, err := GetUserBackendState(context.Background(), string(marshaledJSON))
-			if tt.error {
-				assert.Error(t, err)
-			} else {
-				assert.NoError(t, err)
-			}
-			assert.Equal(t, tt.output, output)
-		})
-	}
-}

storage_drivers/types.go

@@ -76,6 +76,7 @@ type CommonStorageDriverConfig struct {
 	DriverContext     trident.DriverContext `json:"-"`
 	LimitVolumeSize   string                `json:"limitVolumeSize"`
 	Credentials       map[string]string     `json:"credentials"`
+	UserState         string                `json:"userState"`
 }
 
 type CommonStorageDriverConfigDefaults struct {
@@ -715,8 +716,7 @@ type FakeStorageDriverConfig struct {
 	Username     string                  `json:"username"`
 	Password     string                  `json:"password"`
 	// Dummy field for unit tests
-	VolumeAccess     string `json:"volumeAccess"`
-	UserBackendState string `json:"userBackendState"`
+	VolumeAccess string `json:"volumeAccess"`
 	FakeStorageDriverPool
 }