Log out from multiple target portals when using iscsi storage plugin #46239

mtanino · 2017-05-22T19:15:28Z

What this PR does / why we need it:

When using iscsi storage with multiple target portal (TP) addresses
and multipathing the volume manager logs on to the IQN for all
portal addresses, but when a pod gets destroyed the volume manager
only logs out for the primary TP and sessions for another TPs are
always remained.

This patch adds mount points for all TPs, and then log out from all
TPs when a pod is destroyed. If a TP is referred from another pods,
the connection will be remained as usual.

Which issue this PR fixes
fixes #45394

Special notes for your reviewer:

Release note:

iscsi storage plugin: Fix dangling session when using multiple target portal addresses.

k8s-ci-robot · 2017-05-22T19:15:36Z

Hi @mtanino. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gnufied · 2017-05-22T20:55:49Z

/assign @rootfs

rootfs · 2017-05-23T16:19:33Z

@k8s-bot ok to test

tobad357 · 2017-05-24T14:43:44Z

pkg/volume/iscsi/iscsi_util.go

+			portals = append(portals, tp)
+		}
+	}
+	return portals


My go fu is not great but this looks like it will return an array with empty elements at the end in case it removes some duplicates which would mean the kubelet tries to execute iscsiadm commands on empty data.

This go playground example avoids it, maybe can use that
https://play.golang.org/p/q3bZ3hpOzD

Thank you for your comment. Your suggestion looks good.
My go is not great too, so I'm curious what kind of input causes empty elements return?

My bad
I expected the len of the new array to be 3 and the last element empty in the case
of having a list of three portals with on set of duplicates

See
https://play.golang.org/p/hkm8_bhlOy

tobad357 · 2017-05-24T14:44:02Z

pkg/volume/iscsi/iscsi_util.go

+	return portals
+}
+
+// If all mntPaths indicate global mounts, then return ture. Otherwise return false.


Nit: ture = true

tobad357 · 2017-05-24T14:44:40Z

pkg/volume/iscsi/iscsi_util.go

+			return false
+		}
+	}
+	return flag


can just return true here as flag isn't used anywhere

Correct. Thanks.

tobad357 · 2017-05-24T14:49:37Z

pkg/volume/iscsi/iscsi_util.go

+						glog.Errorf("iscsi: failed to delete node record Error: %s", string(out))
+					}
+				} else {
+					glog.Infof("iscsi: log out target %s iqn %s", portal, iqn)


@rootfs how long backwards compatibility is promised.
Can this code be removed after X versions of k8s.
If so would be good to add a todo for when it should be removed

a todo is good for note taking

OK. Will do.

tobad357 · 2017-05-24T15:02:23Z

pkg/volume/iscsi/iscsi_util.go

+
+	for _, mntPath := range mntPaths {
+		// if device is no longer used, see if need to logout the target
+		if cnt == 0 {


I'm not quite following the logic here
If there is two mntpaths where the first one has a cnt of 0 and the first one the second one a count of 2.
Won't we skip logging out the first one?

Agreed. This logic should be changed like following.

In case of multipath with two portals, mntpaths has two global mount paths like;
/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/x.x.x.10:3260-iqn.aaa
/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/x.x.x.20:3260-iqn.bbb

Then, we should;
(1) Unmount all mntPaths
(2) Count num(cnt) of remained devices with GetDeviceNameFromMount
(3) If cnt == 0,then logout from these two iSCSI sessions

Any concerns?

Is there a reason we need to wait to log out one device until the other one has cnt == 0
If we don't have to wait and just log out any device with cnt == 0 then we can combine the loops

The cnt shows that the number of remained devices
If we have two remaining paths like this, then GetDeviceNameFromMount returns '2'.

/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/x.x.x.10:3260-iqn.aaa
/var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/x.x.x.20:3260-iqn.bbb

In this case, I think we should unmount these paths at first. Then if these unmount is succeeded and cnt == 0, it means no one is using iSCSI connection so we can logout each iSCSI connections step by step. so I'm using divided for two loop for these steps.

But can we combine these steps into single for loop? Please let me know if you have good idea.
Thanks,

tobad357 · 2017-05-24T15:03:11Z

pkg/volume/iscsi/iscsi_util.go

-			// This portal/iqn/iface is no longer referenced, log out.
-			// Extract the portal and iqn from device path.
-			portal, iqn, err := extractPortalAndIqn(device)
+		if err = c.mounter.Unmount(mntPath); err != nil {


By doing Unmount first and GetDeviceNameFromMount second can't the cnt, cnt-- be skipped
and only use cnt

mtanino · 2017-05-26T22:19:38Z

@k8s-bot ok to test

mtanino · 2017-05-26T22:40:13Z

@rootfs
Can you take a look of this fix?

mtanino · 2017-05-27T14:37:13Z

@k8s-bot pull-kubernetes-cross test this

rootfs · 2017-05-30T14:47:04Z

pkg/volume/iscsi/iscsi_util.go

+		if len(bkpPortal) == 1 {
+			globalPDPath = b.manager.MakeGlobalPDName(*b.iscsiDisk, "")
+		} else {
+			//


I'll remove it.

rootfs · 2017-05-30T14:50:21Z

@humblec

humblec · 2017-05-30T17:45:47Z

pkg/volume/iscsi/iscsi_util.go

@@ -167,7 +171,11 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {

 	iscsiTransport = extractTransportname(string(out))

-	bkpPortal := b.portals
+	bkpPortal := removeDuplicate(b.portals)


Why we need removeDuplicate in attachdisk, can you please give some details on why we need this ?

At first I thought if user passes duplicate portal IPs like this, iscsiadm will be called multiple times unnecessarily, but it was my misunderstanding.

iscsi: targetPortal: 192.168.122.85:3260 portals: ['192.168.122.85:3260', '192.168.122.7:3260'] iqn: iqn.2017-05.com.example:rhel7

Actually waitForPathToExist() checks existing path so even if user specify duplicate portal IPs in the config, duplicate IPs will be skipped.

I'll remove removeDuplicate() logic from attachdisk()

Thanks,

humblec · 2017-05-30T17:47:08Z

pkg/volume/iscsi/iscsi_util.go

@@ -167,7 +171,11 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {

 	iscsiTransport = extractTransportname(string(out))

-	bkpPortal := b.portals
+	bkpPortal := removeDuplicate(b.portals)
+	if len(bkpPortal) == 0 {


Its perfectly valid that portal can be empty. I may be missing the logic here, can you please fill me in?

same above.

humblec · 2017-05-30T17:50:14Z

pkg/volume/iscsi/disk_manager.go

@@ -27,16 +27,16 @@ import (

 // Abstract interface to disk operations.
 type diskManager interface {
-	MakeGlobalPDName(disk iscsiDisk) string
+	MakeGlobalPDName(disk iscsiDisk, portal string) string


portal is a known member here in this code path as []string. May be you could use some other name to avoid confusion. Just a thought.

humblec · 2017-05-30T18:02:23Z

@mtanino Thanks for this PR ! I have few comments. Also it would be appreciated if you can explain some more on the bind mount approach followed in attachdisk for better understanding, I failed to get it in quick look.

humblec · 2017-05-30T18:05:43Z

@mtanino : The test failure looks to be valid as well.

I0527 08:03:19.329] pkg/volume/util/device_util.go:31: cannot use deviceHandler literal (type *deviceHandler) as type DeviceUtil in return argument:
I0527 08:03:19.329] 	*deviceHandler does not implement DeviceUtil (missing FindDevicemapperName method)
I0527 08:03:19.329] !!! [0527 08:03:19] Call tree:
I0527 08:03:19.330] !!! [0527 08:03:19]  1: /go/src/k8s.io/kubernetes/hack/lib/golang.sh:690 kube::golang::build_binaries_for_platform(...)
I0527 08:03:19.330] !!! [0527 08:03:19]  2: hack/make-rules/build.sh:27 kube::golang::build_binaries(...)
I0527 08:03:19.336] Makefile:89: recipe for target 'all' failed
I0527 08:03:19.339] Makefile:396: recipe for target 'cross' failed

humblec · 2017-05-30T18:13:52Z

@mtanino also the PR says, logout from multiple target portals when using iscsi plugin, however it change the code in attachdisk()..etc, where my expectation was that, the change would be more or less in detachdisk(). It could be better if you can split this PR into different commits and organize the changes. It will make the review process bit easier. At time of merging we could squash it .

mtanino · 2017-05-30T19:17:17Z

@humblec

where my expectation was that, the change would be more or less in detachdisk().

In my understanding,

Currently only first path is mounted under kubelet dir even if user specify multiple portals.

/dev/mapper/mpathb on /var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/192.168.122.85:3260-iqn.2017-05.com.example:rhel7-lun-0

DetachDisk() collects iSCSi connection information from this mount path.
But we don't have 2nd and subsequent sessions information so that we can't logout from them.

Is this correct?

In my approach, In order to logout from multiple iSCSI sessions, we need following two fix in this PR.

Add mount points for all iSCSI sessions at Attachdisk()
Logout from multiple portals using above mount points information at DetachDisk()

As a result, if user specify multiple portals like this,

    iscsi:
      targetPortal: 192.168.122.85:3260
      portals: ['192.168.122.7:3260']
      iqn: iqn.2017-05.com.example:rhel7

we can see mount points like this.

/dev/mapper/mpathb on /var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/192.168.122.85:3260-iqn.2017-05.com.example:rhel7-lun-0 type ext4 (ro,relatime,block_validity,delalloc,barrier,user_xattr,acl,stripe=2048)
/dev/mapper/mpathb on /var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/192.168.122.7:3260-iqn.2017-05.com.example:rhel7-lun-0 type ext4 (ro,relatime,block_validity,delalloc,barrier,user_xattr,acl,stripe=2048)
/dev/mapper/mpathb on /var/lib/kubelet/pods/f3838bc8-4562-11e7-8fd5-525400645a80/volumes/kubernetes.io~iscsi/iscsipd-rw type ext4 (ro,relatime,block_validity,delalloc,barrier,user_xattr,acl,stripe=2048)

Then we can properly logout using these two iSCSI connection information.

Any thought?

Thanks,
Mitsuhiro

mtanino · 2017-05-30T19:27:03Z

@humblec
I'll take a look device_util.go as well.

humblec · 2017-05-30T19:36:52Z

@mtanino thanks for giving more details about the approach! iirc, when there are entries in portals field, we form paths for each portal. You could validate this in /dev/disk/by-path. Please pay attention to the portal IPs ( 10.0.2.{15,16,17,18} and also the IQN. These ( as in below output) all belongs to same mpath device. Also, we could list the active sessions from iscsiadm -m session . I think once we confirmed there is no active mount/ref exist on this mpath device, we can make use of existing iscsi sessions or parse below path to understand the existing connections and logout accordingly.

# ll /dev/disk/by-path/
lrwxrwxrwx. 1 root root 9 Feb 16 15:58 ip-10.0.2.15:3260-iscsi-iqn.2016-04.test.com:storage.target00-lun-0 -> ../../sdb
lrwxrwxrwx. 1 root root 9 Feb 16 15:41 ip-10.0.2.16:3260-iscsi-iqn.2016-04.test.com:storage.target00-lun-0 -> ../../sdc
lrwxrwxrwx. 1 root root 9 Feb 16 15:58 ip-10.0.2.17:3260-iscsi-iqn.2016-04.test.com:storage.target00-lun-0 -> ../../sdd
lrwxrwxrwx. 1 root root 9 Feb 16 15:41 ip-10.0.2.18:3260-iscsi-iqn.2016-04.test.com:storage.target00-lun-0 -> ../../sde

@rootfs Any thoughts?

tobad357 · 2017-05-31T08:25:43Z

@mtanino if going down the route of saving a json file wont that become an issue in the scenario where the umount goes through but something happens with the iscsi logout.
In that case the kubelet will not be able to logout the sessions in successive tries

rootfs · 2017-05-31T13:15:53Z

@tobad357 the json file lives on the host, so it will survive an unmount and iscsi logout.

rootfs · 2017-05-31T20:16:48Z

pkg/volume/iscsi/iscsi_util.go

+	file := path.Join(mnt, "iscsi.json")
+	fp, err := os.Create(file)
+	if err != nil {
+		return fmt.Errorf("iscsi: create err %s/%s", file, err)


iscsi: create %s err %s

rootfs · 2017-05-31T20:17:32Z

pkg/volume/iscsi/iscsi_util.go

+	file := path.Join(mnt, "iscsi.json")
+	fp, err := os.Open(file)
+	if err != nil {
+		return fmt.Errorf("iscsi: open err %s/%s", file, err)


rootfs · 2017-05-31T20:22:54Z

/approve

rootfs · 2017-05-31T20:26:59Z

pkg/volume/iscsi/iscsi_util.go

+	return nil
+}
+
+func (util *ISCSIUtil) loadISCSI(conf *iscsiDisk, mnt string) error {


nit: make a note here, the iscsi config json is not deleted

When using iscsi storage with multiple target portal (TP) addresses and multipathing the volume manager logs on to the IQN for all portal addresses, but when a pod gets destroyed the volume manager only logs out for the primary TP and sessions for another TPs are always remained. This patch adds methods to store and load iscsi disk configrations, then uses the stored config at DetachDisk path. Fix kubernetes#45394

k8s-ci-robot · 2017-05-31T22:26:41Z

@mtanino: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	340a78211e46ab0fe91a4edb06516a4d6452963f	link	`@k8s-bot pull-kubernetes-cross test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rootfs · 2017-05-31T22:49:09Z

/lgtm

mtanino · 2017-06-01T00:49:44Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

mtanino · 2017-06-01T00:50:15Z

@k8s-bot pull-kubernetes-node-e2e test this

humblec · 2017-06-01T12:37:20Z

/lgtm. Thanks !!

mtanino · 2017-06-01T14:57:23Z

After merged the fix, can we cherry pick the PR to v1.6.x tree?

childsb · 2017-06-01T15:27:09Z

/approve

k8s-github-robot · 2017-06-01T15:27:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: childsb, mtanino, rootfs

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/volume/iscsi/OWNERS~~ [childsb,rootfs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

tobad357 · 2017-06-03T03:38:24Z

@mtanino I agree this is very important to us as well
Would be great to get into a 1.6.x release

k8s-github-robot · 2017-06-03T06:27:13Z

Automatic merge from submit-queue

@rootfs

Automatic merge from submit-queue (batch tested with PRs 46112, 46764, 46727, 46974, 46968) iscsi storage plugin: bkpPortal should be initialized beforehand **What this PR does / why we need it**: This patch is a follow up patch for the PR #46239. The bkpPortal in DetachDisk() path should be initialized before using it. **Special notes for your reviewer**: /cc @rootfs @childsb **Release note**: ``` NONE ```

…9-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #46239 Cherry pick of #46239 on release-1.6. #46239: Log out from multiple portals with iscsi storage plugin This Cherry pick also includes fix #46968(follow-up fix for #46239) #46968: iscsi storage plugin: bkpPortal should be initialized beforehand

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 22, 2017

k8s-github-robot assigned deads2k and thockin May 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 22, 2017

deads2k removed their assignment May 22, 2017

k8s-ci-robot assigned rootfs May 22, 2017

thockin removed their assignment May 22, 2017

rootfs mentioned this pull request May 23, 2017

iscsi storage plugin only logs out of primary portal #45394

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 23, 2017

tobad357 reviewed May 24, 2017

View reviewed changes

mtanino force-pushed the issue/45394 branch from 141e014 to 340a782 Compare May 26, 2017 22:14

mtanino changed the title ~~[WIP] Log out from multiple target portals when using iscsi storage plugin~~ Log out from multiple target portals when using iscsi storage plugin May 26, 2017

rootfs reviewed May 30, 2017

View reviewed changes

humblec reviewed May 30, 2017

View reviewed changes

mtanino force-pushed the issue/45394 branch from 340a782 to 444b0d0 Compare May 31, 2017 20:07

rootfs reviewed May 31, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2017

mtanino force-pushed the issue/45394 branch from 444b0d0 to aebaee3 Compare May 31, 2017 20:54

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

calebamiles modified the milestone: v1.7 Jun 2, 2017

k8s-github-robot merged commit fcf183d into kubernetes:master Jun 3, 2017

This was referenced Jun 3, 2017

Automated cherry pick of #46239 #46912

Merged

iscsi storage plugin: bkpPortal should be initialized beforehand #46968

Merged

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 5, 2017

Log out from multiple target portals when using iscsi storage plugin #46239

Log out from multiple target portals when using iscsi storage plugin #46239

Conversation

mtanino commented May 22, 2017 • edited by enisoc

k8s-ci-robot commented May 22, 2017

gnufied commented May 22, 2017

rootfs commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtanino May 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtanino commented May 26, 2017 • edited

mtanino commented May 26, 2017 • edited

mtanino commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec May 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented May 30, 2017

humblec commented May 30, 2017 • edited

humblec commented May 30, 2017

mtanino commented May 30, 2017

mtanino commented May 30, 2017

humblec commented May 30, 2017

tobad357 commented May 31, 2017

rootfs commented May 31, 2017

rootfs May 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 31, 2017 • edited

rootfs commented May 31, 2017

mtanino commented Jun 1, 2017

mtanino commented Jun 1, 2017

humblec commented Jun 1, 2017

mtanino commented Jun 1, 2017

childsb commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

tobad357 commented Jun 3, 2017

k8s-github-robot commented Jun 3, 2017

mtanino commented May 22, 2017 •

edited by enisoc

mtanino May 24, 2017 •

edited

mtanino commented May 26, 2017 •

edited

mtanino commented May 26, 2017 •

edited

humblec May 30, 2017 •

edited

humblec commented May 30, 2017 •

edited

rootfs May 31, 2017 •

edited

k8s-ci-robot commented May 31, 2017 •

edited