From f60a0c86602088159e5b0c5248073ec0545eb18c Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 17 Mar 2020 11:37:18 +0100 Subject: [PATCH 1/2] Fix mount options in iSCSI volumes Do not mount volumes in WaitForAttach(), mount them in MountDevice() instead. They will get proper mount options this way. --- pkg/volume/iscsi/iscsi_util.go | 72 +++++++++++----------------------- 1 file changed, 23 insertions(+), 49 deletions(-) diff --git a/pkg/volume/iscsi/iscsi_util.go b/pkg/volume/iscsi/iscsi_util.go index b46ecd9f235..9e32c405b11 100644 --- a/pkg/volume/iscsi/iscsi_util.go +++ b/pkg/volume/iscsi/iscsi_util.go @@ -37,6 +37,7 @@ import ( "k8s.io/kubernetes/pkg/kubelet/config" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" + "k8s.io/kubernetes/pkg/volume/util/types" ) const ( @@ -194,7 +195,9 @@ func (util *ISCSIUtil) MakeGlobalVDPDName(iscsi iscsiDisk) string { return makeVDPDNameInternal(iscsi.plugin.host, iscsi.Portals[0], iscsi.Iqn, iscsi.Lun, iscsi.Iface) } -func (util *ISCSIUtil) persistISCSI(conf iscsiDisk, mnt string) error { +// persistISCSIFile saves iSCSI volume configuration for DetachDisk +// into given directory. +func (util *ISCSIUtil) persistISCSIFile(conf iscsiDisk, mnt string) error { file := filepath.Join(mnt, "iscsi.json") fp, err := os.Create(file) if err != nil { @@ -457,61 +460,32 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) (string, error) { } klog.V(5).Infof("iscsi: AttachDisk devicePath: %s", devicePath) - // run global mount path related operations based on volumeMode - return globalPDPathOperation(b)(b, devicePath, util) + + if err = util.persistISCSI(b); err != nil { + // Return uncertain error so kubelet calls Unmount / Unmap when the pod + // is deleted. + return "", types.NewUncertainProgressError(err.Error()) + } + return devicePath, util.persistISCSI(b) } -// globalPDPathOperation returns global mount path related operations based on volumeMode. -// If the volumeMode is 'Filesystem' or not defined, plugin needs to create a dir, persist -// iscsi configurations, and then format/mount the volume. -// If the volumeMode is 'Block', plugin creates a dir and persists iscsi configurations. -// Since volume type is block, plugin doesn't need to format/mount the volume. -func globalPDPathOperation(b iscsiDiskMounter) func(iscsiDiskMounter, string, *ISCSIUtil) (string, error) { +// persistISCSI saves iSCSI volume configuration for DetachDisk into global +// mount / map directory. +func (util *ISCSIUtil) persistISCSI(b iscsiDiskMounter) error { klog.V(5).Infof("iscsi: AttachDisk volumeMode: %s", b.volumeMode) + var globalPDPath string if b.volumeMode == v1.PersistentVolumeBlock { - // If the volumeMode is 'Block', plugin don't need to format the volume. - return func(b iscsiDiskMounter, devicePath string, util *ISCSIUtil) (string, error) { - globalPDPath := b.manager.MakeGlobalVDPDName(*b.iscsiDisk) - // Create dir like /var/lib/kubelet/plugins/kubernetes.io/iscsi/volumeDevices/{ifaceName}/{portal-some_iqn-lun-lun_id} - if err := os.MkdirAll(globalPDPath, 0750); err != nil { - klog.Errorf("iscsi: failed to mkdir %s, error", globalPDPath) - return "", err - } - // Persist iscsi disk config to json file for DetachDisk path - util.persistISCSI(*(b.iscsiDisk), globalPDPath) - - return devicePath, nil - } + globalPDPath = b.manager.MakeGlobalVDPDName(*b.iscsiDisk) + } else { + globalPDPath = b.manager.MakeGlobalPDName(*b.iscsiDisk) } - // If the volumeMode is 'Filesystem', plugin needs to format the volume - // and mount it to globalPDPath. - return func(b iscsiDiskMounter, devicePath string, util *ISCSIUtil) (string, error) { - globalPDPath := b.manager.MakeGlobalPDName(*b.iscsiDisk) - notMnt, err := b.mounter.IsLikelyNotMountPoint(globalPDPath) - if err != nil && !os.IsNotExist(err) { - return "", fmt.Errorf("Heuristic determination of mount point failed:%v", err) - } - // Return confirmed devicePath to caller - if !notMnt { - klog.Infof("iscsi: %s already mounted", globalPDPath) - return devicePath, nil - } - // Create dir like /var/lib/kubelet/plugins/kubernetes.io/iscsi/{ifaceName}/{portal-some_iqn-lun-lun_id} - if err := os.MkdirAll(globalPDPath, 0750); err != nil { - klog.Errorf("iscsi: failed to mkdir %s, error", globalPDPath) - return "", err - } - // Persist iscsi disk config to json file for DetachDisk path - util.persistISCSI(*(b.iscsiDisk), globalPDPath) - - err = b.mounter.FormatAndMount(devicePath, globalPDPath, b.fsType, nil) - if err != nil { - klog.Errorf("iscsi: failed to mount iscsi volume %s [%s] to %s, error %v", devicePath, b.fsType, globalPDPath, err) - } - - return devicePath, nil + if err := os.MkdirAll(globalPDPath, 0750); err != nil { + klog.Errorf("iscsi: failed to mkdir %s, error", globalPDPath) + return err } + // Persist iscsi disk config to json file for DetachDisk path + return util.persistISCSIFile(*(b.iscsiDisk), globalPDPath) } // Delete 1 block device of the form "sd*" From 58129fd12cbe1ac4a975c41ed3bc272a73918fce Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 17 Mar 2020 11:41:00 +0100 Subject: [PATCH 2/2] Fix mount options in FibreChannel volumes Do not mount volumes in WaitForAttach(), mount them in MountDevice() instead. They will get proper mount options this way. --- pkg/volume/fc/fc_test.go | 6 +----- pkg/volume/fc/fc_util.go | 30 +----------------------------- pkg/volume/iscsi/BUILD | 1 + pkg/volume/iscsi/iscsi_util.go | 2 +- 4 files changed, 4 insertions(+), 35 deletions(-) diff --git a/pkg/volume/fc/fc_test.go b/pkg/volume/fc/fc_test.go index 63d5a08e09a..763081d8efb 100644 --- a/pkg/volume/fc/fc_test.go +++ b/pkg/volume/fc/fc_test.go @@ -24,7 +24,7 @@ import ( "strings" "testing" - "k8s.io/utils/exec/testing" + testingexec "k8s.io/utils/exec/testing" "k8s.io/utils/mount" v1 "k8s.io/api/core/v1" @@ -122,10 +122,6 @@ func (fake *fakeDiskManager) AttachDisk(b fcDiskMounter) (string, error) { if err != nil { return "", err } - // Simulate the global mount so that the fakeMounter returns the - // expected number of mounts for the attached disk. - b.mounter.Mount(globalPath, globalPath, b.fsType, nil) - fake.attachCalled = true return "", nil } diff --git a/pkg/volume/fc/fc_util.go b/pkg/volume/fc/fc_util.go index b2ac628cd22..a48e7ae349b 100644 --- a/pkg/volume/fc/fc_util.go +++ b/pkg/volume/fc/fc_util.go @@ -27,7 +27,6 @@ import ( "k8s.io/klog" "k8s.io/utils/mount" - v1 "k8s.io/api/core/v1" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" ) @@ -242,34 +241,7 @@ func (util *fcUtil) AttachDisk(b fcDiskMounter) (string, error) { return "", err } - // If the volumeMode is 'Block', plugin don't have to format the volume. - // The globalPDPath will be created by operationexecutor. Just return devicePath here. - klog.V(5).Infof("fc: AttachDisk volumeMode: %s, devicePath: %s", b.volumeMode, devicePath) - if b.volumeMode == v1.PersistentVolumeBlock { - return devicePath, nil - } - - // mount it - globalPDPath := util.MakeGlobalPDName(*b.fcDisk) - if err := os.MkdirAll(globalPDPath, 0750); err != nil { - return devicePath, fmt.Errorf("fc: failed to mkdir %s, error", globalPDPath) - } - - noMnt, err := b.mounter.IsLikelyNotMountPoint(globalPDPath) - if err != nil { - return devicePath, fmt.Errorf("Heuristic determination of mount point failed:%v", err) - } - if !noMnt { - klog.Infof("fc: %s already mounted", globalPDPath) - return devicePath, nil - } - - err = b.mounter.FormatAndMount(devicePath, globalPDPath, b.fsType, b.mountOptions) - if err != nil { - return devicePath, fmt.Errorf("fc: failed to mount fc volume %s [%s] to %s, error %v", devicePath, b.fsType, globalPDPath, err) - } - - return devicePath, err + return devicePath, nil } // DetachDisk removes scsi device file such as /dev/sdX from the node. diff --git a/pkg/volume/iscsi/BUILD b/pkg/volume/iscsi/BUILD index 846edeb3975..af6b3935d82 100644 --- a/pkg/volume/iscsi/BUILD +++ b/pkg/volume/iscsi/BUILD @@ -20,6 +20,7 @@ go_library( "//pkg/kubelet/config:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/util:go_default_library", + "//pkg/volume/util/types:go_default_library", "//pkg/volume/util/volumepathhandler:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/volume/iscsi/iscsi_util.go b/pkg/volume/iscsi/iscsi_util.go index 9e32c405b11..ec9bdda8791 100644 --- a/pkg/volume/iscsi/iscsi_util.go +++ b/pkg/volume/iscsi/iscsi_util.go @@ -466,7 +466,7 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) (string, error) { // is deleted. return "", types.NewUncertainProgressError(err.Error()) } - return devicePath, util.persistISCSI(b) + return devicePath, nil } // persistISCSI saves iSCSI volume configuration for DetachDisk into global