Skip to content

[cinder-csi-plugin] find device by path from lun address #1197

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
Closed
peterrosell wants to merge 2 commits into from
Closed

[cinder-csi-plugin] find device by path from lun address #1197

peterrosell wants to merge 2 commits into from

Conversation

@peterrosell
Copy link

@peterrosell peterrosell commented Sep 11, 2020

Disks can appear with the pattern /dev/disk/by-path/*lun-<id> on Hyper-V.
Using the last value in the address as an id.

What this PR does / why we need it:
Makes it possible to detect disks on Hyper-V that appear with the pattern /dev/disk/by-path/*lun-<id>.

Which issue this PR fixes(if applicable):
fixes #1192

Special notes for reviewers:
To test this PR you need Openstack installed on top of Hyper-V and install kube and cinder-csi-plugin. Then create a matching storage class, a PVC using that storage class and a Pod that mounts the PVC. See the issue for more details.

Release note:

[cinder-csi-plugin] Add support for detecting disks by-path for lun volumes on Hyper-V.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 11, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 11, 2020

Welcome @peterrosell!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 11, 2020

Hi @peterrosell. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@peterrosell
Copy link
Author

peterrosell commented Sep 11, 2020

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 11, 2020

@peterrosell: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@jichenjc
Copy link
Contributor

jichenjc commented Sep 11, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 11, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@peterrosell
Copy link
Author

peterrosell commented Sep 11, 2020

/test ?

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 11, 2020

@peterrosell: The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

Details

In response to this:

/test ?

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.

@jichenjc
Copy link
Contributor

jichenjc commented Sep 11, 2020

/test all

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 11, 2020

Build succeeded.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 6, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 6, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jichenjc after the PR has been reviewed.
You can assign the PR to them by writing /assign @jichenjc in a comment when ready.

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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@peterrosell
Copy link
Author

peterrosell commented Oct 6, 2020

/retest

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 6, 2020

Build succeeded.

@peterrosell
Copy link
Author

peterrosell commented Oct 6, 2020

I have added a solution for Hyper-V generation 1 VMs that ignores the root disk and the metadata disk when listing disk under by-path. I have assumed that the disks always have the same uuids 00000000000088990000000000000000 and 00000001000088990000000000000000. I think we can go with that and if someone finds another solution for this or have better knowledge of how it works we can patch it then.

@peterrosell peterrosell requested a review from jichenjc October 6, 2020 10:29
@jichenjc
Copy link
Contributor

jichenjc commented Oct 9, 2020

I have added a solution for Hyper-V generation 1 VMs that ignores the root disk and the metadata disk when listing disk under by-path. I have assumed that the disks always have the same uuids 00000000000088990000000000000000 and 00000001000088990000000000000000. I think we can go with that and if someone finds another solution for this or have better knowledge of how it works we can patch it then.

I don't have strong skill on those 0000 xxxxx numbers which looks weird
as this can solve hyper-V gen-1 issue

maybe better to have a hyper-V CI but that's a much bigger story

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020
@peterrosell
Copy link
Author

peterrosell commented Oct 9, 2020

@capsali Do you have any input on this solution? I have a feeling that you know more details about Hyper-V?

@peterrosell
Copy link
Author

peterrosell commented Oct 19, 2020

@ramineni I have tested this against the OpenStack environment that I have access to and it works for both gen1 and gen2 VMs so it should be fine to merge it.

@peterrosell
Copy link
Author

peterrosell commented Nov 10, 2020

It's okay from me if you want to close this PR as our hosting provider has changed their underlying infrastructure from Hyper-V to KVM so I will not put anymore work on it.

@lingxiankong
Copy link
Contributor

lingxiankong commented Nov 10, 2020

It's okay from me if you want to close this PR as our hosting provider has changed their underlying infrastructure from Hyper-V to KVM so I will not put anymore work on it.

Thanks for letting us know @peterrosell, I will close this PR for now as there was not much interest in Hyper-V Kubernetes cluster environment. I'm happy to reopen this if someone shows interest in the future.

/close

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 10, 2020

@lingxiankong: Closed this PR.

Details

In response to this:

It's okay from me if you want to close this PR as our hosting provider has changed their underlying infrastructure from Hyper-V to KVM so I will not put anymore work on it.

Thanks for letting us know @peterrosell, I will close this PR for now as there was not much interest in Hyper-V Kubernetes cluster environment. I'm happy to reopen this if someone shows interest in the future.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ricolin ricolin Awaiting requested review from ricolin

@jichenjc jichenjc Awaiting requested review from jichenjc

Assignees

@jichenjc jichenjc

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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

No milestone

Development

Successfully merging this pull request may close these issues.

[cinder-csi-plugin] Failed to mount volume to pod on HyperV VM

7 participants

@peterrosell @k8s-ci-robot @jichenjc @capsali @ramineni @lingxiankong @p7p8p9p0p