-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new iSCSI refcounter #80091
Add new iSCSI refcounter #80091
Conversation
/assign @bswartz @j-griffith |
I'll check if it's possible to add a test for this. |
1a7c014
to
7d824df
Compare
/assign @jsafrane |
@bswartz, @j-griffith, @jsafrane: For now I'm mostly looking for feedback on the refcounter's strategy. If you think the way it's counting the volumes is the way to go, I'll go ahead and forward and finish this up. |
7d824df
to
73e2135
Compare
volumeMount := m.Name() | ||
prefix := portal + "-" + iqn | ||
if strings.HasPrefix(volumeMount, prefix) { | ||
counter++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this relying on lun to be the ref counter? That won't work in a RWM case for a block device I don't think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the question correctly, but the prefix doesn't have the lun, so the counter doesn't rely on it (only on portal + iqn).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was worried there was a uniqueness check based on lun number somewhere else in the changes. Thanks for clarifying.
73e2135
to
8cdd4e0
Compare
CC @humblec |
8cdd4e0
to
dacba78
Compare
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not used anywhere.
|
||
// If device is no longer used, see if need to logout the target | ||
if isSessionBusy(c.iscsiDisk.plugin.host, portals[0], iqn) { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to prevent races, the lock here is necessary just like in DetachDisk
.
dacba78
to
c23c456
Compare
c23c456
to
0007c2a
Compare
3063794
to
44d7510
Compare
@bertinatto: The following tests failed, say
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. |
/test pull-kubernetes-kubemark-e2e-gce-big |
/approve |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, j-griffith, jsafrane The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This looks good to me, I was holding off on the l g t m to give @bswartz a chance to chime in. |
I don't object to this -- it looks like an improvement. It's not clear to me that the affects both raw block and filesystem volumes, but if so, then that's good. I hope we consider looking at using sysfs for refcounting in the future. That would be a larger change, but probably even more reliable than this improvement. |
/lgtm |
Apologies, late to the party :(. Thanks @jsafrane and @j-griffith for reviewing it! |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR adds a new refcounter mechanism to iSCSI storage plugin so that sessions are not logged out too early.
The previous refcounter looked at the mounted volumes to determine whether the session could be logged out or not. This works well with filesystem volumes, however, we can't use this mechanism for block volumes because they are not necessarily mounted in the host.
This new refcounter looks at the volume directories created/deleted by the kubelet instead.
Which issue(s) this PR fixes:
Fixes #74313
Does this PR introduce a user-facing change?: