Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-8239 Add VirtIO SCSI support for KVM hosts - #1955
Merged
Conversation
kiwiflyer
commented
Feb 20, 2017
Contributor
@remibergsma@ustcweizhou This might be of interest to you guys as well. |
Contributor
Nice work @kiwiflyer@nathanejohnson, thanks! Will give it a go soon. |
wido
commented
Feb 21, 2017
Contributor
Very nice indeed! I will take a look asap. |
dmabry
commented
Feb 21, 2017
Contributor
We are deploying this to our QA environment right now and hope to have it tested in a few days. Great work @kiwiflyer and @nathanejohnson. |
borisstoyanov
commented
Feb 22, 2017
Contributor
Thanks for this PR @nathanejohnson, I will build it and run smoketests agains it. |
blueorangutan
commented
Feb 22, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Feb 22, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-522 |
borisstoyanov
commented
Feb 22, 2017
Contributor
@blueorangutan test |
blueorangutan
commented
Feb 22, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Feb 22, 2017
Trillian test result (tid-878)
|
borisstoyanov
commented
Feb 23, 2017
Contributor
@nathanejohnson Thanks, for this good enhancement. Can we have a marvin tests that covers the new functionality? |
wido
reviewed
Feb 23, 2017
| if (getGuestDiskModel(vmTO.getPlatformEmulator()) == DiskDef.DiskBus.SCSI) { | ||
| final SCSIDef sd = new SCSIDef((short)0, 0, 0, 9, 0); | ||
| devices.addDevice(sd); | ||
| s_logger.debug("Adding scsi definition:\n" + sd.toString()); |
Contributor
There was a problem hiding this comment.
Might want to include the name of the VM here as well? And SCSI is written in upper-case.
| if (diskBusType == null) { | ||
| diskBusType = getGuestDiskModel(vmSpec.getPlatformEmulator()); | ||
| s_logger.debug("disk bus type derived from getPlatformEmulator: " + vmSpec.getPlatformEmulator() + ", diskbustype is: "+diskBusType.toString()); |
Contributor
There was a problem hiding this comment.
Same here, please add the name of the VM
| } | ||
| } | ||
| s_logger.debug("Adding disk: " + disk.toString()); |
Contributor
There was a problem hiding this comment.
Same here, please include the name of the VM
Also, do we really need all these logging lines?
nathanejohnson
commented
Feb 24, 2017
MemberAuthor
As an update, I'm working on a Marvin test, and I'm also planning to address wido's concerns. Today had many distractions unfortunately. |
borisstoyanov
commented
Feb 24, 2017
Contributor
@nathanejohnson thank you, let me know when you're done with the changes so I could kick in smoke tests. |
nathanejohnsonforce-pushed
the
virtio-scsi
branch
3 times, most recently
from
March 1, 2017 15:07
e6bef28 to
29f83e6CompareMemberAuthor
@wido I have removed the extraneous log messages. @borisstoyanov I have added a smoke test for this that tests both virsh output from the host and lspci / lsblk output from the guest. Here is the output from me running this in a bubble: |
nathanejohnsonforce-pushed
the
virtio-scsi
branch
2 times, most recently
from
March 1, 2017 17:20
91a7f13 to
708a1cfCompareborisstoyanov
commented
Mar 1, 2017
Contributor
Thanks @nathanejohnson, lets run that with BO |
blueorangutan
commented
Mar 1, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Mar 1, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-552 |
borisstoyanov
commented
Mar 1, 2017
Contributor
@blueorangutan test |
blueorangutan
commented
Mar 1, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
nathanejohnson
commented
Mar 1, 2017
MemberAuthor
@borisstoyanov Quick questions, what tags need to be present to get picked up by BO? I'm worried my smoke test might get skipped. |
blueorangutan
commented
Mar 2, 2017
Trillian test result (tid-921)
|
nathanejohnson
commented
Mar 2, 2017
MemberAuthor
@borisstoyanov so the good news is it didn't skip the test. the bad news is it didn't seem to have proper credentials for ssh'ing into your hosts. It did pass the test where it ssh'ed into the guest, however. So this at least demonstrates a guest is happy. Do you have any tips on ways of making the smoke test play nice with BO wrt ssh'ing into the kvm hosts? |
borisstoyanov
commented
Mar 2, 2017
Contributor
Thanks @nathanejohnson I will have a look |
remibergsma
commented
Mar 2, 2017
Contributor
Tested it manually, works great. LGTM. |
borisstoyanov
commented
Mar 9, 2017
Contributor
@nathanejohnson sure, sorry I've missed your comment about the test fix earlier. |
blueorangutan
commented
Mar 9, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Mar 9, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-577 |
borisstoyanov
commented
Mar 9, 2017
Contributor
@blueorangutan test |
blueorangutan
commented
Mar 9, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Mar 9, 2017
Trillian test result (tid-938)
|
borisstoyanov
approved these changes
Mar 10, 2017
borisstoyanov
left a comment
Contributor
There was a problem hiding this comment.
Thanks @nathanejohnson, tests results are looking good now, there are some failures but they are known issues. LGTM
kiwiflyer
commented
Mar 10, 2017
Contributor
tag:mergeready |
wido
commented
Mar 10, 2017
Contributor
I would like to give a LGTM, but it's only the logging lines which I don't like yet. They seem like personal debug lines, but a admin will never know what they actually mean. In addition, I would like to see discard/unmap enabled by default for Virtio-SCSI disks: https://blog.widodh.nl/2015/12/using-trimdiscard-with-ceph-rbd-and-libvirt/ This will also allow for QCOW2 and RBD trimming. |
nathanejohnson
commented
Mar 11, 2017
MemberAuthor
@wido I have removed all of the debug logging statements that didn't exist before I touched the code, I have also added the discard option to the disks. |
wido
commented
Mar 11, 2017
Contributor
Do we want to pass the unmap not as a string? Shouldn't that be a enum? |
nathanejohnson
commented
Mar 11, 2017
MemberAuthor
@wido Actually I was just about to push a commit that changed the string to enum, but builds are failing locally because of this test: Daylight savings time strikes again |
wido
commented
Mar 11, 2017
Contributor
@nathanejohnson: Good! I would like to see that being enums. |
nathanejohnsonforce-pushed
the
virtio-scsi
branch
2 times, most recently
from
March 12, 2017 01:41
6446ed0 to
ed4fa15Comparenathanejohnson
commented
Mar 12, 2017
MemberAuthor
I just re-depoyed and re-ran the test: @wido , I believe all of your concerns should be addressed. trim is enabled, it's using an enum and not a string, and spurious log messages have been eliminated. I have squashed all of the commits into one. I'm at a point where I'm happy with the code. |
Contributor
LGTM from the code Awesome work! |
This adds support for virtio-scsi on KVM hosts, either for guests that are associated with a new os_type of 'Other PV Virtio-SCSI (64-bit)', or when a VM or template is regstered with a detail parameter rootDiskController=scsi. Update cloudstack add template dialog to allow for selecting rootDiskController with KVM Update cloudstack kvm virtio-scsi to enable discard=unmap
nathanejohnsonforce-pushed
the
virtio-scsi
branch
from
March 12, 2017 15:54
ed4fa15 to
5c47649Comparekiwiflyer
commented
Mar 13, 2017
Contributor
@karuturi 3 x LGTM, testing successful. Ready for Merge. |
wido
commented
Mar 27, 2017
Contributor
Yes, we are indeed ready for a merge. Shall we do that? |
karuturi
commented
Apr 20, 2017
Member
merging.. |
DaanHoogland
commented
Apr 25, 2017
Contributor
@nathanejohnson@wido@remibergsma@karuturi@kiwiflyer @dmambry |
nathanejohnson
commented
Apr 25, 2017
MemberAuthor
@DaanHoogland@wido@remibergsma@karuturi@kiwiflyer I think I see the issue with the test, I should hopefully have a new PR opened later today. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an initial implementation that we have working in the lab.
win_virtio_scsi.xml.zip
@wido@syed Would love to get some feedback on this approach.