Trident correctly handles updating backends that have volumes provis…

…ioned using storage classes that no longer exist

 Closes #29

CHANGELOG.md

@@ -9,6 +9,8 @@
 container orchestrator version.
 - When running in a pod, the Trident REST interface is no longer accessible
 by default outside the pod.
+- Trident correctly handles updating backends that have volumes provisioned
+using storage classes that no longer exist.
 
 **Enhancements:**
 - Added support for storage.k8s.io/v1 storage classes introduced in Kubernetes

README.md

@@ -222,6 +222,7 @@ class to provision its volumes, see [Provisioning Workflow](#provisioning-workfl
 and demonstrates some advanced use cases (please see
 [CHANGELOG](https://github.com/NetApp/trident/blob/master/CHANGELOG.md) for the
 changes since v1.0).
+
 [![Trident v1.0](https://img.youtube.com/vi/NDcnyGe2GFo/0.jpg)](https://www.youtube.com/watch?v=NDcnyGe2GFo)
 
 ## Requirements

core/orchestrator_core.go

@@ -313,12 +313,14 @@ func (o *tridentOrchestrator) validateBackendUpdate(
 	err = nil
 	errorList := make([]string, 0)
 
-	// Validate that protocols haven't changed
-	if oldBackend.GetProtocol() != newBackend.GetProtocol() {
-		errorList = append(errorList, "Cannot change backend protocol")
+	// In case Trident supports custom backend names in the future, validate
+	// that backend type isn't being changed as backend type has implications
+	// for the internal volume name.
+	if oldBackend.GetDriverName() != newBackend.GetDriverName() {
+		errorList = append(errorList, "Cannot update the backend type")
 	}
 
-	// Identifying all storage pools that have been used for
+	// Identifying all storage pools in the old backend that have been used for
 	// provisioning volumes.
 	usedPools := make([]string, 0)
 	for name, storagePool := range oldBackend.Storage {
@@ -343,32 +345,45 @@ func (o *tridentOrchestrator) validateBackendUpdate(
 		// we only care about the storage classes in use.
 		for _, volume := range storagePool.Volumes {
 			scName := volume.Config.StorageClass
-			_, ok := storagePoolsForStorageClasses[scName]
-			if !ok {
+			if _, found := storagePoolsForStorageClasses[scName]; !found {
 				storagePoolsForStorageClasses[scName] = make([]string, 0, 1)
 			}
 			storagePoolsForStorageClasses[scName] =
 				append(storagePoolsForStorageClasses[scName], storagePoolName)
 		}
 	}
 	for scName, storagePoolList := range storagePoolsForStorageClasses {
-		sc := o.storageClasses[scName]
+		sc, found := o.storageClasses[scName]
+		if !found {
+			// The storage class for the volume was deleted. The volume remains
+			// associated with the storage pool of the same name in the new
+			// backend.
+			continue
+		}
 		for _, storagePoolName := range storagePoolList {
-			log.WithFields(log.Fields{
-				"backend":      newBackend.Name,
-				"storagePool":  storagePoolName,
-				"storageClass": scName,
-			}).Debug("Checking whether storage class satisfies the new backend's storage pool.")
 			if newStoragePool, ok := newBackend.Storage[storagePoolName]; ok && !sc.Matches(newStoragePool) {
+				log.WithFields(log.Fields{
+					"backend":      newBackend.Name,
+					"storagePool":  storagePoolName,
+					"storageClass": scName,
+				}).Debug("The storage pool in the new backend doesn't satisfy " +
+					"the storage class.")
 				errorList = append(errorList, fmt.Sprintf("Storage pool "+
 					"%s has volumes with storage class %s, but it no longer "+
 					"satisfies that storage class", storagePoolName, scName))
+			} else if ok {
+				log.WithFields(log.Fields{
+					"backend":      newBackend.Name,
+					"storagePool":  storagePoolName,
+					"storageClass": scName,
+				}).Debug("The storage pool in the new backend satisfies the " +
+					"storage class.")
 			}
 		}
 	}
 	if len(errorList) > 0 {
-		err = fmt.Errorf("Cannot update backend:\n\t%s",
-			strings.Join(errorList, "\n\t"))
+		err = fmt.Errorf("Cannot update backend: %s", strings.Join(errorList,
+			"; "))
 	}
 	return err
 }
@@ -379,10 +394,6 @@ func (o *tridentOrchestrator) GetVersion() string {
 
 func (o *tridentOrchestrator) AddStorageBackend(configJSON string) (
 	*storage.StorageBackendExternal, error) {
-	var (
-		protocol config.Protocol
-	)
-
 	o.mutex.Lock()
 	defer o.mutex.Unlock()
 
@@ -391,7 +402,6 @@ func (o *tridentOrchestrator) AddStorageBackend(configJSON string) (
 		return nil, err
 	}
 	newBackend := true
-	protocol = storageBackend.GetProtocol()
 	originalBackend, ok := o.backends[storageBackend.Name]
 	if ok {
 		newBackend = false
@@ -401,9 +411,8 @@ func (o *tridentOrchestrator) AddStorageBackend(configJSON string) (
 	}
 
 	log.WithFields(log.Fields{
-		"backendName": storageBackend.Name,
-		"protocol":    protocol,
-		"newBackend":  newBackend,
+		"backend":       storageBackend.Name,
+		"backendUpdate": !newBackend,
 	}).Debug("Adding backend.")
 	if err = o.updateBackendOnPersistentStore(storageBackend, newBackend); err != nil {
 		return nil, err
@@ -421,13 +430,11 @@ func (o *tridentOrchestrator) AddStorageBackend(configJSON string) (
 	}
 	if len(classes) == 0 {
 		log.WithFields(log.Fields{
-			"backendName": storageBackend.Name,
-			"protocol":    protocol,
+			"backend": storageBackend.Name,
 		}).Info("Newly added backend satisfies no storage classes.")
 	} else {
 		log.WithFields(log.Fields{
-			"backendName": storageBackend.Name,
-			"protocol":    protocol,
+			"backend": storageBackend.Name,
 		}).Infof("Newly added backend satisfies storage classes %s.",
 			strings.Join(classes, ", "))
 	}

core/orchestrator_core_test.go

@@ -953,22 +953,6 @@ func TestBackendUpdateAndDelete(t *testing.T) {
 			},
 			protocol: config.File,
 		},
-		{
-			name: "Wrong protocol",
-			pools: map[string]*fake.FakeStoragePool{
-				"primary": &fake.FakeStoragePool{
-					Attrs: map[string]sa.Offer{
-						sa.Media:            sa.NewStringOffer("hdd"),
-						sa.ProvisioningType: sa.NewStringOffer("thick", "thin"),
-						// testingAttribute is here to ensure that only one
-						// storage class will match this backend.
-						sa.TestingAttribute: sa.NewBoolOffer(true),
-					},
-					Bytes: 100 * 1024 * 1024 * 1024,
-				},
-			},
-			protocol: config.Block,
-		},
 	} {
 		newConfigJSON, err := fake.NewFakeStorageDriverConfigJSON(backendName,
 			c.protocol, c.pools)

persistent_store/etcd_test.go

@@ -333,7 +333,7 @@ func TestEtcdv2Volume(t *testing.T) {
 		Name: "nfs_server-" + nfsServerConfig.ManagementLIF,
 	}
 	vol1Config := storage.VolumeConfig{
-		Version:      string(config.OrchestratorMajorVersion),
+		Version:      string(config.OrchestratorAPIVersion),
 		Name:         "vol1",
 		Size:         "1GB",
 		Protocol:     config.File,
@@ -418,7 +418,7 @@ func TestEtcdv2Volumes(t *testing.T) {
 
 	for i := 1; i <= newVolumeCount; i++ {
 		volConfig := storage.VolumeConfig{
-			Version:      string(config.OrchestratorMajorVersion),
+			Version:      string(config.OrchestratorAPIVersion),
 			Name:         "vol" + strconv.Itoa(i),
 			Size:         strconv.Itoa(i) + "GB",
 			Protocol:     config.File,
@@ -464,7 +464,7 @@ func TestEtcdv2VolumeTransactions(t *testing.T) {
 	// Adding volume transactions
 	for i := 1; i <= 5; i++ {
 		volConfig := storage.VolumeConfig{
-			Version:      string(config.OrchestratorMajorVersion),
+			Version:      string(config.OrchestratorAPIVersion),
 			Name:         "vol" + strconv.Itoa(i),
 			Size:         strconv.Itoa(i) + "GB",
 			Protocol:     config.File,
@@ -516,7 +516,7 @@ func TestEtcdv2VolumeTransactions(t *testing.T) {
 func TestDuplicateVolumeTransaction(t *testing.T) {
 	firstTxn := &VolumeTransaction{
 		Config: &storage.VolumeConfig{
-			Version:      string(config.OrchestratorMajorVersion),
+			Version:      string(config.OrchestratorAPIVersion),
 			Name:         "testVol",
 			Size:         "1 GB",
 			Protocol:     config.File,
@@ -526,7 +526,7 @@ func TestDuplicateVolumeTransaction(t *testing.T) {
 	}
 	secondTxn := &VolumeTransaction{
 		Config: &storage.VolumeConfig{
-			Version:      string(config.OrchestratorMajorVersion),
+			Version:      string(config.OrchestratorAPIVersion),
 			Name:         "testVol",
 			Size:         "1 GB",
 			Protocol:     config.File,

storage/storage_pool.go

@@ -28,30 +28,30 @@ func NewStoragePool(backend *StorageBackend, name string) *StoragePool {
 	}
 }
 
-func (vc *StoragePool) AddVolume(vol *Volume, bootstrap bool) {
-	vc.Volumes[vol.Config.Name] = vol
+func (pool *StoragePool) AddVolume(vol *Volume, bootstrap bool) {
+	pool.Volumes[vol.Config.Name] = vol
 }
 
-func (vc *StoragePool) DeleteVolume(vol *Volume) bool {
-	if _, ok := vc.Volumes[vol.Config.Name]; ok {
-		delete(vc.Volumes, vol.Config.Name)
+func (pool *StoragePool) DeleteVolume(vol *Volume) bool {
+	if _, ok := pool.Volumes[vol.Config.Name]; ok {
+		delete(pool.Volumes, vol.Config.Name)
 		return true
 	}
 	return false
 }
 
-func (vc *StoragePool) AddStorageClass(class string) {
+func (pool *StoragePool) AddStorageClass(class string) {
 	// Note that this function should get called once per storage class
 	// affecting the volume; thus, we don't need to check for duplicates.
-	vc.StorageClasses = append(vc.StorageClasses, class)
+	pool.StorageClasses = append(pool.StorageClasses, class)
 }
 
-func (vc *StoragePool) RemoveStorageClass(class string) bool {
+func (pool *StoragePool) RemoveStorageClass(class string) bool {
 	found := false
-	for i, name := range vc.StorageClasses {
+	for i, name := range pool.StorageClasses {
 		if name == class {
-			vc.StorageClasses = append(vc.StorageClasses[:i],
-				vc.StorageClasses[i+1:]...)
+			pool.StorageClasses = append(pool.StorageClasses[:i],
+				pool.StorageClasses[i+1:]...)
 			found = true
 			break
 		}
@@ -60,23 +60,24 @@ func (vc *StoragePool) RemoveStorageClass(class string) bool {
 }
 
 type StoragePoolExternal struct {
-	Name           string              `json:"name"`
-	StorageClasses []string            `json:"storageClasses"`
-	Attributes     map[string]sa.Offer `json:"storageAttributes"`
-	Volumes        []string            `json:"volumes"`
+	Name           string   `json:"name"`
+	StorageClasses []string `json:"storageClasses"`
+	//TODO: can't have an interface here for unmarshalling
+	Attributes map[string]sa.Offer `json:"storageAttributes"`
+	Volumes    []string            `json:"volumes"`
 }
 
-func (vc *StoragePool) ConstructExternal() *StoragePoolExternal {
+func (pool *StoragePool) ConstructExternal() *StoragePoolExternal {
 	external := &StoragePoolExternal{
-		Name:           vc.Name,
-		StorageClasses: vc.StorageClasses,
+		Name:           pool.Name,
+		StorageClasses: pool.StorageClasses,
 		Attributes:     make(map[string]sa.Offer),
-		Volumes:        make([]string, 0, len(vc.Volumes)),
+		Volumes:        make([]string, 0, len(pool.Volumes)),
 	}
-	for k, v := range vc.Attributes {
+	for k, v := range pool.Attributes {
 		external.Attributes[k] = v
 	}
-	for name, _ := range vc.Volumes {
+	for name, _ := range pool.Volumes {
 		external.Volumes = append(external.Volumes, name)
 	}
 	// We want to sort these so that the output remains consistent;