From 0007c2abd4ff9fe94672f75816e8ce7e8fd99726 Mon Sep 17 00:00:00 2001 From: Fabio Bertinatto Date: Mon, 22 Jul 2019 14:25:39 +0200 Subject: [PATCH 1/2] Add new refcounter for iSCSI volumes --- pkg/volume/iscsi/BUILD | 1 + pkg/volume/iscsi/iscsi_util.go | 106 +++++++++++++++++++--------- pkg/volume/iscsi/iscsi_util_test.go | 31 -------- 3 files changed, 72 insertions(+), 66 deletions(-) diff --git a/pkg/volume/iscsi/BUILD b/pkg/volume/iscsi/BUILD index 41381d0bb49..48208d1f705 100644 --- a/pkg/volume/iscsi/BUILD +++ b/pkg/volume/iscsi/BUILD @@ -18,6 +18,7 @@ go_library( importpath = "k8s.io/kubernetes/pkg/volume/iscsi", deps = [ "//pkg/features:go_default_library", + "//pkg/kubelet/config:go_default_library", "//pkg/util/mount:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/util:go_default_library", diff --git a/pkg/volume/iscsi/iscsi_util.go b/pkg/volume/iscsi/iscsi_util.go index f3d9a8d3971..6c6e0230731 100644 --- a/pkg/volume/iscsi/iscsi_util.go +++ b/pkg/volume/iscsi/iscsi_util.go @@ -19,6 +19,7 @@ package iscsi import ( "encoding/json" "fmt" + "io/ioutil" "os" "path/filepath" "regexp" @@ -26,10 +27,11 @@ import ( "strings" "time" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog" "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/kubelet/config" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" @@ -162,30 +164,6 @@ func waitForPathToExistInternal(devicePath *string, maxRetries int, deviceTransp return false } -// getDevicePrefixRefCount: given a prefix of device path, find its reference count from /proc/mounts -// returns the reference count to the device and error code -// for services like iscsi construct multiple device paths with the same prefix pattern. -// this function aggregates all references to a service based on the prefix pattern -// More specifically, this prefix semantics is to aggregate disk paths that belong to the same iSCSI target/iqn pair. -// an iSCSI target could expose multiple LUNs through the same IQN, and Linux iSCSI initiator creates disk paths that start the same prefix but end with different LUN number -// When we decide whether it is time to logout a target, we have to see if none of the LUNs are used any more. -// That's where the prefix based ref count kicks in. If we only count the disks using exact match, we could log other disks out. -func getDevicePrefixRefCount(mounter mount.Interface, deviceNamePrefix string) (int, error) { - mps, err := mounter.List() - if err != nil { - return -1, err - } - - // Find the number of references to the device. - refCount := 0 - for i := range mps { - if strings.HasPrefix(mps[i].Path, deviceNamePrefix) { - refCount++ - } - } - return refCount, nil -} - // make a directory like /var/lib/kubelet/plugins/kubernetes.io/iscsi/iface_name/portal-some_iqn-lun-lun_id func makePDNameInternal(host volume.VolumeHost, portal string, iqn string, lun string, iface string) string { return filepath.Join(host.GetPluginDir(iscsiPluginName), "iface-"+iface, portal+"-"+iqn+"-lun-"+lun) @@ -612,7 +590,7 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error { } // if device is no longer used, see if need to logout the target - device, prefix, err := extractDeviceAndPrefix(mntPath) + device, _, err := extractDeviceAndPrefix(mntPath) if err != nil { return err } @@ -650,17 +628,16 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error { c.plugin.targetLocks.LockKey(iqn) defer c.plugin.targetLocks.UnlockKey(iqn) - // if device is no longer used, see if need to logout the target - refCount, err := getDevicePrefixRefCount(c.mounter, prefix) - if err != nil || refCount != 0 { - return nil - } - portals := removeDuplicate(bkpPortal) if len(portals) == 0 { return fmt.Errorf("iscsi detach disk: failed to detach iscsi disk. Couldn't get connected portals from configurations") } + // If device is no longer used, see if need to logout the target + if isSessionBusy(c.iscsiDisk.plugin.host, portals[0], iqn) { + return nil + } + err = util.detachISCSIDisk(c.exec, portals, iqn, iface, volName, initiatorName, found) if err != nil { return fmt.Errorf("failed to finish detachISCSIDisk, err: %v", err) @@ -718,10 +695,16 @@ func (util *ISCSIUtil) DetachBlockISCSIDisk(c iscsiDiskUnmapper, mapPath string) if _, err = os.Stat(devicePath); err != nil { return fmt.Errorf("failed to validate devicePath: %s", devicePath) } - // check if the dev is using mpio and if so mount it via the dm-XX device - if mappedDevicePath := c.deviceUtil.FindMultipathDeviceForDevice(devicePath); mappedDevicePath != "" { - devicePath = mappedDevicePath + + // Lock the target while we determine if we can safely log out or not + c.plugin.targetLocks.LockKey(iqn) + defer c.plugin.targetLocks.UnlockKey(iqn) + + // If device is no longer used, see if need to logout the target + if isSessionBusy(c.iscsiDisk.plugin.host, portals[0], iqn) { + return nil } + // Detach a volume from kubelet node err = util.detachISCSIDisk(c.exec, portals, iqn, iface, volName, initiatorName, found) if err != nil { @@ -897,3 +880,56 @@ func cloneIface(b iscsiDiskMounter) error { } return lastErr } + +// isSessionBusy determines if the iSCSI session is busy by counting both FS and block volumes in use. +func isSessionBusy(host volume.VolumeHost, portal, iqn string) bool { + fsDir := host.GetPluginDir(iscsiPluginName) + countFS, err := getVolCount(fsDir, portal, iqn) + if err != nil { + klog.Errorf("iscsi: could not determine FS volumes in use: %v", err) + return true + } + + blockDir := host.GetVolumeDevicePluginDir(iscsiPluginName) + countBlock, err := getVolCount(blockDir, portal, iqn) + if err != nil { + klog.Errorf("iscsi: could not determine block volumes in use: %v", err) + return true + } + + return countFS+countBlock > 1 +} + +// getVolCount returns the number of volumes in use by the kubelet. +// It does so by counting the number of directories prefixed by the given portal and IQN. +func getVolCount(dir, portal, iqn string) (int, error) { + // The topmost dirs are named after the ifaces, e.g., iface-default or iface-127.0.0.1:3260:pv0 + contents, err := ioutil.ReadDir(dir) + if err != nil { + return 0, err + } + + // Inside each iface dir, we look for volume dirs prefixed by the given + // portal + iqn, e.g., 127.0.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-2 + var counter int + for _, c := range contents { + if !c.IsDir() || c.Name() == config.DefaultKubeletVolumeDevicesDirName { + continue + } + + mounts, err := ioutil.ReadDir(filepath.Join(dir, c.Name())) + if err != nil { + return 0, err + } + + for _, m := range mounts { + volumeMount := m.Name() + prefix := portal + "-" + iqn + if strings.HasPrefix(volumeMount, prefix) { + counter++ + } + } + } + + return counter, nil +} diff --git a/pkg/volume/iscsi/iscsi_util_test.go b/pkg/volume/iscsi/iscsi_util_test.go index 22093cc479d..d4ed4c5f768 100644 --- a/pkg/volume/iscsi/iscsi_util_test.go +++ b/pkg/volume/iscsi/iscsi_util_test.go @@ -28,37 +28,6 @@ import ( "k8s.io/kubernetes/pkg/volume" ) -func TestGetDevicePrefixRefCount(t *testing.T) { - fm := &mount.FakeMounter{ - MountPoints: []mount.MountPoint{ - {Device: "/dev/sdb", - Path: "/127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-0"}, - {Device: "/dev/sdb", - Path: "/127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-1"}, - {Device: "/dev/sdb", - Path: "/127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-2"}, - {Device: "/dev/sdb", - Path: "/127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-3"}, - }, - } - - tests := []struct { - devicePrefix string - expectedRefs int - }{ - { - "/127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00", - 4, - }, - } - - for i, test := range tests { - if refs, err := getDevicePrefixRefCount(fm, test.devicePrefix); err != nil || test.expectedRefs != refs { - t.Errorf("%d. GetDevicePrefixRefCount(%s) = %d, %v; expected %d, nil", i, test.devicePrefix, refs, err, test.expectedRefs) - } - } -} - func TestExtractDeviceAndPrefix(t *testing.T) { devicePath := "127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00" mountPrefix := "/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/" + devicePath From 44d7510ee05040ce90337215880c7a76b51e7404 Mon Sep 17 00:00:00 2001 From: Fabio Bertinatto Date: Wed, 24 Jul 2019 14:21:36 +0200 Subject: [PATCH 2/2] Add unit test for iSCSI refcounter --- pkg/volume/iscsi/iscsi_util_test.go | 82 +++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/pkg/volume/iscsi/iscsi_util_test.go b/pkg/volume/iscsi/iscsi_util_test.go index d4ed4c5f768..c8560297711 100644 --- a/pkg/volume/iscsi/iscsi_util_test.go +++ b/pkg/volume/iscsi/iscsi_util_test.go @@ -19,6 +19,7 @@ package iscsi import ( "errors" "fmt" + "io/ioutil" "os" "path/filepath" "reflect" @@ -329,3 +330,84 @@ func TestClonedIfaceUpdateError(t *testing.T) { } } + +func TestGetVolCount(t *testing.T) { + testCases := []struct { + name string + portal string + iqn string + count int + }{ + { + name: "wrong portal, no volumes", + portal: "192.168.0.2:3260", // incorrect IP address + iqn: "iqn.2003-01.io.k8s:e2e.volume-1", + count: 0, + }, + { + name: "wrong iqn, no volumes", + portal: "127.0.0.1:3260", + iqn: "iqn.2003-01.io.k8s:e2e.volume-3", // incorrect volume + count: 0, + }, + { + name: "single volume", + portal: "192.168.0.1:3260", + iqn: "iqn.2003-01.io.k8s:e2e.volume-1", + count: 1, + }, + { + name: "two volumes", + portal: "127.0.0.1:3260", + iqn: "iqn.2003-01.io.k8s:e2e.volume-1", + count: 2, + }, + } + + // This will create a dir structure like this: + // /tmp/refcounter555814673 + // ├── iface-127.0.0.1:3260:pv1 + // │   └── 127.0.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-3 + // └── iface-127.0.0.1:3260:pv2 + // ├── 127.0.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-2 + // └── 192.168.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-1 + + baseDir, err := createFakePluginDir() + if err != nil { + t.Errorf("error creating fake plugin dir: %v", err) + } + defer os.RemoveAll(baseDir) + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + count, err := getVolCount(baseDir, tc.portal, tc.iqn) + if err != nil { + t.Errorf("expected no error, got %v", err) + } + if count != tc.count { + t.Errorf("expected %d volumes, got %d", tc.count, count) + } + }) + } +} + +func createFakePluginDir() (string, error) { + dir, err := ioutil.TempDir("", "refcounter") + if err != nil { + return "", err + } + + subdirs := []string{ + "iface-127.0.0.1:3260:pv1/127.0.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-3", + "iface-127.0.0.1:3260:pv2/127.0.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-2", + "iface-127.0.0.1:3260:pv2/192.168.0.1:3260-iqn.2003-01.io.k8s:e2e.volume-1-lun-1", + } + + for _, d := range subdirs { + if err := os.MkdirAll(filepath.Join(dir, d), os.ModePerm); err != nil { + return dir, err + } + } + + return dir, err +}