Skip to content
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.

Sign up for GitHub

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

Jump to bottom

iSCSI plugin: Update devicepath with filepath.Glob result #47281

Merged
merged 1 commit into from
Jun 12, 2017
Merged

iSCSI plugin: Update devicepath with filepath.Glob result #47281

merged 1 commit into from
Jun 12, 2017

Conversation

mtanino
Copy link

@mtanino mtanino commented Jun 10, 2017 •
edited by enisoc
Loading

What this PR does / why we need it:

If iscsiTransport is not tcp, iSCSI plugin tries to
find devicepath using filepath.Glob but never updates
devicepath with the filepath.Glob result.

This patch fixes the problem.

Which issue this PR fixes : fixes #47253
Special notes for your reviewer:

Release note:

Fix iSCSI iSER mounting.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 10, 2017
@k8s-ci-robot
Copy link
Contributor

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.

I understand the commands that are listed here.

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 k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 10, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 10, 2017
@mtanino
Copy link
Author

mtanino commented Jun 10, 2017

@rootfs @humblec @thoro

rootfs
rootfs reviewed Jun 10, 2017
View reviewed changes
pkg/volume/iscsi/iscsi_util.go Outdated
for i := 0; i < maxRetries; i++ {
var err error
if deviceTransport == "tcp" {
_, err = osStat(devicePath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate devicePath != nil

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

rootfs
rootfs reviewed Jun 10, 2017
View reviewed changes
pkg/volume/iscsi/iscsi_util.go Outdated
// There might be a case that fpath contains multiple device paths if
// multiple PCI devices connect to same iscsi target. We handle this
// case at subsequent logic. Pick up only first path here.
*devicePath = fpath[0]
Copy link
Contributor

@rootfs rootfs Jun 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtanino @thoro can you get a sample of /dev/disk/by-path/ and add to the comment? Would it be possible to sort the paths and pick the first one? I am concerned if the paths appear at non-deterministic order, there is a chance the devicePath is randomly chosen yet points to the same target, which could be a problem later for logout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thoro
Could you paste your result here? I don't have iSER device, so I can't.

Copy link
Author

@mtanino mtanino Jun 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this kind of results might be obtained.

  • /dev/disk/by-path/pci-DEVICE1-ip-127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-0
  • /dev/disk/by-path/pci-DEVICE2-ip-127.0.0.1:3260-iqn.2014-12.com.example:test.tgt00-lun-0

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path would look like that, the

/dev/disk/by-path/pci-0000:06:00.0-ip-10.0.13.3:3260-iscsi-iqn.2017-01.at.cf-it.at-storage-01-lun-0

which corresponds to the Infiniband Adapter

06:00.0 InfiniBand: QLogic Corp. IBA7322 QDR InfiniBand HCA (rev 01)

@rootfs
Copy link
Contributor

rootfs commented Jun 10, 2017

@k8s-bot ok to test
/approve

@k8s-ci-robot 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 Jun 10, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2017
iSCSI plugin: Update devicepath with filepath.Glob result
b1f06aa 
If iscsiTransport is not tcp, iSCSI plugin tries to
find devicepath using filepath.Glob but never updates
devicepath with the filepath.Glob result.

This patch fixes the problem.

Fixes kubernetes#47253
@mtanino
Copy link
Author

mtanino commented Jun 10, 2017

  • devicePath != nil check added to waitForPathToExist().

@thoro
Copy link

thoro commented Jun 10, 2017

@rootfs @mtanino Confirmed working on my local install - no more errors!

@rootfs
Copy link
Contributor

rootfs commented Jun 10, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 47253

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

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

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

@mtanino
Copy link
Author

mtanino commented Jun 10, 2017

@thoro
Thanks!

@rootfs
Is this proper to backport to v1.6?

@rootfs
Copy link
Contributor

rootfs commented Jun 11, 2017

@thoro do you need it in 1.6?

@thoro
Copy link

thoro commented Jun 11, 2017

@rootfs Nope, is missing the CHAP Auth anyways ;) - running my own from master

@rootfs
Copy link
Contributor

rootfs commented Jun 11, 2017

@thoro Happy bug reporting!

@mtanino
Copy link
Author

mtanino commented Jun 11, 2017

@rootfs @thoro
Since this PR is bug fix, it's worthwhile to backport even if the reporter does not use it, IMO.

@mtanino
Copy link
Author

mtanino commented Jun 12, 2017

@childsb @eparis @rootfs
Could you add v1.7 milestone for this PR?
Thanks.

@eparis eparis added this to the v1.7 milestone Jun 12, 2017
@mtanino
Copy link
Author

mtanino commented Jun 12, 2017

@eparis
Thanks!

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8fc4e17 into kubernetes:master Jun 12, 2017
@mtanino mtanino mentioned this pull request Jun 12, 2017
Merged
@enisoc 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 12, 2017
k8s-github-robot pushed a commit that referenced this pull request Jun 13, 2017
Merge pull request #47365 from mtanino/automated-cherry-pick-of-#4728…
97b8fab 
…1-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #47281

Cherry pick of #47281 on release-1.6.

#47281: iSCSI plugin: Update devicepath with filepath.Glob result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rootfs rootfs rootfs left review comments

@thoro thoro thoro left review comments

Assignees

@justinsb justinsb

@rootfs rootfs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

iSCSI iSER mounting does use glob path for mount utilities
8 participants
@mtanino @k8s-ci-robot @rootfs @thoro @k8s-github-robot @justinsb @enisoc @eparis