LibVirt: Initial support for virtio-scsi virtual drives (read: ssd trim)

[alip]

1. Honour driver_cache and driver_discard keys in storage disk config.
2. Add a SCSI controller if any of the storage disks have SCSI as the target bus.

The SCSI model defaults to virtio-scsi and may be overriden by the scsi_model parameter.

The second change is also a bug fix since without a SCSI controller configuration a VM with a virtual disk attached to a SCSI bus won't boot.

This PR is an attempt to fix #1473 by .

Checklist

• [ ] based on top of latest source code
• [ ] changelog entry included
• [ ] tests pass on Travis CI
• [ ] git history is clean
• [ ] git commit messages are well-written

[alip]

I think we agree on the overall goal here, so let's see how to get it on board.

+1

* [ ]  Ideally, there could be a related issue open about the exact use case, which this PR could close then.

#1473.

* [ ]  If this PR mixes a new feature with a bugfix, it would be preferable to have two commits for the two logical changes.

Done.

* [ ]  As the CI failure indicates, the code requires to be reformatted with latest perltidy.

I reformatted the code with perltidy-20210111 and forced pushed the branch. The tests pass locally for me however CI tests still fail with Failed test 'Test::Perl::Critic::Progressive'. I don't know how to debug this, @ferki, can you please help?

* [ ]  As discussed, #1472 could probably be folded into this PR as a related test change (perhaps it's a prerequisite even?).

Done.

[ferki]

* [ ]  If this PR mixes a new feature with a bugfix, it would be preferable to have two commits for the two logical changes.

Done.

Erm, I think what confuses me is that I see only one commit, but the commit message talks about two changes, and there are no tests to establish further context. I find it hard to guess which part of the diff is intended to solve which part of story.

Do I understand it correctly, that the commit addresses two separate things (a new feature which got implemented, and a bug which got fixed)?

---

The tests pass locally for me however CI tests still fail with Failed test 'Test::Perl::Critic::Progressive'. I don't know how to debug this, @ferki, can you please help?

Context: Test::Perl::Critic::Progressive ensures that the amount of perlcritic violations in the codebase are not growing. It works by saving a reference state on first run, and then comparing against that state on subsequent runs (see description).

It's failing in CI, because there are 4 new violations compared to master. I would do something like this to reproduce locally:

• delete xt/author/.perlcritic-history if it's present (so the next test run will be first run)
• run AUTHOR_TESTING=1 prove -l xt/author/critic-progressive.t on master (to generate master's list of existing violations)
• run the same on the feature branch (so it can now be compared against the saved state)

---

Nitpick, optional to fix: I would find the commit messages clear enough even without their prefixes (since the diff itself already communicates which file or subsystem is being modified). It's fine for now, but if we decide to split the commit, this can be easily addressed.

[ferki]

Do I understand it correctly, that the commit addresses two separate things (a new feature which got implemented, and a bug which got fixed)?

I think I started to get it:

• there's a part about supporting driver_type and driver_cache in the XML template => that's a new feature
• there's another part in the XML template to add a SCSI controller if bus is set to scsi => that's also a new feature
• but this last change causes virsh define to fail when adding the drive, because there's already a bus from the SCSI controller, so there must be a condition => that's a ~workaround according to the code comment~ part of the previous item

@alip: could you confirm if I understand it correctly now?

[alip]

Updated taking into account the requested changes.

[ferki]

Updated taking into account the requested changes.

Thank you for following up, @alip! This looks good, but I need some time to get back into context before a full review. Stay tuned!

[ferki]

@alip: thanks for the patience here! I think I'll rebase this on top of current default branch here to trigger a fresh round of CI tests. ~~If all's well, I can do that on my own.~~

edit: oh, I thought I could rely on GitHub's feature to push to a PR's original branch as maintainer, but that didn't work here. @alip, please rebase on top of current default branch.

---

@einhverfr: it seems one of the commits on this PR was originally authored by you (and using your gmail address). Given my impression of the context, and the fact that the same commit content has already been published under the same license in the adjust/Rex fork, I think that's fine.

Still, I wished to make you aware of the case, and I'd be grateful for an explicit consent from your side to know it's OK with you to have that contributed by @alip and be merged upstream.