From 706a616fbd9928f6998fd15cb280f7f8cad3b63e Mon Sep 17 00:00:00 2001 From: Ardalan Kangarlou Date: Wed, 14 Jun 2017 13:55:11 -0400 Subject: [PATCH] Trident correctly handles updating backends that have volumes provisioned using storage classes that no longer exist Closes #29 --- CHANGELOG.md | 2 ++ README.md | 1 + core/orchestrator_core.go | 59 +++++++++++++++++++--------------- core/orchestrator_core_test.go | 16 --------- persistent_store/etcd_test.go | 10 +++--- storage/storage_pool.go | 43 +++++++++++++------------ storage_class/storage_class.go | 58 ++++++++++++++++----------------- 7 files changed, 92 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc5d0ef60..14529a838 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 986c30ade..0f4270550 100644 --- a/README.md +++ b/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 diff --git a/core/orchestrator_core.go b/core/orchestrator_core.go index 249e979b8..f3350241e 100644 --- a/core/orchestrator_core.go +++ b/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,8 +345,7 @@ 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] = @@ -352,23 +353,37 @@ func (o *tridentOrchestrator) validateBackendUpdate( } } 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, ", ")) } diff --git a/core/orchestrator_core_test.go b/core/orchestrator_core_test.go index 8ecb73d03..c724f0de7 100644 --- a/core/orchestrator_core_test.go +++ b/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) diff --git a/persistent_store/etcd_test.go b/persistent_store/etcd_test.go index f45f3d92b..21904060f 100644 --- a/persistent_store/etcd_test.go +++ b/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, diff --git a/storage/storage_pool.go b/storage/storage_pool.go index 0a2531b44..0c3a89554 100644 --- a/storage/storage_pool.go +++ b/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; diff --git a/storage_class/storage_class.go b/storage_class/storage_class.go index d1cff2073..4d9bc31d5 100644 --- a/storage_class/storage_class.go +++ b/storage_class/storage_class.go @@ -37,11 +37,11 @@ func NewFromPersistent(persistent *StorageClassPersistent) *StorageClass { return New(persistent.Config) } -func (s *StorageClass) Matches(vc *storage.StoragePool) bool { +func (s *StorageClass) Matches(storagePool *storage.StoragePool) bool { if len(s.config.BackendStoragePools) > 0 { - if vcList, ok := s.config.BackendStoragePools[vc.Backend.Name]; ok { - for _, vcName := range vcList { - if vcName == vc.Name { + if storagePoolList, ok := s.config.BackendStoragePools[storagePool.Backend.Name]; ok { + for _, storagePoolName := range storagePoolList { + if storagePoolName == storagePool.Name { return true } } @@ -49,19 +49,19 @@ func (s *StorageClass) Matches(vc *storage.StoragePool) bool { } matches := len(s.config.Attributes) > 0 for name, request := range s.config.Attributes { - if vc.Attributes == nil { + if storagePool.Attributes == nil { log.WithFields(log.Fields{ "storageClass": s.GetName(), - "pool": vc.Name, + "pool": storagePool.Name, "attribute": name, }).Panic("Storage pool attributes are nil") } - if offer, ok := vc.Attributes[name]; !ok || !offer.Matches(request) { + if offer, ok := storagePool.Attributes[name]; !ok || !offer.Matches(request) { log.WithFields(log.Fields{ "offer": offer, "request": request, "storageClass": s.GetName(), - "pool": vc.Name, + "pool": storagePool.Name, "attribute": name, "found": ok}).Debug("Attribute for storage " + "pool failed to match storage class.") @@ -80,10 +80,10 @@ func (s *StorageClass) CheckAndAddBackend(b *storage.StorageBackend) int { return 0 } added := 0 - for _, vc := range b.Storage { - if s.Matches(vc) { - s.pools = append(s.pools, vc) - vc.AddStorageClass(s.GetName()) + for _, storagePool := range b.Storage { + if s.Matches(storagePool) { + s.pools = append(s.pools, storagePool) + storagePool.AddStorageClass(s.GetName()) added++ } } @@ -91,19 +91,19 @@ func (s *StorageClass) CheckAndAddBackend(b *storage.StorageBackend) int { } func (s *StorageClass) RemovePoolsForBackend(backend *storage.StorageBackend) { - newVCList := make([]*storage.StoragePool, 0) - for _, vc := range s.pools { - if vc.Backend != backend { - newVCList = append(newVCList, vc) + newStoragePools := make([]*storage.StoragePool, 0) + for _, storagePool := range s.pools { + if storagePool.Backend != backend { + newStoragePools = append(newStoragePools, storagePool) } } - s.pools = newVCList + s.pools = newStoragePools } func (s *StorageClass) GetVolumes() []*storage.Volume { ret := make([]*storage.Volume, 0) - for _, vc := range s.pools { - for _, vol := range vc.Volumes { + for _, storagePool := range s.pools { + for _, vol := range storagePool.Volumes { // Because storage pools can fulfill more than one storage // class, we have to check each volume for that pool to see // if it uses this storage class. @@ -130,9 +130,9 @@ func (s *StorageClass) GetBackendStoragePools() map[string][]string { func (s *StorageClass) GetStoragePoolsForProtocol(p config.Protocol) []*storage.StoragePool { ret := make([]*storage.StoragePool, 0, len(s.pools)) // TODO: Change this to work with indices of backends? - for _, vc := range s.pools { - if p == config.ProtocolAny || vc.Backend.GetProtocol() == p { - ret = append(ret, vc) + for _, storagePool := range s.pools { + if p == config.ProtocolAny || storagePool.Backend.GetProtocol() == p { + ret = append(ret, storagePool) } } return ret @@ -143,25 +143,25 @@ func (s *StorageClass) ConstructExternal() *StorageClassExternal { Config: s.config, StoragePools: make(map[string][]string), } - for _, vc := range s.pools { - backendName := vc.Backend.Name - if vcList, ok := ret.StoragePools[backendName]; ok { + for _, storagePool := range s.pools { + backendName := storagePool.Backend.Name + if storagePoolList, ok := ret.StoragePools[backendName]; ok { log.WithFields(log.Fields{ "storageClass": s.GetName(), - "pool": vc.Name, + "pool": storagePool.Name, "Backend": backendName, "Method": "ConstructExternal", }).Debug("Appending to existing storage pool list for backend.") - ret.StoragePools[backendName] = append(vcList, vc.Name) + ret.StoragePools[backendName] = append(storagePoolList, storagePool.Name) } else { log.WithFields(log.Fields{ "storageClass": s.GetName(), - "pool": vc.Name, + "pool": storagePool.Name, "Backend": backendName, "Method": "ConstructExternal", }).Debug("Creating new storage pool list for backend.") ret.StoragePools[backendName] = make([]string, 1, 1) - ret.StoragePools[backendName][0] = vc.Name + ret.StoragePools[backendName][0] = storagePool.Name } } for _, list := range ret.StoragePools {