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

[ferki]

As a follow-up for our previous IRC chat, thanks again for your contributions!

As discussed, this PR doesn't exactly follow the general contributing workflow as there was no issue to discuss/design changes first. To avoid being overly redundant, I'm not going to mention this on the rest of the PR reviews, but I'll provide a summary of our chat.

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

• Ideally, there could be a related issue open about the exact use case, which this PR could close then.
• If this PR mixes a new feature with a bugfix, it would be preferable to have two commits for the two logical changes.
• As the CI failure indicates, the code requires to be reformatted with latest perltidy.
• As discussed, t/issue/948.t: Fixing whitespace detection in test #1472 could probably be folded into this PR as a related test change (perhaps it's a prerequisite even?).

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

[ferki]

@alip: I believe I could understand the changes much better after a closer look at the libvirt XML definitions.

If my understanding is complete and correct now, I'd say let's do this:

• if possible, let's split the combined commit into:
  • one that adds support to driver_cache and `driver_discard
  • another one that deals with the rest (SCSI bits)
• fix Test::Perl::Critic::Progressive complaints (I've left some line comments for that)
• see if there's a better default for SCSI controller model

A related test is already fixed as part of the PR, and adding a comprehensive test coverage would be out of scope. So I believe we could merge this as soon as we get to a passing state after the above list.

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

[einhverfr]

Totally ok with the merge.