Libvirt allows specifying out-of-spec disk dev attributes it is then unable to handle

Jon,

Libvirt issue - can someone from the Server team take a look?

Thanks.

                     Michael

On 02/16/2017 09:09 AM, Launchpad Bug Tracker wrote:
> bugproxy (bugproxy) has assigned this bug to you for Ubuntu:
>
> While trying to add multiple disks to ubuntu guest on an ubuntu KVM, the following error is reported.
> Error: internal error: process exited while connecting to monitor: qemu-system-ppc64: -drive file=/dev/sdd,format=raw,if=none,id=drive-scsi0-0-0-0,cache=none,aio=native: Duplicate ID 'drive-scsi0-0-0-0' for drive
>
> It seems libvirt is using the same id for more than one disk. My
> impression is that something is wrong with the libvirt being used in
> Ubuntu 16.04.x.
>
> The target device parsing logic which generates the device alias has
> gone wrong as the XML used has the target dev names given in unusual
> format as vd2, vd3 where usual nomenclature is to have
> /^[fhv]d[a-z]+[0-9]*$/.
>
> The following regex should be followed while assigning the disk name: (regex) /^[fhv]d[a-z]+[0-9]*$/
> However, note that any trailing string of digits is simply ignored by libvirt.
>
> A patch has been sent upstream.
>
> The patch has been accepted upstream. commit 5729746.
>
> ** Affects: ubuntu
> Importance: Undecided
> Assignee: Taco Screen team (taco-screen-team)
> Status: New
>
>
> ** Tags: architecture-ppc64le bugnameltc-150842 severity-high targetmilestone-inin16042

--
Michael Hohnbaum
OIL Program Manager
Power (ppc64el) Development Project Manager
Canonical, Ltd.

Is there an example XML where this is reproducible?

------- Comment From <email address hidden> 2017-02-16 13:02 EDT-------
Hi Ryan,

The culprit here seems to be the usage of a non-conventional disk name in the guest XML. For instance:

<disk type='block' device='disk'>
<driver name='qemu' type='raw' cache='none' io='native'/>
<source dev='/dev/sdj'/>
<target dev='vd2' bus='virtio'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x2'/>
</disk>

The target has dev='vd2' and this is causing the issue. You might be able to reproduce this with any domain XML you already have, just changing the target dev name.

Hi,
tested on Xenial (qemu 2.5 + libvirt 1.3.1) and Zesty (qemu 2.8 + libvirt 2.5).

Repro:
$ uvt-kvm create --password=ubuntu test-xenial-extradev release=xenial arch=amd64 label=daily
$ virsh shutdown test-xenial-extrade
$ virsh edit test-xenial-extradev
# change vda -> vd1 and vdb -> vd2
$ virsh start test-xenial-extrade

According to the definition there are neither constraints nor suggestions to the <target dev='X'/> definition:
"The dev attribute indicates the "logical" device name. The actual device name specified is not guaranteed to map to the device name in the guest OS. Treat it as a device ordering hint."

So yeah that seems a bug - setting confirmed for now.

I wondered on an all numerical sha but found:

commit 5729746543cf4df5c7b1adea877b4fe45bf67e51
Author: Nitesh Konkar <email address hidden>
Date: Wed Feb 15 16:44:58 2017 +0530

    Ensure disk names follow the disk name regex

    Currently disk names do not follow the
    (regex) /^[fhv]d[a-z]+[0-9]*$/ completely
    and hence one can assign disk names like
    vd2 etc. This patch ensures that the
    disk names follow the regex mentioned.
    This patch also adds a testcase.

So it is not a bug to handle it wrong, but instead to allow those names while they should not be allowed. And then by being a specifier outside of the expected specification failing to process correctly.
TL;DR of the bug "[a-z]+ was actually a [a-z]*"

I checked and the fix would apply cleanly to Xenial, Yakkety and Zesty.
To some extend (the fix yes, the change to the testcase no) also to Trusty.

The upstream patch is only 2 days old, and the impact somewhat bearable for the moment.

@bugproxy - is "Nitesh Konkar" who submitted the patch one of your people?
I found the very likely related "Nitesh Konkar <nitkon12 linux vnet ibm com>"
If so would you mind giving this like ~1 week more upstream and then let Nitesh report if there was any follow up issue.

If not I think we should be totally fine to backport to all releases to help avoiding users to get into that issue.
Setting "incomplete" on the tasks until such a confirmation or at least some minimal maturing time showed up.

Don't get me wrong - I'm really convinced of the change and might even pre-prepare the uploads already. Just want to be on the safe side.

While there was no feedback here there was also no objection upstream and it got released with 3.1 as expected.
That said I think the impact still does not qualify for an SRU.

I'm preparing to include a fix for zesty atm.

I have added it to Ubuntu's libvirt git and lined that up for a zesty upload together with another bug that shall be fixed in zesty before fully freezing zesty.

=> https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/2536

It just started building and I'll throw a pile of tests at it before moving it forward to proposed.

Given that is only prevents an uncommon user-error I set the older releases to Won't Fix.

Please if you really think this is SRU worthy please get in touch and help me preparing a matching SRU teamplate that would convince the SRU Team.

... Tests still running but so far looking good.

This bug was fixed in the package libvirt - 2.5.0-3ubuntu3

---------------
libvirt (2.5.0-3ubuntu3) zesty; urgency=medium

  [ Christian Ehrhardt ]
  * d/p/ubuntu/Ensure-disk-names-follow-the-disk-name-regex.patch:
    guarantee disk spec is following the defined regex (LP: #1665410).

  [ Bryan Quigley ]
  * d/p/ubuntu/0007-apparmor-fix-for-new-virt-manager.patch: Add Apparmor
    permissions so virt-manager 1.4.0 viewing works (LP: #1668681).

 -- Christian Ehrhardt <email address hidden> Mon, 06 Mar 2017 08:24:06 +0100