From cef61f0c8ff5681e367d02e054a89433687d9a2a Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 24 Jun 2024 15:46:09 +0300 Subject: [PATCH] [controller][csi] Fix StorageClass annotation preservation and improve NodePublishVolume mount check (#43) Signed-off-by: Aleksandr Zimin --- .../controller/local_storage_class_watcher.go | 3 ++ .../local_storage_class_watcher_func.go | 15 +++++++- .../local_storage_class_watcher_test.go | 26 ++++++++++++++ images/sds-local-volume-csi/driver/node.go | 35 ++++++++++--------- .../pkg/utils/node-store-manager.go | 22 ++++++------ 5 files changed, 73 insertions(+), 28 deletions(-) diff --git a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher.go b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher.go index 4f1194e1..71016988 100644 --- a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher.go +++ b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher.go @@ -60,6 +60,9 @@ const ( LocalStorageClassFinalizerName = "storage.deckhouse.io/local-storage-class-controller" LocalStorageClassFinalizerNameOld = "localstorageclass.storage.deckhouse.io" + StorageClassDefaultAnnotationKey = "storageclass.kubernetes.io/is-default-class" + StorageClassDefaultAnnotationValTrue = "true" + AllowVolumeExpansionDefaultValue = true FailedStatusPhase = "Failed" diff --git a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_func.go b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_func.go index f1d881a1..b01357bd 100644 --- a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_func.go +++ b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_func.go @@ -142,7 +142,7 @@ func reconcileLSCUpdateFunc( if hasDiff { log.Info(fmt.Sprintf("[reconcileLSCUpdateFunc] current Storage Class LVMVolumeGroups do not match LocalStorageClass ones. The Storage Class %s will be recreated with new ones", lsc.Name)) - newSC, err := configureStorageClass(lsc) + newSC, err := updateStorageClass(lsc, oldSC) if err != nil { log.Error(err, fmt.Sprintf("[reconcileLSCUpdateFunc] unable to configure a Storage Class for the LocalStorageClass %s", lsc.Name)) upError := updateLocalStorageClassPhase(ctx, cl, lsc, FailedStatusPhase, err.Error()) @@ -703,3 +703,16 @@ func removeFinalizerIfExists(ctx context.Context, cl client.Client, obj metav1.O return removed, nil } + +func updateStorageClass(lsc *v1alpha1.LocalStorageClass, oldSC *v1.StorageClass) (*v1.StorageClass, error) { + newSC, err := configureStorageClass(lsc) + if err != nil { + return nil, err + } + + if oldSC.Annotations != nil { + newSC.Annotations = oldSC.Annotations + } + + return newSC, nil +} diff --git a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_test.go b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_test.go index 4a510703..5f63fe5d 100644 --- a/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_test.go +++ b/images/sds-local-volume-controller/pkg/controller/local_storage_class_watcher_test.go @@ -115,6 +115,23 @@ var _ = Describe(controller.LocalStorageClassCtrlName, func() { performStandartChecksForSC(sc, lvgSpec, nameForLocalStorageClass, controller.LocalStorageClassLvmType, controller.LVMThickType, reclaimPolicyDelete, volumeBindingModeWFFC) }) + It("Annotate_sc_as_default_sc", func() { + sc := &v1.StorageClass{} + err := cl.Get(ctx, client.ObjectKey{Name: nameForLocalStorageClass}, sc) + Expect(err).NotTo(HaveOccurred()) + Expect(sc.Annotations).To(BeNil()) + + sc.Annotations = map[string]string{ + controller.StorageClassDefaultAnnotationKey: controller.StorageClassDefaultAnnotationValTrue, + } + + err = cl.Update(ctx, sc) + Expect(err).NotTo(HaveOccurred()) + Expect(sc.Annotations).To(HaveLen(1)) + Expect(sc.Annotations).To(HaveKeyWithValue(controller.StorageClassDefaultAnnotationKey, controller.StorageClassDefaultAnnotationValTrue)) + + }) + It("Update_local_sc_add_existing_lvg", func() { lvgSpec := []v1alpha1.LocalStorageClassLVG{ {Name: existingThickLVG1Name}, @@ -155,6 +172,15 @@ var _ = Describe(controller.LocalStorageClassCtrlName, func() { performStandartChecksForSC(sc, lvgSpec, nameForLocalStorageClass, controller.LocalStorageClassLvmType, controller.LVMThickType, reclaimPolicyDelete, volumeBindingModeWFFC) }) + It("Check_anotated_sc_after_lsc_update", func() { + sc := &v1.StorageClass{} + err := cl.Get(ctx, client.ObjectKey{Name: nameForLocalStorageClass}, sc) + Expect(err).NotTo(HaveOccurred()) + Expect(sc.Annotations).To(HaveLen(1)) + Expect(sc.Annotations).To(HaveKeyWithValue(controller.StorageClassDefaultAnnotationKey, controller.StorageClassDefaultAnnotationValTrue)) + + }) + It("Update_local_sc_remove_existing_lvg", func() { lvgSpec := []v1alpha1.LocalStorageClassLVG{ {Name: existingThickLVG1Name}, diff --git a/images/sds-local-volume-csi/driver/node.go b/images/sds-local-volume-csi/driver/node.go index 5559daa3..85112e87 100644 --- a/images/sds-local-volume-csi/driver/node.go +++ b/images/sds-local-volume-csi/driver/node.go @@ -207,8 +207,24 @@ func (d *Driver) NodePublishVolume(ctx context.Context, request *csi.NodePublish mountOptions = append(mountOptions, "ro") } + vgName, ok := request.GetVolumeContext()[internal.VGNameKey] + if !ok { + return nil, status.Error(codes.InvalidArgument, "[NodePublishVolume] Volume group name cannot be empty") + } + + devPath := fmt.Sprintf("/dev/%s/%s", vgName, request.VolumeId) + d.log.Debug(fmt.Sprintf("[NodePublishVolume] Checking if device exists: %s", devPath)) + exists, err := d.storeManager.PathExists(devPath) + if err != nil { + return nil, status.Errorf(codes.Internal, "[NodePublishVolume] Error checking if device exists: %v", err) + } + if !exists { + return nil, status.Errorf(codes.NotFound, "[NodePublishVolume] Device %q not found", devPath) + } + d.log.Debug(fmt.Sprintf("[NodePublishVolume] Volume %s operation started", volumeID)) - ok := d.inFlight.Insert(volumeID) + + ok = d.inFlight.Insert(volumeID) if !ok { return nil, status.Errorf(codes.Aborted, VolumeOperationAlreadyExists, volumeID) } @@ -220,21 +236,8 @@ func (d *Driver) NodePublishVolume(ctx context.Context, request *csi.NodePublish switch volCap.GetAccessType().(type) { case *csi.VolumeCapability_Block: d.log.Trace("[NodePublishVolume] Block volume detected.") - vgName, ok := request.GetVolumeContext()[internal.VGNameKey] - if !ok { - return nil, status.Error(codes.InvalidArgument, "[NodeStageVolume] Volume group name cannot be empty") - } - devPath := fmt.Sprintf("/dev/%s/%s", vgName, request.VolumeId) - d.log.Debug(fmt.Sprintf("[NodePublishVolume] Checking if device exists: %s", devPath)) - exists, err := d.storeManager.PathExists(devPath) - if err != nil { - return nil, status.Errorf(codes.Internal, "[NodePublishVolume] Error checking if device exists: %v", err) - } - if !exists { - return nil, status.Errorf(codes.NotFound, "[NodePublishVolume] Device %q not found", devPath) - } - err = d.storeManager.NodePublishVolumeBlock(devPath, target, mountOptions) + err := d.storeManager.NodePublishVolumeBlock(devPath, target, mountOptions) if err != nil { return nil, status.Errorf(codes.Internal, "[NodePublishVolume] Error mounting volume %q at %q: %v", devPath, target, err) } @@ -257,7 +260,7 @@ func (d *Driver) NodePublishVolume(ctx context.Context, request *csi.NodePublish mountOptions = collectMountOptions(fsType, mountVolume.GetMountFlags(), mountOptions) - err := d.storeManager.NodePublishVolumeFS(source, target, fsType, mountOptions) + err := d.storeManager.NodePublishVolumeFS(source, devPath, target, fsType, mountOptions) if err != nil { return nil, status.Errorf(codes.Internal, "[NodePublishVolume] Error bind mounting volume %q. Source: %q. Target: %q. Mount options:%v. Err: %v", volumeID, source, target, mountOptions, err) } diff --git a/images/sds-local-volume-csi/pkg/utils/node-store-manager.go b/images/sds-local-volume-csi/pkg/utils/node-store-manager.go index 150313b2..f917454a 100644 --- a/images/sds-local-volume-csi/pkg/utils/node-store-manager.go +++ b/images/sds-local-volume-csi/pkg/utils/node-store-manager.go @@ -31,7 +31,7 @@ import ( type NodeStoreManager interface { NodeStageVolumeFS(source, target string, fsType string, mountOpts []string, lvmType, lvmThinPoolName string) error NodePublishVolumeBlock(source, target string, mountOpts []string) error - NodePublishVolumeFS(source, target, fsType string, mountOpts []string) error + NodePublishVolumeFS(source, devPath, target, fsType string, mountOpts []string) error Unstage(target string) error Unpublish(target string) error IsNotMountPoint(target string) (bool, error) @@ -173,7 +173,7 @@ func (s *Store) NodePublishVolumeBlock(source, target string, mountOpts []string return nil } -func (s *Store) NodePublishVolumeFS(source, target, fsType string, mountOpts []string) error { +func (s *Store) NodePublishVolumeFS(source, devPath, target, fsType string, mountOpts []string) error { s.Log.Info(" ----== Start NodePublishVolumeFS ==---- ") s.Log.Trace(fmt.Sprintf("[NodePublishVolumeFS] params source=%q target=%q mountOptions=%v", source, target, mountOpts)) isMountPoint := false @@ -197,11 +197,11 @@ func (s *Store) NodePublishVolumeFS(source, target, fsType string, mountOpts []s if isMountPoint { s.Log.Trace(fmt.Sprintf("[NodePublishVolumeFS] target directory %q is a mount point. Check mount", target)) - err := checkMount(s, source, target, mountOpts) + err := checkMount(s, devPath, target, mountOpts) if err != nil { return fmt.Errorf("[NodePublishVolumeFS] failed to check mount info for %q: %w", target, err) } - s.Log.Trace(fmt.Sprintf("[NodePublishVolumeFS] target directory %q is a mount point and already mounted to source %q", target, source)) + s.Log.Trace(fmt.Sprintf("[NodePublishVolumeFS] target directory %q is a mount point and already mounted to source %s. Skipping mount", target, source)) return nil } @@ -283,7 +283,7 @@ func toMapperPath(devPath string) string { return "/dev/mapper/" + mapperPath } -func checkMount(s *Store, source, target string, mountOpts []string) error { +func checkMount(s *Store, devPath, target string, mountOpts []string) error { mntInfo, err := s.NodeStorage.Interface.List() if err != nil { return fmt.Errorf("[checkMount] failed to list mounts: %w", err) @@ -291,19 +291,19 @@ func checkMount(s *Store, source, target string, mountOpts []string) error { for _, m := range mntInfo { if m.Path == target { - if m.Device != source { - return fmt.Errorf("[checkMount] device from mount point %q does not match expected source %q", m.Device, source) + mapperDevicePath := toMapperPath(devPath) + if m.Device != devPath && m.Device != mapperDevicePath { + return fmt.Errorf("[checkMount] device from mount point %q does not match expected source device path %s or mapper device path %s", m.Device, devPath, mapperDevicePath) } + s.Log.Trace(fmt.Sprintf("[checkMount] mount point %s is mounted to device %s", target, m.Device)) if slices.Contains(mountOpts, "ro") { if !slices.Contains(m.Opts, "ro") { return fmt.Errorf("[checkMount] passed mount options contain 'ro' but mount options from mount point %q do not", target) } - - if slices.Equal(m.Opts, mountOpts) { - return fmt.Errorf("mount options %v do not match expected mount options %v", m.Opts, mountOpts) - } + s.Log.Trace(fmt.Sprintf("[checkMount] mount point %s is mounted read-only", target)) } + s.Log.Trace(fmt.Sprintf("[checkMount] mount point %s is mounted to device %s with mount options %v", target, m.Device, m.Opts)) return nil }