Fix for Premature iSCSI logout #39202. #41196

cristianpop · 2017-02-09T15:29:40Z

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:

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.

k8s-ci-robot · 2017-02-09T15:29:41Z

Hi @cristianpop. 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.

k8s-ci-robot · 2017-02-09T15:29:43Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

k8s-reviewable · 2017-02-09T15:29:47Z

This change is

rootfs · 2017-02-09T15:51:40Z

/approve

rootfs · 2017-02-09T15:52:05Z

@k8s-bot ok to test

rootfs · 2017-02-09T15:52:42Z

@cristianpop have you signed CLA yet?

rootfs · 2017-02-09T15:53:32Z

@cdragga

rootfs · 2017-02-09T15:57:03Z

pkg/volume/iscsi/iscsi_util.go

-	return path.Join(host.GetPluginDir(iscsiPluginName), portal+"-"+iqn+"-lun-"+lun)
+// 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 path.Join(host.GetPluginDir(iscsiPluginName), iface, portal+"-"+iqn+"-lun-"+lun)


this could be a problem for kubelet upgrade

I thought it may be problematic but I needed to somehow obtain the iSCSI iface in DettachDisk to logout only on the given iface in case there are multiple connections to the same target using different ifaces. The use of multiple connections on different ifaces might not be common, but that's the setup I have. The servers are booted using iSCSI and if the persistent volumes end up on the same target as the boot image, the disk dettaches would end up closing the iSCSI connection that imported the boot image as well.

I understand you need to make the code more robust, but it should work out of the box when updated from older Kubernetes release without killing pods. So you should "adopt" iSCSI LUNs mounted to the old directory too and unmount+"logoff" them when the pods are deleted.

Looking at the code, it seems it could work, however I'd like someone to confirm this.

I think it would work but it would not log out of any interface, supposing the directory name is not a legit iSCSI interface.

I could use the plugin directory to determine whether the path is an old one or not and return the "default" interface or an empty string in extractIface and act accordingly, either log out on the default iface or do not log out at all to avoid logging out of sessions that may be used by other pods.

rootfs · 2017-02-09T16:02:11Z

pkg/volume/iscsi/iscsi_util.go

@@ -185,14 +192,20 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error {
 		refCount, err := getDevicePrefixRefCount(c.mounter, prefix)


also see this #41041 (comment)

I thought about changing the HasPrefix call to Contains but I wanted to account for the iface as well and log out only if there are no other pods that use that specific iface. I know it's not foul proof especially when there are multiple ifaces using the same transport to the same target as the devices that get mounted are not necessarily the ones that were imported on the given iface, but I could not find a better solution until now.

This seems like a solid solution to me, at least for the moment. In the long-term, I'm concerned about cases where sessions that exist independently of Kubernetes get logged off because a pod used a volume connected to the target using the same interface (I think this is the same concern @cristianpop cited just now, but I'm not totally sure). Solving that might be doable with something like resolving all of the symlinks in /dev/disk/by-path to their devices and then checking whether any of the devices that belong to the session are still mounted; I don't know if this would behave well with multipathing, though.

At any rate, this is a better fix than just changing HasPrefix to Contains.

cristianpop · 2017-02-09T16:40:35Z

@cristianpop have you signed CLA yet?

I haven't signed the CLA yet, but I'm working on it (the company I'm working for has to sign it).

eparis · 2017-02-10T23:23:34Z

@k8s-bot gci gce e2e test this

@cristianpop do you know when you will be able to get the CLA signed? I know that it can be difficult with many companies.

Are you willing to say that the contribution was created in whole by you and that you have the right to submit it under the Apache License Version 2.0? Do you also understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information you submited with it) is maintained indefinitely and may be redistributed consistent with this project or the license involved?

cristianpop · 2017-02-13T11:33:45Z

@eparis I'm sorry for the delay. The management is handling it and I've been told it will be signed today or tomorrow at the latest.

Yes, the contribution is mine, I have the right to submit it and I understand the contribution is public.

rootfs · 2017-02-14T00:15:24Z

@cristianpop can you rebase it?

@saad-ali PTAL

…ect the changes brought by the kubernetes#39202 fix.

cristianpop · 2017-02-14T12:31:44Z

@rootfs I've rebased it.

cristianpop · 2017-02-14T14:31:29Z

The CLA has been signed. Sorry for the delay.

rootfs · 2017-02-14T14:32:50Z

@k8s-bot gci gce e2e test this

rootfs · 2017-02-14T14:37:40Z

LGTM

jsafrane · 2017-02-14T14:58:44Z

pkg/volume/iscsi/iscsi_util.go

 	return device, prefix, nil
 }

+func extractIface(mntPath string) (string, error) {
+	ind := strings.LastIndex(mntPath, "/")


use path.Dir()?

I wanted to keep it consistent with the existing code but I can change it.

If I use path.Base and path.Dir to extract the interface, I would lose the malformed mount path checks. I could keep the checks as they are and use path.Base and path.Dir but it would not make any sense to not use the indexes, or I could rewrite them to use path.Match or check for empty strings and ".", or scrap the checks. I would prefer to keep the checks, although they may end up not failing ever. What do you prefer?

let's get the check work correctly as the first priority.

jsafrane · 2017-02-14T14:59:17Z

pkg/volume/iscsi/iscsi_util.go

+		return "", fmt.Errorf("iscsi detach disk: malformatted mnt path: %s", mntPath)
+	}
+
+	iface := baseMntPath[(ind + 1):]


path.Dir()?

rootfs · 2017-02-14T15:18:34Z

@cristianpop @jsafrane

here is my 2 cents dealing with backward compatiblity.

In makePDNameInternal, make the path look like iface-iface_name/portal/iqn/lun-0.

In extractIface, search pattern iface-, if not found, return not exist. And caller skips this error and not use -I during log out. Otherwise, use the iface for logout.

cristianpop · 2017-02-14T19:05:59Z

@rootfs

Sounds good to me. Better than searching for the volume plugin directory in the path.

…t upgrade. The detachDisk behavior is now preserved for pods that were created before the kubelet upgrade.

k8s-github-robot · 2017-02-14T20:51:20Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: CristianPop, rootfs

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

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

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

rootfs · 2017-02-14T21:20:38Z

@k8s-bot test this

rootfs · 2017-02-14T21:46:50Z

/lgtm

jsafrane · 2017-02-15T09:36:15Z

@k8s-bot kops aws e2e test this

k8s-github-robot · 2017-02-15T12:11:55Z

Automatic merge from submit-queue

adkerr · 2017-02-16T15:20:47Z

Would it be possible to get this cherry-picked to 1.5?

saad-ali · 2017-02-25T01:12:50Z

CC @mwielgus 1.5 release branch manager for cherry pick approval

saad-ali · 2017-02-25T01:15:54Z

@cristianpop: Once this is approved for cherry pick, execute ./hack/cherry_pick_pull.sh upstream/release-1.5 41196 to cherry pick this to the 1.5 branch.

innergy · 2017-03-14T14:38:45Z

@saad-ali Anything that needs to be done to start the cherry pick approval process?

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 9, 2017

k8s-github-robot assigned saad-ali Feb 9, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 9, 2017

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

rootfs reviewed Feb 9, 2017

View reviewed changes

eparis added the cla: human-approved label Feb 13, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

Cristian Pop added 2 commits February 14, 2017 11:34

Fix for Premature iSCSI logout kubernetes#39202.

b0d285c

Updated TestExtractDeviceAndPrefix and added TestExtractIface to refl…

2aaeefe

…ect the changes brought by the kubernetes#39202 fix.

cristianpop force-pushed the master branch from c5bee23 to 2aaeefe Compare February 14, 2017 09:36

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 14, 2017

jsafrane reviewed Feb 14, 2017

View reviewed changes

Implemented suggestions for kubernetes#39202 fix to facilitate kubele…

b23b475

…t upgrade. The detachDisk behavior is now preserved for pods that were created before the kubelet upgrade.

childsb assigned rootfs and unassigned saad-ali Feb 14, 2017

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

k8s-github-robot merged commit a50ea2f into kubernetes:master Feb 15, 2017

saad-ali added this to the v1.5 milestone Feb 25, 2017

saad-ali added the cherrypick-candidate label Feb 25, 2017

xiangpengzhao mentioned this pull request Jul 27, 2017

mungegithub: extract hardcoded list of CLA labels into configurable list kubernetes/test-infra#3724

Closed

wojtek-t removed the cherrypick-candidate label Nov 28, 2017

bertinatto mentioned this pull request Jan 22, 2019

Reused iSCSI sessions not being deleted #73181

Closed

		@@ -185,14 +192,20 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error {
		refCount, err := getDevicePrefixRefCount(c.mounter, prefix)

Fix for Premature iSCSI logout #39202. #41196

Fix for Premature iSCSI logout #39202. #41196

Conversation

cristianpop commented Feb 9, 2017

k8s-ci-robot commented Feb 9, 2017

k8s-ci-robot commented Feb 9, 2017

k8s-reviewable commented Feb 9, 2017

rootfs commented Feb 9, 2017

rootfs commented Feb 9, 2017

rootfs commented Feb 9, 2017

rootfs commented Feb 9, 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

cristianpop commented Feb 9, 2017

eparis commented Feb 10, 2017

cristianpop commented Feb 13, 2017

rootfs commented Feb 14, 2017

cristianpop commented Feb 14, 2017

cristianpop commented Feb 14, 2017

rootfs commented Feb 14, 2017

rootfs commented Feb 14, 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

rootfs commented Feb 14, 2017

cristianpop commented Feb 14, 2017

k8s-github-robot commented Feb 14, 2017

rootfs commented Feb 14, 2017

rootfs commented Feb 14, 2017

jsafrane commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

adkerr commented Feb 16, 2017

saad-ali commented Feb 25, 2017 • edited Loading

saad-ali commented Feb 25, 2017

innergy commented Mar 14, 2017

saad-ali commented Feb 25, 2017 •

edited

Loading