From be5421f00b2d9eac968259a0aea41c58da537669 Mon Sep 17 00:00:00 2001 From: f10atin9 Date: Mon, 27 Sep 2021 16:42:19 +0800 Subject: [PATCH 1/2] update capability_controller.go, make sure that annotations is generated correctly.StorageClass without csiDriver will no longer generate false annotations. Signed-off-by: f10atin9 --- .../capability/capability_controller.go | 27 ++++++++++++------- .../capability/capability_controller_test.go | 19 +++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/pkg/controller/storage/capability/capability_controller.go b/pkg/controller/storage/capability/capability_controller.go index 27cc7b2ef..4c5b209ab 100644 --- a/pkg/controller/storage/capability/capability_controller.go +++ b/pkg/controller/storage/capability/capability_controller.go @@ -208,13 +208,17 @@ func (c *StorageCapabilityController) syncHandler(key string) error { // Annotate storageClass storageClassUpdated := storageClass.DeepCopy() - err = c.addStorageClassSnapshotAnnotation(storageClassUpdated, isCSIStorage) - if err != nil { - return err - } - err = c.addCloneVolumeAnnotation(storageClassUpdated, isCSIStorage) - if err != nil { - return err + if !isCSIStorage { + c.removeAnnotations(storageClassUpdated) + } else { + err = c.updateStorageClassSnapshotAnnotation(storageClassUpdated, isCSIStorage) + if err != nil { + return err + } + err = c.updateCloneVolumeAnnotation(storageClassUpdated, isCSIStorage) + if err != nil { + return err + } } _, err = c.storageClassClient.Update(context.Background(), storageClassUpdated, metav1.UpdateOptions{}) if err != nil { @@ -234,7 +238,7 @@ func (c *StorageCapabilityController) hasCSIDriver(storageClass *storagev1.Stora return false } -func (c *StorageCapabilityController) addStorageClassSnapshotAnnotation(storageClass *storagev1.StorageClass, snapshotAllow bool) error { +func (c *StorageCapabilityController) updateStorageClassSnapshotAnnotation(storageClass *storagev1.StorageClass, snapshotAllow bool) error { if storageClass.Annotations == nil { storageClass.Annotations = make(map[string]string) } @@ -246,7 +250,7 @@ func (c *StorageCapabilityController) addStorageClassSnapshotAnnotation(storageC return nil } -func (c *StorageCapabilityController) addCloneVolumeAnnotation(storageClass *storagev1.StorageClass, cloneAllow bool) error { +func (c *StorageCapabilityController) updateCloneVolumeAnnotation(storageClass *storagev1.StorageClass, cloneAllow bool) error { if storageClass.Annotations == nil { storageClass.Annotations = make(map[string]string) } @@ -256,3 +260,8 @@ func (c *StorageCapabilityController) addCloneVolumeAnnotation(storageClass *sto } return nil } + +func (c *StorageCapabilityController) removeAnnotations(storageClass *storagev1.StorageClass) { + delete(storageClass.Annotations, annotationAllowClone) + delete(storageClass.Annotations, annotationAllowSnapshot) +} diff --git a/pkg/controller/storage/capability/capability_controller_test.go b/pkg/controller/storage/capability/capability_controller_test.go index 22486e5e5..7890d5d2f 100644 --- a/pkg/controller/storage/capability/capability_controller_test.go +++ b/pkg/controller/storage/capability/capability_controller_test.go @@ -270,3 +270,22 @@ func TestStorageClassHadOneAnnotation(t *testing.T) { // Run test fixture.run(getKey(storageClass, t)) } + +func TestStorageClassHadNoCSIDriver(t *testing.T) { + fixture := newFixture(t, true) + storageClass := newStorageClass("csi-example", "csi.example.com") + storageClass.Annotations = map[string]string{} + storageClassUpdate := storageClass.DeepCopy() + storageClass.Annotations = map[string]string{annotationAllowSnapshot: "false"} + storageClass.Annotations = map[string]string{annotationAllowClone: "false"} + + // Object exist + fixture.storageObjects = append(fixture.storageObjects, storageClass) + fixture.storageClassLister = append(fixture.storageClassLister, storageClass) + + // Action expected + fixture.expectUpdateStorageClassAction(storageClassUpdate) + + // Run test + fixture.run(getKey(storageClass, t)) +} From 60cd523a0f8a3e78bc392285c2b1702d60db4f42 Mon Sep 17 00:00:00 2001 From: f10atin9 Date: Tue, 28 Sep 2021 10:29:26 +0800 Subject: [PATCH 2/2] [fix] fix update logic Now controller will judge whether the storageClassClient need to send the update request. Signed-off-by: f10atin9 --- .../capability/capability_controller.go | 48 +++++++++---------- .../capability/capability_controller_test.go | 5 +- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/pkg/controller/storage/capability/capability_controller.go b/pkg/controller/storage/capability/capability_controller.go index 4c5b209ab..df0910ebd 100644 --- a/pkg/controller/storage/capability/capability_controller.go +++ b/pkg/controller/storage/capability/capability_controller.go @@ -21,13 +21,15 @@ package capability import ( "context" "fmt" + "reflect" "strconv" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" storagev1 "k8s.io/api/storage/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" storageinformersv1 "k8s.io/client-go/informers/storage/v1" @@ -89,7 +91,8 @@ func NewController( }) csiDriverInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: controller.enqueueStorageClassByCSI, + AddFunc: controller.enqueueStorageClassByCSI, + DeleteFunc: controller.enqueueStorageClassByCSI, }) return controller @@ -202,27 +205,25 @@ func (c *StorageCapabilityController) syncHandler(key string) error { // Get StorageClass storageClass, err := c.storageClassLister.Get(name) + if err != nil { + return err + } // Cloning and volumeSnapshot support only available for CSI drivers. isCSIStorage := c.hasCSIDriver(storageClass) - // Annotate storageClass storageClassUpdated := storageClass.DeepCopy() - if !isCSIStorage { - c.removeAnnotations(storageClassUpdated) + if isCSIStorage { + c.updateSnapshotAnnotation(storageClassUpdated, isCSIStorage) + c.updateCloneVolumeAnnotation(storageClassUpdated, isCSIStorage) } else { - err = c.updateStorageClassSnapshotAnnotation(storageClassUpdated, isCSIStorage) - if err != nil { - return err - } - err = c.updateCloneVolumeAnnotation(storageClassUpdated, isCSIStorage) - if err != nil { - return err - } + c.removeAnnotations(storageClassUpdated) } - _, err = c.storageClassClient.Update(context.Background(), storageClassUpdated, metav1.UpdateOptions{}) - if err != nil { - return err + if !reflect.DeepEqual(storageClass, storageClassUpdated) { + _, err = c.storageClassClient.Update(context.Background(), storageClassUpdated, metav1.UpdateOptions{}) + if err != nil { + return err + } } return nil } @@ -238,27 +239,24 @@ func (c *StorageCapabilityController) hasCSIDriver(storageClass *storagev1.Stora return false } -func (c *StorageCapabilityController) updateStorageClassSnapshotAnnotation(storageClass *storagev1.StorageClass, snapshotAllow bool) error { +func (c *StorageCapabilityController) updateSnapshotAnnotation(storageClass *storagev1.StorageClass, snapshotAllow bool) { if storageClass.Annotations == nil { storageClass.Annotations = make(map[string]string) } - _, err := strconv.ParseBool(storageClass.Annotations[annotationAllowSnapshot]) - // err != nil means annotationAllowSnapshot is not illegal, include empty - if err != nil { + if _, err := strconv.ParseBool(storageClass.Annotations[annotationAllowSnapshot]); err != nil { storageClass.Annotations[annotationAllowSnapshot] = strconv.FormatBool(snapshotAllow) } - return nil + return } -func (c *StorageCapabilityController) updateCloneVolumeAnnotation(storageClass *storagev1.StorageClass, cloneAllow bool) error { +func (c *StorageCapabilityController) updateCloneVolumeAnnotation(storageClass *storagev1.StorageClass, cloneAllow bool) { if storageClass.Annotations == nil { storageClass.Annotations = make(map[string]string) } - _, err := strconv.ParseBool(storageClass.Annotations[annotationAllowClone]) - if err != nil { + if _, err := strconv.ParseBool(storageClass.Annotations[annotationAllowClone]); err != nil { storageClass.Annotations[annotationAllowClone] = strconv.FormatBool(cloneAllow) } - return nil + return } func (c *StorageCapabilityController) removeAnnotations(storageClass *storagev1.StorageClass) { diff --git a/pkg/controller/storage/capability/capability_controller_test.go b/pkg/controller/storage/capability/capability_controller_test.go index 7890d5d2f..986cbad27 100644 --- a/pkg/controller/storage/capability/capability_controller_test.go +++ b/pkg/controller/storage/capability/capability_controller_test.go @@ -235,14 +235,13 @@ func TestCreateStorageClass(t *testing.T) { func TestStorageClassHadAnnotation(t *testing.T) { fixture := newFixture(t, true) storageClass := newStorageClass("csi-example", "csi.example.com") - storageClass.Annotations = map[string]string{annotationAllowSnapshot: "false", annotationAllowClone: "false"} + storageClass.Annotations = make(map[string]string) storageClassUpdate := storageClass.DeepCopy() - csiDriver := newCSIDriver("csi.example.com") + storageClass.Annotations = map[string]string{annotationAllowSnapshot: "false", annotationAllowClone: "false"} // Object exist fixture.storageObjects = append(fixture.storageObjects, storageClass) fixture.storageClassLister = append(fixture.storageClassLister, storageClass) - fixture.csiDriverLister = append(fixture.csiDriverLister, csiDriver) // Action expected fixture.expectUpdateStorageClassAction(storageClassUpdate)