Merge pull request #41196 from bigstepinc/master

Automatic merge from submit-queue

Fix for Premature iSCSI logout #39202.

**What this PR does / why we need it**:

Modifies the iSCSI volume plugin code to prevent premature iSCSI logouts and the establishment of multiple iSCSI connections to the same target in certain cases.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #39202, fixes #41041, fixes #40941

**Special notes for your reviewer**:

The existing iSCSI connections are now rescanned on every AttachDisk call to discover newly created LUNs.

The disk mount points now contain an additional directory in the path corresponding to the disk iface that is later used for iSCSI logout.

The device prefixes that are used to count the existing references to the portal-target pair now contain the whole path including the mount point until the lun index.

**Release note**:
```release-note
Fixed issues #39202, #41041 and #40941 that caused the iSCSI connections to be prematurely closed when deleting a pod with an iSCSI persistent volume attached and that prevented the use of newly created LUNs on targets with preestablished connections.
```

pkg/volume/iscsi/iscsi_util.go

@@ -90,21 +90,22 @@ func getDevicePrefixRefCount(mounter mount.Interface, deviceNamePrefix string) (
 	return refCount, nil
 }
 
-// make a directory like /var/lib/kubelet/plugins/kubernetes.io/iscsi/portal-some_iqn-lun-lun_id
+// 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) string {
+func makePDNameInternal(host volume.VolumeHost, portal string, iqn string, lun string, iface string) string {
-	return path.Join(host.GetPluginDir(iscsiPluginName), portal+"-"+iqn+"-lun-"+lun)
+	return path.Join(host.GetPluginDir(iscsiPluginName), "iface-"+iface, portal+"-"+iqn+"-lun-"+lun)
 }
 
 type ISCSIUtil struct{}
 
 func (util *ISCSIUtil) MakeGlobalPDName(iscsi iscsiDisk) string {
-	return makePDNameInternal(iscsi.plugin.host, iscsi.portals[0], iscsi.iqn, iscsi.lun)
+	return makePDNameInternal(iscsi.plugin.host, iscsi.portals[0], iscsi.iqn, iscsi.lun, iscsi.iface)
 }
 
 func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {
 	var devicePath string
 	var devicePaths []string
 	var iscsiTransport string
+
 	out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", b.iface, "-o", "show"})
 	if err != nil {
 		glog.Errorf("iscsi: could not read iface %s error: %s", b.iface, string(out))
@@ -112,8 +113,16 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {
 	}
 
 	iscsiTransport = extractTransportname(string(out))
+
 	bkpPortal := b.portals
 	for _, tp := range bkpPortal {
+		// Rescan sessions to discover newly mapped LUNs. Do not specify the interface when rescanning
+		// to avoid establishing additional sessions to the same target.
+		out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-T", b.iqn, "-R"})
+		if err != nil {
+			glog.Errorf("iscsi: failed to rescan session with error: %s (%v)", string(out), err)
+		}
+
 		if iscsiTransport == "" {
 			glog.Errorf("iscsi: could not find transport name in iface %s", b.iface)
 			return errors.New(fmt.Sprintf("Could not parse iface file for %s", b.iface))
@@ -207,18 +216,29 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error {
 			return err
 		}
 		refCount, err := getDevicePrefixRefCount(c.mounter, prefix)
-
 		if err == nil && refCount == 0 {
-			// this portal/iqn are no longer referenced, log out
+			// This portal/iqn/iface is no longer referenced, log out.
-			// extract portal and iqn from device path
+			// Extract the portal and iqn from device path.
 			portal, iqn, err := extractPortalAndIqn(device)
 			if err != nil {
 				return err
 			}
-			glog.Infof("iscsi: log out target %s iqn %s", portal, iqn)
+			// Extract the iface from the mountPath and use it to log out. If the iface
-			out, err := c.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", portal, "-T", iqn, "--logout"})
+			// is not found, maintain the previous behavior to facilitate kubelet upgrade.
-			if err != nil {
+			// Logout may fail as no session may exist for the portal/IQN on the specified interface.
-				glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
+			iface, found := extractIface(mntPath)
+			if found {
+				glog.Infof("iscsi: log out target %s iqn %s iface %s", portal, iqn, iface)
+				out, err := c.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", portal, "-T", iqn, "-I", iface, "--logout"})
+				if err != nil {
+					glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
+				}
+			} else {
+				glog.Infof("iscsi: log out target %s iqn %s", portal, iqn)
+				out, err := c.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", portal, "-T", iqn, "--logout"})
+				if err != nil {
+					glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
+				}
 			}
 		}
 	}
@@ -248,15 +268,26 @@ func extractDeviceAndPrefix(mntPath string) (string, string, error) {
 		return "", "", fmt.Errorf("iscsi detach disk: malformatted mnt path: %s", mntPath)
 	}
 	device := mntPath[(ind + 1):]
-	// strip -lun- from device path
+	// strip -lun- from mount path
-	ind = strings.LastIndex(device, "-lun-")
+	ind = strings.LastIndex(mntPath, "-lun-")
 	if ind < 0 {
 		return "", "", fmt.Errorf("iscsi detach disk: malformatted mnt path: %s", mntPath)
 	}
-	prefix := device[:ind]
+	prefix := mntPath[:ind]
 	return device, prefix, nil
 }
 
+func extractIface(mntPath string) (string, bool) {
+	re := regexp.MustCompile(`.+/iface-([^/]+)/.+`)
+
+	re_output := re.FindStringSubmatch(mntPath)
+	if re_output != nil {
+		return re_output[1], true
+	}
+
+	return "", false
+}
+
 func extractPortalAndIqn(device string) (string, string, error) {
 	ind1 := strings.Index(device, "-")
 	if ind1 < 0 {

pkg/volume/iscsi/iscsi_util_test.go

@@ -57,10 +57,24 @@ func TestGetDevicePrefixRefCount(t *testing.T) {
 
 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
 	lun := "-lun-0"
-	device, prefix, err := extractDeviceAndPrefix("/var/lib/kubelet/plugins/kubernetes.io/iscsi/" + devicePath + lun)
+	device, prefix, err := extractDeviceAndPrefix(mountPrefix + lun)
-	if err != nil || device != (devicePath+lun) || prefix != devicePath {
+	if err != nil || device != (devicePath+lun) || prefix != mountPrefix {
-		t.Errorf("extractDeviceAndPrefix: expected %s and %s, got %v %s and %s", devicePath+lun, devicePath, err, device, prefix)
+		t.Errorf("extractDeviceAndPrefix: expected %s and %s, got %v %s and %s", devicePath+lun, mountPrefix, err, device, prefix)
+	}
+}
+
+func TestExtractIface(t *testing.T) {
+	ifaceName := "default"
+	devicePath := "127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-0"
+	iface, found := extractIface("/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-" + ifaceName + "/" + devicePath)
+	if !found || iface != ifaceName {
+		t.Errorf("extractIface: expected %s and %t, got %s and %t", ifaceName, true, iface, found)
+	}
+	iface, found = extractIface("/var/lib/kubelet/plugins/kubernetes.io/iscsi/" + devicePath)
+	if found || iface != "" {
+		t.Errorf("extractIface: expected %s and %t, got %s and %t", "", false, iface, found)
 	}
 }