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

cinder: Add support for the KVM virtio-scsi driver #41498

Merged
merged 1 commit into from
Apr 21, 2017
Merged

cinder: Add support for the KVM virtio-scsi driver #41498

merged 1 commit into from
Apr 21, 2017

Conversation

mikebryant
Copy link
Contributor

@mikebryant mikebryant commented Feb 15, 2017 •
edited
Loading

What this PR does / why we need it:

The VirtIO SCSI driver for KVM changes the way disks appear in /dev/disk/by-id.
This adds support for the new format.
Without this, volume attaching on an openstack cluster using this kvm driver doesn't work

Special notes for your reviewer:
Does this need e2e tests? I couldn't find anywhere to add another openstack configuration used in the e2e tests.

Wiki page about this: https://wiki.openstack.org/wiki/Virtio-scsi-for-bdm

Release note:

cinder: Add support for the KVM virtio-scsi driver

@mikebryant
cinder: Add support for virtio-scsi
e2e924e 
The VirtIO SCSI driver for KVM changes the way disks appear in /dev/disk/by-id.
This adds support for the new format.
@k8s-ci-robot
Copy link
Contributor

Hi @mikebryant. 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 k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Feb 15, 2017
@mikebryant mikebryant mentioned this pull request Feb 21, 2017
Closed
@idvoretskyi idvoretskyi added the area/provider/openstack Issues or PRs related to openstack provider label Feb 21, 2017
@idvoretskyi
Copy link
Member

cc @k8s-sig-openstack-pr-reviews

@mikebryant
Copy link
Contributor Author

Is there anything I need to do here?

@xsgordon
Copy link

xsgordon commented Mar 8, 2017

@k8s-bot ok to test

hogepodge
hogepodge approved these changes Mar 22, 2017
View reviewed changes
Copy link

@hogepodge hogepodge left a comment

Choose a reason for hiding this comment

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

lgtm

Currently, afaict, there are no dependent e2e tests that would need to be updated. This patch should be considered a bug fix, as OpenStack can provide Cinder volumes as scsi devices.

@mikebryant
Copy link
Contributor Author

@hogepodge Should your comment have applied the lgtm label?

@hogepodge
Copy link

@mikebryant I don't have approver status, afaik.

@stuart-warren
Copy link
Contributor

ping @anguslees

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 12, 2017
@mikebryant
Copy link
Contributor Author

/assign @anguslees

@anguslees
Copy link
Member

Sorry for the delay, I am no longer working on the openstack cloud provider and no longer have access to openstack clusters for testing :(

This lgtm to me though:
/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2017
@mikebryant
Copy link
Contributor Author

@anguslees Thanks for taking the time :)
Sorry for bugging you, you're currently listed as the only owner of the openstack cloudprovider: https://github.com/kubernetes/kubernetes/blob/2c6fbc95c43b9f8de41903361a9a286dba417f23/pkg/cloudprovider/providers/openstack/OWNERS

Now... who do I need to ping to get an lgtm?

@anguslees
Copy link
Member

@mikebryant: Yes I know, this is why I haven't removed my name from there yet - I figured one disinterested and occasional reviewer is still better than no reviewers :(

Huh, I thought /approve was sufficient, let's try:
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, mikebryant

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

@anguslees
Copy link
Member

@k8s-bot cvm gce e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit cdc0cbd into kubernetes:master Apr 21, 2017
@mikebryant
Copy link
Contributor Author

@anguslees Thanks muchly! :)

@mikebryant mikebryant deleted the cinder-virtio-scsi branch April 21, 2017 09:04
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#41498 from mikebryant/cinder-virtio-scsi
ef1ba0a 
Automatic merge from submit-queue

cinder: Add support for the KVM virtio-scsi driver

**What this PR does / why we need it**:

The VirtIO SCSI driver for KVM changes the way disks appear in /dev/disk/by-id.
This adds support for the new format.
Without this, volume attaching on an openstack cluster using this kvm driver doesn't work

**Special notes for your reviewer**:
Does this need e2e tests? I couldn't find anywhere to add another openstack configuration used in the e2e tests.

Wiki page about this: https://wiki.openstack.org/wiki/Virtio-scsi-for-bdm

**Release note**:

```release-note
cinder: Add support for the KVM virtio-scsi driver
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradbeam bradbeam bradbeam approved these changes

@hogepodge hogepodge hogepodge approved these changes

Assignees

@anguslees anguslees

@vmarmol vmarmol

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/openstack Issues or PRs related to openstack provider 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.