Refactor storage prefix update prevention (#344)

NetApp · Jul 29, 2020 · a9a26b8 · a9a26b8
1 parent ba749c5
commit a9a26b8
Show file tree

Hide file tree

Showing 13 changed files with 111 additions and 97 deletions.
diff --git a/core/orchestrator_core.go b/core/orchestrator_core.go
@@ -823,61 +823,9 @@ func (o *TridentOrchestrator) validateBackendUpdate(
 			" %s", oldBackend.GetDriverName(), newBackend.GetDriverName())
 	}
 
-        // storagePrefix attribute is not used and ignored in the SF driver
-        // hence we need not compare storagePrefix during backend updates.
-        if oldBackend.GetDriverName() == drivers.SolidfireSANStorageDriverName {
-                return nil
-        }
-
-        oldStoragePrefix := o.getStoragePrefix(oldBackend)
-        newStoragePrefix := o.getStoragePrefix(newBackend)
-
-        if oldStoragePrefix != newStoragePrefix {
-                err := fmt.Errorf("cannot update the backend as updating the StoragePrefix isn't currently supported")
-                log.WithFields(log.Fields{
-                        "oldStoragePrefix": oldStoragePrefix,
-                        "newStoragePrefix": newStoragePrefix,
-                }).Error(err)
-
-                return err
-        }
-
 	return nil
 }
 
-func (o *TridentOrchestrator) getStoragePrefix(backend *storage.Backend) string {
-        backendConfig := backend.Driver.GetExternalConfig()
-        storagePrefix := ""
-        switch backendConf := backendConfig.(type) {
-        case drivers.OntapStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        case drivers.AWSNFSStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        case drivers.ESeriesStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        case drivers.AzureNFSStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        case drivers.GCPNFSStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        case drivers.FakeStorageDriverConfig:
-                if backendConf.StoragePrefix != nil {
-                        storagePrefix = *backendConf.StoragePrefix
-                }
-        }
-
-	return storagePrefix
-}
-
 func (o *TridentOrchestrator) GetVersion() (string, error) {
 	return config.OrchestratorVersion.String(), o.bootstrapError
 }
@@ -1131,33 +1079,43 @@ func (o *TridentOrchestrator) updateBackendByBackendUUID(backendName, configJSON
 	updateCode := backend.GetUpdateType(originalBackend)
 	switch {
 	case updateCode.Contains(storage.InvalidUpdate):
-		log.Error("invalid backend update")
-		return nil, fmt.Errorf("invalid backend update")
+		err := errors.New("invalid backend update")
+		log.WithField("error", err).Error("Backend update failed.")
+		return nil, err
 	case updateCode.Contains(storage.VolumeAccessInfoChange):
-		log.Error("updating the data plane IP address isn't currently supported")
-		return nil, fmt.Errorf("updating the data plane IP address isn't currently supported")
+		err := errors.New("updating the data plane IP address isn't currently supported")
+		log.WithField("error", err).Error("Backend update failed.")
+		return nil, err
 	case updateCode.Contains(storage.BackendRename):
 		checkingBackend, lookupErr := o.getBackendByBackendName(backend.Name)
 		if lookupErr == nil {
-			// don't rename if the name is already in use
-			log.Errorf("backend name %v is already in use by %v", backend.Name, checkingBackend.BackendUUID)
-			return nil, fmt.Errorf("backend name %v is already in use by %v", backend.Name, checkingBackend.BackendUUID)
+			// Don't rename if the name is already in use
+			err := fmt.Errorf("backend name %v is already in use by %v", backend.Name, checkingBackend.BackendUUID)
+			log.WithField("error", err).Error("Backend update failed.")
+			return nil, err
 		} else if utils.IsNotFoundError(lookupErr) {
-			// ok, we couldn't find it so it's not in use, let's rename
-			err = o.replaceBackendAndUpdateVolumesOnPersistentStore(originalBackend, backend)
-			if err != nil {
-				log.Errorf("problem while renaming backend from %v to %v error: %v", originalBackend.Name, backend.Name, err)
+			// We couldn't find it so it's not in use, let's rename
+			if err := o.replaceBackendAndUpdateVolumesOnPersistentStore(originalBackend, backend); err != nil {
+				log.WithField("error", err).Errorf(
+					"Could not rename backend from %v to %v", originalBackend.Name, backend.Name)
 				return nil, err
 			}
 		} else {
-			// unexpected error while checking if the backend is already in use
-			log.Errorf("unexpected problem while renaming backend from %v to %v error: %v", originalBackend.Name, backend.Name, lookupErr)
-			return nil, fmt.Errorf("unexpected problem while renaming backend from %v to %v error: %v", originalBackend.Name, backend.Name, lookupErr)
+			// Unexpected error while checking if the backend is already in use
+			err := fmt.Errorf("unexpected problem while renaming backend from %v to %v; %v",
+				originalBackend.Name, backend.Name, lookupErr)
+			log.WithField("error", err).Error("Backend update failed.")
+			return nil, err
 		}
+	case updateCode.Contains(storage.PrefixChange):
+		err := errors.New("updating the storage prefix isn't currently supported")
+		log.WithField("error", err).Error("Backend update failed.")
+		return nil, err
 	default:
 		// Update backend information
 		if err = o.updateBackendOnPersistentStore(backend, false); err != nil {
-			log.Errorf("problem persisting renamed backend from %v to  %v error: %v", originalBackend.Name, backend.Name, err)
+			log.WithField("error", err).Errorf("Could not persist renamed backend from %v to %v",
+				originalBackend.Name, backend.Name)
 			return nil, err
 		}
 	}

diff --git a/storage/backend.go b/storage/backend.go
@@ -694,6 +694,7 @@ const (
 	InvalidUpdate
 	UsernameChange
 	PasswordChange
+	PrefixChange
 )
 
 func (b *Backend) GetUpdateType(origBackend *Backend) *roaring.Bitmap {

diff --git a/storage_drivers/aws/aws_cvs.go b/storage_drivers/aws/aws_cvs.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net"
 	"net/url"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -1600,12 +1601,16 @@ func (d *NFSStorageDriver) getVolumeExternal(volumeAttrs *api.FileSystem) *stora
 // GetUpdateType returns a bitmap populated with updates to the driver
 func (d *NFSStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bitmap {
 	bitmap := roaring.New()
-	_, ok := driverOrig.(*NFSStorageDriver)
+	dOrig, ok := driverOrig.(*NFSStorageDriver)
 	if !ok {
 		bitmap.Add(storage.InvalidUpdate)
 		return bitmap
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/azure/azure_anf.go b/storage_drivers/azure/azure_anf.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -1535,12 +1536,16 @@ func (d *NFSStorageDriver) getVolumeExternal(volumeAttrs *sdk.FileSystem) *stora
 // GetUpdateType returns a bitmap populated with updates to the driver
 func (d *NFSStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bitmap {
 	bitmap := roaring.New()
-	_, ok := driverOrig.(*NFSStorageDriver)
+	dOrig, ok := driverOrig.(*NFSStorageDriver)
 	if !ok {
 		bitmap.Add(storage.InvalidUpdate)
 		return bitmap
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/eseries/eseries_iscsi.go b/storage_drivers/eseries/eseries_iscsi.go
@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -1347,6 +1348,10 @@ func (d *SANStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bit
 		bitmap.Add(storage.UsernameChange)
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/fake/fake.go b/storage_drivers/fake/fake.go
@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
@@ -1050,12 +1051,16 @@ func (d *StorageDriver) getVolumeExternal(volume fake.Volume) *storage.VolumeExt
 func (d *StorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bitmap {
 
 	bitmap := roaring.New()
-	_, ok := driverOrig.(*StorageDriver)
+	dOrig, ok := driverOrig.(*StorageDriver)
 	if !ok {
 		bitmap.Add(storage.InvalidUpdate)
 		return bitmap
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return roaring.New()
 }
 

diff --git a/storage_drivers/gcp/gcp_cvs.go b/storage_drivers/gcp/gcp_cvs.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -1637,12 +1638,16 @@ func (d *NFSStorageDriver) getVolumeExternal(volumeAttrs *api.Volume) *storage.V
 // GetUpdateType returns a bitmap populated with updates to the driver
 func (d *NFSStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bitmap {
 	bitmap := roaring.New()
-	_, ok := driverOrig.(*NFSStorageDriver)
+	dOrig, ok := driverOrig.(*NFSStorageDriver)
 	if !ok {
 		bitmap.Add(storage.InvalidUpdate)
 		return bitmap
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/ontap/ontap_nas.go b/storage_drivers/ontap/ontap_nas.go
@@ -4,6 +4,7 @@ package ontap
 
 import (
 	"fmt"
+	"reflect"
 	"strconv"
 	"strings"
 
@@ -397,19 +398,19 @@ func (d *NASStorageDriver) Import(volConfig *storage.VolumeConfig, originalName
 		}
 	}
 
-        // Modify unix-permissions of the volume if Trident will manage its lifecycle
-        if !volConfig.ImportNotManaged {
-                // unixPermissions specified in PVC annotation takes precedence over backend's unixPermissions config
-                unixPerms := volConfig.UnixPermissions
-                if unixPerms == "" {
-                        unixPerms = d.Config.UnixPermissions
-                }
-                modifyUnixPermResponse, err := d.API.VolumeModifyUnixPermissions(volConfig.InternalName, unixPerms)
-                if err = api.GetError(modifyUnixPermResponse, err); err != nil {
-                        log.WithField("originalName", originalName).Errorf("Could not import volume, modifying unix permissions failed: %v", err)
-                        return fmt.Errorf("volume %s modify failed: %v", originalName, err)
-                }
-        }
+	// Modify unix-permissions of the volume if Trident will manage its lifecycle
+	if !volConfig.ImportNotManaged {
+		// unixPermissions specified in PVC annotation takes precedence over backend's unixPermissions config
+		unixPerms := volConfig.UnixPermissions
+		if unixPerms == "" {
+			unixPerms = d.Config.UnixPermissions
+		}
+		modifyUnixPermResponse, err := d.API.VolumeModifyUnixPermissions(volConfig.InternalName, unixPerms)
+		if err = api.GetError(modifyUnixPermResponse, err); err != nil {
+			log.WithField("originalName", originalName).Errorf("Could not import volume, modifying unix permissions failed: %v", err)
+			return fmt.Errorf("volume %s modify failed: %v", originalName, err)
+		}
+	}
 
 	// Make sure we're not importing a volume without a junction path when not managed
 	if volConfig.ImportNotManaged {
@@ -770,6 +771,10 @@ func (d *NASStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bit
 		bitmap.Add(storage.UsernameChange)
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/ontap/ontap_nas_flexgroup.go b/storage_drivers/ontap/ontap_nas_flexgroup.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"reflect"
 	"runtime/debug"
 	"strconv"
 	"strings"
@@ -614,19 +615,19 @@ func (d *NASFlexGroupStorageDriver) Import(volConfig *storage.VolumeConfig, orig
 	// We cannot rename flexgroups, so internal name should match the imported originalName
 	volConfig.InternalName = originalName
 
-        // Modify unix-permissions of the volume if Trident will manage its lifecycle
-        if !volConfig.ImportNotManaged {
-                // unixPermissions specified in PVC annotation takes precedence over backend's unixPermissions config
-                unixPerms := volConfig.UnixPermissions
-                if unixPerms == "" {
-                        unixPerms = d.Config.UnixPermissions
-                }
-                modifyUnixPermResponse, err := d.API.FlexGroupModifyUnixPermissions(volConfig.InternalName, unixPerms)
-                if err = api.GetError(modifyUnixPermResponse, err); err != nil {
-                        log.WithField("originalName", originalName).Errorf("Could not import volume, modifying unix permissions failed: %v", err)
-                        return fmt.Errorf("volume %s modify failed: %v", originalName, err)
-                }
-        }
+	// Modify unix-permissions of the volume if Trident will manage its lifecycle
+	if !volConfig.ImportNotManaged {
+		// unixPermissions specified in PVC annotation takes precedence over backend's unixPermissions config
+		unixPerms := volConfig.UnixPermissions
+		if unixPerms == "" {
+			unixPerms = d.Config.UnixPermissions
+		}
+		modifyUnixPermResponse, err := d.API.FlexGroupModifyUnixPermissions(volConfig.InternalName, unixPerms)
+		if err = api.GetError(modifyUnixPermResponse, err); err != nil {
+			log.WithField("originalName", originalName).Errorf("Could not import volume, modifying unix permissions failed: %v", err)
+			return fmt.Errorf("volume %s modify failed: %v", originalName, err)
+		}
+	}
 
 	// Make sure we're not importing a volume without a junction path when not managed
 	if volConfig.ImportNotManaged {
@@ -1042,6 +1043,10 @@ func (d *NASFlexGroupStorageDriver) GetUpdateType(driverOrig storage.Driver) *ro
 		bitmap.Add(storage.UsernameChange)
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/ontap/ontap_nas_qtree.go b/storage_drivers/ontap/ontap_nas_qtree.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"reflect"
 	"strconv"
 	"strings"
 	"sync"
@@ -1547,6 +1548,10 @@ func (d *NASQtreeStorageDriver) GetUpdateType(driverOrig storage.Driver) *roarin
 		bitmap.Add(storage.UsernameChange)
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }
 

diff --git a/storage_drivers/ontap/ontap_san.go b/storage_drivers/ontap/ontap_san.go
@@ -4,6 +4,7 @@ package ontap
 
 import (
 	"fmt"
+	"reflect"
 	"strconv"
 	"strings"
 
@@ -937,6 +938,10 @@ func (d *SANStorageDriver) GetUpdateType(driverOrig storage.Driver) *roaring.Bit
 		bitmap.Add(storage.UsernameChange)
 	}
 
+	if !reflect.DeepEqual(d.Config.StoragePrefix, dOrig.Config.StoragePrefix) {
+		bitmap.Add(storage.PrefixChange)
+	}
+
 	return bitmap
 }