Re: [Autotest] [PATCH] KVM test: Update pci_hotplug to suit for new qemu

Re: [Autotest] [PATCH] KVM test: Update pci_hotplug to suit for new qemu

[Lucas Meneghel Rodrigues]

On Fri, 2010-08-06 at 10:23 +0800, Yiqiao Pu wrote:
> In the new version of qemu in RHEL 6.0 device_add is instead of pci_add. So
> modify the scripts to suit for the new version of qemu.
> There is still one problem here. Block device htoplug only supported in qmp
> command line, but qmp don't implement pci query in the current version which
> is needed by the verification of device hotplug. So the block device
> hotplug will report a failed rignt now.
> 
> Signed-off-by: Yiqiao Pu <ypu@redhat.com>
> ---
>  client/tests/kvm/tests/pci_hotplug.py |   64 ++++++++++++++++++++++++---------
>  1 files changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/client/tests/kvm/tests/pci_hotplug.py b/client/tests/kvm/tests/pci_hotplug.py
> index 2c459d7..5202cf6 100644
> --- a/client/tests/kvm/tests/pci_hotplug.py
> +++ b/client/tests/kvm/tests/pci_hotplug.py
> @@ -1,4 +1,4 @@
> -import logging, os
> +import logging, os, commands, re
>  from autotest_lib.client.common_lib import error
>  import kvm_subprocess, kvm_test_utils, kvm_utils, kvm_vm
>  
> @@ -35,26 +35,56 @@ def run_pci_hotplug(test, params, env):
>  
>      tested_model = params.get("pci_model")
>      test_type = params.get("pci_type")
> -
> -    if test_type == "nic":
> -        pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
> -    elif test_type == "block":
> -        image_params = kvm_utils.get_sub_dict(params, "stg")
> -        image_filename = kvm_vm.get_image_filename(image_params, test.bindir)
> -        pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
> -                       (image_filename, tested_model))
> -
> -    # Execute pci_add (should be replaced by a proper monitor method call)
> -    add_output = vm.monitor.cmd(pci_add_cmd)
> -    if not "OK domain" in add_output:
> -        raise error.TestFail("Add device failed. Hypervisor command is: %s. "
> -                             "Output: %r" % (pci_add_cmd, add_output))
> -    after_add = vm.monitor.info("pci")
> +    s, o = commands.getstatusoutput("uname -r")
> +    if len(re.findall("el6", o)) > 0:

^ This check is not good for upstream code, after all we might be
running on qemu-kvm upstream, generic versions that already support the
new syntax. Asking the monitor the set of supported commands seems the
most sensible solution here. Please resolve this issue and re-send the
patch.

> +        cmd_type = "device"
> +    else:
> +        cmd_type = "pci"
> +
> +    if cmd_type == "pci":
> +        if test_type == "nic":
> +            pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
> +        elif test_type == "block":
> +            image_params = kvm_utils.get_sub_dict(params, "stg")
> +            image_filename = kvm_vm.get_image_filename(image_params,
> +                                                        test.bindir)
> +            pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
> +                           (image_filename, tested_model))
> +        # Execute pci_add (should be replaced by a proper monitor method call)
> +        add_output = vm.monitor.cmd(pci_add_cmd)

^ You might want to wrap pci device addition on a new monitor method, as
the TODO comment on this above line suggests. This comment was made by
Michael on his monitor patchset, and since you are working on this test,
it's a good opportunity to get this implemented.

> +        if not "OK domain" in add_output:
> +            raise error.TestFail("Add device failed. Hypervisor command is: %s"
> +                                  ". Output: %r" % (pci_add_cmd, add_output))
> +        after_add = vm.monitor.info("pci")
> +    elif cmd_type == "device":
> +        # block device hotplug is not support rignt now

^ Typo, "supported"

> +        id = test_type + "-" + kvm_utils.generate_random_string(4)
> +        if test_type == "nic":
> +            if tested_model == "virtio":
> +                tested_model = "virtio-net-pci"
> +            pci_add_cmd = "device_add id=%s,driver=%s" % (id, tested_model)
> +        elif test_type == "block":
> +            if tested_model == "virtio":
> +                tested_model = "virtio-blk-pci"
> +            if tested_model == "scsi":
> +                tested_model = "scsi-disk"
> +            pci_add_cmd = "device_add %s,id=%s,driver=%s" % (image_filename,
> +                           id, tested_model)
> +
> +            raise error.TestError("Do not support in current version")

^ Here, we should verify whether the monitor does support pci query
command (at some point, this *will* be implemented). If it doesn't,
throw a error.TestNA with the message "PCI block hotplug is not
supported by the monitor".

> +        add_output = vm.monitor.cmd(pci_add_cmd)
> +        after_add = vm.monitor.info("pci")
> +        if not id in after_add:
> +            raise error.TestFail("Add device failed. Hypervisor command is: %s"
> +                                 ". Output: %r" % (pci_add_cmd, add_output))
>  
>      # Define a helper function to delete the device
>      def pci_del(ignore_failure=False):
> -        slot_id = "0" + add_output.split(",")[2].split()[1]
> -        cmd = "pci_del pci_addr=%s" % slot_id
> +        if cmd_type == "pci":
> +            slot_id = "0" + add_output.split(",")[2].split()[1]
> +            cmd = "pci_del pci_addr=%s" % slot_id
> +        elif cmd_type == "device":
> +            cmd = "device_del id=%s" % id
>          # This should be replaced by a proper monitor method call
>          vm.monitor.cmd(cmd)
>  

Re: [Autotest] [PATCH] KVM test: Update pci_hotplug to suit for new qemu

[Yiqiao Pu]

  On 08/27/2010 11:17 AM, Lucas Meneghel Rodrigues wrote:
> On Fri, 2010-08-06 at 10:23 +0800, Yiqiao Pu wrote:
>> In the new version of qemu in RHEL 6.0 device_add is instead of pci_add. So
>> modify the scripts to suit for the new version of qemu.
>> There is still one problem here. Block device htoplug only supported in qmp
>> command line, but qmp don't implement pci query in the current version which
>> is needed by the verification of device hotplug. So the block device
>> hotplug will report a failed rignt now.
>>
>> Signed-off-by: Yiqiao Pu<ypu@redhat.com>
>> ---
>>   client/tests/kvm/tests/pci_hotplug.py |   64 ++++++++++++++++++++++++---------
>>   1 files changed, 47 insertions(+), 17 deletions(-)
>>
>> diff --git a/client/tests/kvm/tests/pci_hotplug.py b/client/tests/kvm/tests/pci_hotplug.py
>> index 2c459d7..5202cf6 100644
>> --- a/client/tests/kvm/tests/pci_hotplug.py
>> +++ b/client/tests/kvm/tests/pci_hotplug.py
>> @@ -1,4 +1,4 @@
>> -import logging, os
>> +import logging, os, commands, re
>>   from autotest_lib.client.common_lib import error
>>   import kvm_subprocess, kvm_test_utils, kvm_utils, kvm_vm
>>
>> @@ -35,26 +35,56 @@ def run_pci_hotplug(test, params, env):
>>
>>       tested_model = params.get("pci_model")
>>       test_type = params.get("pci_type")
>> -
>> -    if test_type == "nic":
>> -        pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
>> -    elif test_type == "block":
>> -        image_params = kvm_utils.get_sub_dict(params, "stg")
>> -        image_filename = kvm_vm.get_image_filename(image_params, test.bindir)
>> -        pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
>> -                       (image_filename, tested_model))
>> -
>> -    # Execute pci_add (should be replaced by a proper monitor method call)
>> -    add_output = vm.monitor.cmd(pci_add_cmd)
>> -    if not "OK domain" in add_output:
>> -        raise error.TestFail("Add device failed. Hypervisor command is: %s. "
>> -                             "Output: %r" % (pci_add_cmd, add_output))
>> -    after_add = vm.monitor.info("pci")
>> +    s, o = commands.getstatusoutput("uname -r")
>> +    if len(re.findall("el6", o))>  0:
> ^ This check is not good for upstream code, after all we might be
> running on qemu-kvm upstream, generic versions that already support the
> new syntax. Asking the monitor the set of supported commands seems the
> most sensible solution here. Please resolve this issue and re-send the
> patch.
>
>> +        cmd_type = "device"
>> +    else:
>> +        cmd_type = "pci"
>> +
>> +    if cmd_type == "pci":
>> +        if test_type == "nic":
>> +            pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
>> +        elif test_type == "block":
>> +            image_params = kvm_utils.get_sub_dict(params, "stg")
>> +            image_filename = kvm_vm.get_image_filename(image_params,
>> +                                                        test.bindir)
>> +            pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
>> +                           (image_filename, tested_model))
>> +        # Execute pci_add (should be replaced by a proper monitor method call)
>> +        add_output = vm.monitor.cmd(pci_add_cmd)
> ^ You might want to wrap pci device addition on a new monitor method, as
> the TODO comment on this above line suggests. This comment was made by
> Michael on his monitor patchset, and since you are working on this test,
> it's a good opportunity to get this implemented.
>
>> +        if not "OK domain" in add_output:
>> +            raise error.TestFail("Add device failed. Hypervisor command is: %s"
>> +                                  ". Output: %r" % (pci_add_cmd, add_output))
>> +        after_add = vm.monitor.info("pci")
>> +    elif cmd_type == "device":
>> +        # block device hotplug is not support rignt now
> ^ Typo, "supported"
>
>> +        id = test_type + "-" + kvm_utils.generate_random_string(4)
>> +        if test_type == "nic":
>> +            if tested_model == "virtio":
>> +                tested_model = "virtio-net-pci"
>> +            pci_add_cmd = "device_add id=%s,driver=%s" % (id, tested_model)
>> +        elif test_type == "block":
>> +            if tested_model == "virtio":
>> +                tested_model = "virtio-blk-pci"
>> +            if tested_model == "scsi":
>> +                tested_model = "scsi-disk"
>> +            pci_add_cmd = "device_add %s,id=%s,driver=%s" % (image_filename,
>> +                           id, tested_model)
>> +
>> +            raise error.TestError("Do not support in current version")
> ^ Here, we should verify whether the monitor does support pci query
> command (at some point, this *will* be implemented). If it doesn't,
> throw a error.TestNA with the message "PCI block hotplug is not
> supported by the monitor".
>
>> +        add_output = vm.monitor.cmd(pci_add_cmd)
>> +        after_add = vm.monitor.info("pci")
>> +        if not id in after_add:
>> +            raise error.TestFail("Add device failed. Hypervisor command is: %s"
>> +                                 ". Output: %r" % (pci_add_cmd, add_output))
>>
>>       # Define a helper function to delete the device
>>       def pci_del(ignore_failure=False):
>> -        slot_id = "0" + add_output.split(",")[2].split()[1]
>> -        cmd = "pci_del pci_addr=%s" % slot_id
>> +        if cmd_type == "pci":
>> +            slot_id = "0" + add_output.split(",")[2].split()[1]
>> +            cmd = "pci_del pci_addr=%s" % slot_id
>> +        elif cmd_type == "device":
>> +            cmd = "device_del id=%s" % id
>>           # This should be replaced by a proper monitor method call
>>           vm.monitor.cmd(cmd)
>>
Hi Lucas,
Sorry that I didn't see your reply until now, because my bad filter for 
emails. Now we find that the block devices also supported by monitor 
command line(virtio blocks), so we update the patch. I will modify that 
patch according your comments and resend it soon. Thank you.

Best Regards
Yiqiao Pu