Bug 1664324

+++ This bug was initially created as a clone of Bug #1344899 +++

Description of problem:
Failed to attach scsi controller with option "--config --live"

Version-Release number of selected component (if applicable):
libvirt-1.3.5-1.el7.x86_64
qemu-kvm-rhev-2.6.0-5.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Start a domxml without scsi controller.
# virsh start avocado-vt-vm1
Domain avocado-vt-vm1 started

2. Hot-plug a scsi controller device without index.
# cat ctl.xml
<controller model="virtio-scsi" type="scsi" />

# virsh attach-device avocado-vt-vm1 ctl.xml --config --live
error: Failed to attach device from con.xml
error: internal error: Cannot parse controller index -1

Actual results:
Failed to attach scsi controller with option "--config --live"

Expected results:
Device attachment should work well.

Infomation:
The command can work with '--config' or '--live' respectively.

--- Additional comment from Laine Stump on 2016-06-12 21:43:33 UTC ---

This *should have been* taken care of by the call to virDomainControllerFindUnusedIndex() that was added to qemuDomainAttachControllerDevice() in the same commit that made specifying the index optional. Apparently there is a bad interaction with this and combining the --live and --config flags. I'll look into it.

--- Additional comment from Laine Stump on 2016-06-22 19:03:21 UTC ---

Patch posted upstream:

https://www.redhat.com/archives/libvir-list/2016-June/msg01579.html

--- Additional comment from Laine Stump on 2016-06-27 14:17:03 UTC ---

Upstream patch caused other problems; this has turned into more of a general problem with preventing --live additions and --config additions from conflicting with each other (details in the email thread replying to the above patch).

--- Additional comment from Jingjing Shao on 2016-08-29 09:03:18 UTC ---

It gets same error when attach scsi controller with option "--persistent" 

libvirt-2.0.0-6.el7.x86_64
# virsh dumpxml vm-jishao | grep scsi -A8
# 
# 
# virsh start vm-jishao
Domain vm-jishao started

# cat controller.xml 
<controller type='scsi' model='virtio-scsi'/>

# virsh attach-device  vm-jishao controller.xml  --persistent
error: Failed to attach device from controller.xml
error: internal error: Cannot parse controller index -1

--- Additional comment from Jingjing Shao on 2016-08-30 02:49:18 UTC ---

With kernel-3.10.0-327.el7.x86_64, qemu-kvm-1.5.3-105.el7.x86_64,libvirt-1.2.17-13.el7.x86_64, I can not reproduce the issue 

1.# virsh dumpxml vm-jishao | grep scsi
  # 
  # 

2.# virsh start vm-jishao
Domain vm-jishao started

3.# cat scsi-controller.xml 
<controller model="virtio-scsi" type="scsi" />

4.# virsh attach-device  vm-jishao  scsi-controller.xml  --config --live
Device attached successfully

# virsh dumpxml vm-jishao | grep scsi -A8
    <controller type='scsi' index='0' model='virtio-scsi'>
      <alias name='scsi0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
    </controller>


5.# virsh destroy vm-jishao
Domain vm-jishao destroyed

Delete the part of the scsi 
# virsh edit vm-jishao
Domain vm-jishao XML configuration edited.

# virsh start vm-jishao
Domain vm-jishao started

6.# virsh start vm-jishao
Domain vm-jishao started

7.# virsh attach-device  vm-jishao  scsi-controller.xml  --persistent
Device attached successfully

# virsh dumpxml vm-jishao | grep scsi -A8
    <controller type='scsi' index='0' model='virtio-scsi'>
      <alias name='scsi0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
    </controller>

--- Additional comment from Jingjing Shao on 2016-08-30 02:56:23 UTC ---

(In reply to Jingjing Shao from comment #6)
> With kernel-3.10.0-327.el7.x86_64,
> qemu-kvm-1.5.3-105.el7.x86_64,libvirt-1.2.17-13.el7.x86_64, I can not
> reproduce the issue 

The env is with RHEL7.2 release os + qemu-kvm + libvirt 

> 
> 1.# virsh dumpxml vm-jishao | grep scsi
>   # 
>   # 
> 
> 2.# virsh start vm-jishao
> Domain vm-jishao started
> 
> 3.# cat scsi-controller.xml 
> <controller model="virtio-scsi" type="scsi" />
> 
> 4.# virsh attach-device  vm-jishao  scsi-controller.xml  --config --live
> Device attached successfully
> 
> # virsh dumpxml vm-jishao | grep scsi -A8
>     <controller type='scsi' index='0' model='virtio-scsi'>
>       <alias name='scsi0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>     </controller>
> 
> 
> 5.# virsh destroy vm-jishao
> Domain vm-jishao destroyed
> 
> Delete the part of the scsi 
> # virsh edit vm-jishao
> Domain vm-jishao XML configuration edited.
> 
> # virsh start vm-jishao
> Domain vm-jishao started
> 
> 6.# virsh start vm-jishao
> Domain vm-jishao started
> 
> 7.# virsh attach-device  vm-jishao  scsi-controller.xml  --persistent
> Device attached successfully
> 
> # virsh dumpxml vm-jishao | grep scsi -A8
>     <controller type='scsi' index='0' model='virtio-scsi'>
>       <alias name='scsi0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>     </controller>

--- Additional comment from Laine Stump on 2016-09-07 14:28:07 UTC ---

Although I agree that this is annoying once you're accustomed to not supplying an index, I disagree that this is a test blocker - just specify an index in the <controller> element:

<controller model="virtio-scsi" index='0' type="scsi" />

(or index='1' or '2' or whatever is still unused). Until just a few months ago, libvirt always required manually specifying the index in a <controller> element, so it shouldn't be a problem to continue doing that until this one case is fixed.

--- Additional comment from Jingjing Shao on 2016-09-08 02:39:44 UTC ---

(In reply to Laine Stump from comment #9)
> Although I agree that this is annoying once you're accustomed to not
> supplying an index, I disagree that this is a test blocker - just specify an
> index in the <controller> element:
> 
> <controller model="virtio-scsi" index='0' type="scsi" />

Hi laine, 

I try this issue with "<controller model="virtio-scsi" index='0' type="scsi" />"
and the device attachment works well.

> 
> (or index='1' or '2' or whatever is still unused). Until just a few months
> ago, libvirt always required manually specifying the index in a <controller>
> element, so it shouldn't be a problem to continue doing that until this one
> case is fixed.


The operation without supplying an index is not recommended，right?

But I see "Since 1.3.5 the index is optional; if not specified, it will be auto-assigned to be the lowest unused index for the given controller type" in the libvirt.org. http://libvirt.org/formatdomain.html#elementsControllers 

Does it meam we should support this operation ?

--- Additional comment from Laine Stump on 2016-09-08 13:48:03 UTC ---

(In reply to Jingjing Shao from comment #10)

> 
> The operation without supplying an index is not recommended，right?

Currently the only time omitting the index doesn't work properly is the specific case reported in this BZ - if you try to add a controller with --live --config. And since that is the only type of controller that can be hotplugged, it's only an issue for scsi controllers. For all other controller types (and even for scsi controllers if you do the hotplug (--live) without --config, or --config without --live, it works properly.


> 
> But I see "Since 1.3.5 the index is optional; if not specified, it will be
> auto-assigned to be the lowest unused index for the given controller type"
> in the libvirt.org. http://libvirt.org/formatdomain.html#elementsControllers 
> 
> Does it meam we should support this operation ?

Yes. That's why I didn't close the BZ. It is a bug and needs to be fixed. I just don't think it qualifies as a blocker, so we should remove the blocker flag. I didn't want to do that myself though, since I'm not the person who added it.

--- Additional comment from yangyang on 2016-09-09 05:04:49 UTC ---

Hi Laine,

Is there time to fix it in RHEL7.3. If no, can we move it to RHEL7.4

--- Additional comment from Laine Stump on 2016-09-09 14:58:04 UTC ---

Reading through the thread that started from my proposed patch back in Comment 3: https://www.redhat.com/archives/libvir-list/2016-June/msg01579.html I can say that *definitely* this can't be fixed in any reasonable manner for 7.3. (and if anybody else looks at this BZ in the future, I would recommend they read *all* the child messages of that one).

Additionally, I just now noticed that this is marked as a regression. It definitely it *not* a regression. The ability to define controllers without an explicit user-determined index is a very new thing. In all previous releases of libvirt used by RHEL, any controller definition required  index to be specified.

So, I'm removing the Regression keyword, "[regression]" from the summary, and moving this to 7.4

--- Additional comment from Jingjing Shao on 2016-09-12 03:06:41 UTC ---

(In reply to Laine Stump from comment #13)
> Reading through the thread that started from my proposed patch back in
> Comment 3:
> https://www.redhat.com/archives/libvir-list/2016-June/msg01579.html I can
> say that *definitely* this can't be fixed in any reasonable manner for 7.3.
> (and if anybody else looks at this BZ in the future, I would recommend they
> read *all* the child messages of that one).
> 
> Additionally, I just now noticed that this is marked as a regression. It
> definitely it *not* a regression. The ability to define controllers without
> an explicit user-determined index is a very new thing. In all previous
> releases of libvirt used by RHEL, any controller definition required  index
> to be specified.

Hi laine, 

Would you please check the comment6 ? I mark it as a regression for that I test this issue on the env with RHEL7.2 release os + qemu-kvm + libvirt.
The device attachment can work well.

So I can not quite understand it, can you give me a more detailed explanation about it?  Thank you in advance


> 
> So, I'm removing the Regression keyword, "[regression]" from the summary,
> and moving this to 7.4

--- Additional comment from Laine Stump on 2016-09-12 13:44:20 UTC ---

Okay, I can see how this xml:

  <controller model="virtio-scsi" type='scsi' />

would have been accepted by libvirt and worked prior to the "auto-generate unspecified controller indexes" feature being added (which happened in 1.3.5) - if no index was given, the index would have remained at its initial value, which was 0. So the next time it was parsed, it would format as "index='0'".

However, the fact that it worked was just an accident, and it would only work for index='0'. The libvirt documentation has always stated this:

  "Each controller has [...], and a mandatory attribute index ..."

Also, the XML grammer (which isn't referenced when defining a domain) specifically required an index.

I'm now undecided whether this should be regression or not, since the behavior does change, but the behavior that has changed was previously documented as illegal...