@remibergsma @ustcweizhou This might be of interest to you guys as well.

Nice work @kiwiflyer @nathanejohnson, thanks! Will give it a go soon.

Very nice indeed! I will take a look asap.

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.

Thanks for this PR @nathanejohnson, I will build it and run smoketests agains it.
@blueorangutan package

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-522

@blueorangutan test

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-878)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32944 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1955-t878-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 47 look ok, 2 have error(s)

@nathanejohnson Thanks, for this good enhancement. Can we have a marvin tests that covers the new functionality?

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.

@nathanejohnson thank you, let me know when you're done with the changes so I could kick in smoke tests.

@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:

Test that libvirt properly created domain with scsi controller ... === TestName: test_01_verify_libvirt | Status : SUCCESS ===
ok
Verify that libvirt settings are as expected after a VM stop / start ... === TestName: test_02_verify_libvirt_after_restart | Status : SUCCESS ===
ok
Verify that libvirt settings are expected after a disk add ... === TestName: test_03_verify_libvirt_attach_disk | Status : SUCCESS ===
ok
Verify that guest sees scsi controller and disks ... === TestName: test_04_verify_guest_lspci | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 4 tests in 658.770s

OK

Thanks @nathanejohnson, lets run that with BO
@blueorangutan package

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-552

@blueorangutan test

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@borisstoyanov Quick questions, what tags need to be present to get picked up by BO? I'm worried my smoke test might get skipped.

Trillian test result (tid-921)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36820 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1955-t921-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 47 look ok, 3 have error(s)

@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?

Thanks @nathanejohnson I will have a look

Tested it manually, works great. LGTM.

@nathanejohnson sure, sorry I've missed your comment about the test fix earlier.
@blueorangutan package

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-577

@blueorangutan test

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-938)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34569 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1955-t938-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 47 look ok, 3 have error(s)

tag:mergeready

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.

@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.

Do we want to pass the unmap not as a string? Shouldn't that be a enum?

@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:

assertTrue(QuotaAlertManagerImpl.getDifferenceDays(now, new DateTime(now).plusDays(1).toDate()) == 1L);

Daylight savings time strikes again

@nathanejohnson: Good! I would like to see that being enums.

I just re-depoyed and re-ran the test:

Test that libvirt properly created domain with scsi controller ... === TestName: test_01_verify_libvirt | Status : SUCCESS ===
ok
Verify that libvirt settings are as expected after a VM stop / start ... === TestName: test_02_verify_libvirt_after_restart | Status : SUCCESS ===
ok
Verify that libvirt settings are expected after a disk add ... === TestName: test_03_verify_libvirt_attach_disk | Status : SUCCESS ===
ok
Verify that guest sees scsi controller and disks ... === TestName: test_04_verify_guest_lspci | Status : SUCCESS ===
ok
Update os type to Ubuntu, change vm details rootdiskController ... === TestName: test_05_change_vm_ostype_restart | Status : SUCCESS ===
ok
Verify that guest sees scsi controller and disks after switching ostype and rdc ... === TestName: test_06_verify_guest_lspci_again | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 6 tests in 785.914s

OK

@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.

LGTM from the code

Awesome work!

@karuturi 3 x LGTM, testing successful. Ready for Merge.

Yes, we are indeed ready for a merge. Shall we do that?

merging..

@nathanejohnson @wido @remibergsma @karuturi @kiwiflyer @dmambry
in #2044 tests are failing on vmware while this is only for kvm. Can we add a check on the tests for this or is it supposed to work for all hypervisors?

@DaanHoogland @wido @remibergsma @karuturi @kiwiflyer I think I see the issue with the test, I should hopefully have a new PR opened later today.