From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lucas Meneghel Rodrigues <lmr@redhat.com>
Subject: Re: [Autotest] [PATCH] Add a subtest pci_hotplug in kvm test
Date: Tue, 7 Jul 2009 22:51:31 -0300
Message-ID: <6ac58f4f0907071851k1d484f18o4100b30281160b31@mail.gmail.com>
References: <1246600652-16339-1-git-send-email-yzhou@redhat.com>
	 <4A4D9E8F.9010802@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: kvm@vger.kernel.org, autotest@test.kernel.org
To: Yolkfull Chow <yzhou@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-vw0-f199.google.com ([209.85.212.199]:38947 "EHLO
	mail-vw0-f199.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754743AbZGHB5H convert rfc822-to-8bit (ORCPT
	<rfc822;kvm@vger.kernel.org>); Tue, 7 Jul 2009 21:57:07 -0400
Received: by vwj37 with SMTP id 37so115348vwj.33
        for <kvm@vger.kernel.org>; Tue, 07 Jul 2009 18:57:04 -0700 (PDT)
In-Reply-To: <4A4D9E8F.9010802@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

I've spent some time doing a second review and test of the code.
During my tests:

 * I found some problems with PCI hotplug itself and would like help
to figure out why things are not working as expected.
 * Made suggestions regarding the phrasing of the error messages
thrown by the test. Mostly nipticking. Let me know if you think the
new messages make sense.
 * The order of the final test steps looks a bit weird to me

Comments follow.

On Fri, Jul 3, 2009 at 3:00 AM, Yolkfull Chow<yzhou@redhat.com> wrote:
> On 07/03/2009 01:57 PM, Yolkfull Chow wrote:
>> Signed-off-by: Yolkfull Chow<yzhou@redhat.com>
>> ---
>> =A0 client/tests/kvm/kvm.py =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A01 +
>> =A0 client/tests/kvm/kvm_tests.cfg.sample | =A0 65 +++++++++++++++++=
+++++-
>> =A0 client/tests/kvm/kvm_tests.py =A0 =A0 =A0 =A0 | =A0 94 +++++++++=
++++++++++++++++++++++++
>> =A0 client/tests/kvm/kvm_vm.py =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A02 +
>> =A0 4 files changed, 161 insertions(+), 1 deletions(-)
>>
>> diff --git a/client/tests/kvm/kvm.py b/client/tests/kvm/kvm.py
>> index b18b643..fc92e10 100644
>> --- a/client/tests/kvm/kvm.py
>> +++ b/client/tests/kvm/kvm.py
>> @@ -55,6 +55,7 @@ class kvm(test.test):
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "kvm_install": =A0test_routine("=
kvm_install", "run_kvm_install"),
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "linux_s3": =A0 =A0 test_routine=
("kvm_tests", "run_linux_s3"),
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "stress_boot": =A0test_routine("=
kvm_tests", "run_stress_boot"),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"pci_hotplug": =A0test_routine("kvm=
_tests", "run_pci_hotplug"),
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 =A0 =A0 # Make it possible to import modules from the te=
st's bindir
>> diff --git a/client/tests/kvm/kvm_tests.cfg.sample b/client/tests/kv=
m/kvm_tests.cfg.sample
>> index 2f864de..a9e16d6 100644
>> --- a/client/tests/kvm/kvm_tests.cfg.sample
>> +++ b/client/tests/kvm/kvm_tests.cfg.sample
>> @@ -94,6 +94,53 @@ variants:
>> =A0 =A0 =A0 =A0 =A0 max_vms =3D 5
>> =A0 =A0 =A0 =A0 =A0 alive_test_cmd =3D ps aux
>>
>> +
>> + =A0 =A0- nic_hotplug:
>> + =A0 =A0 =A0 =A0type =3D pci_hotplug
>> + =A0 =A0 =A0 =A0pci_type =3D nic
>> + =A0 =A0 =A0 =A0modprobe_acpiphp =3D yes
>> + =A0 =A0 =A0 =A0reference_cmd =3D lspci
>> + =A0 =A0 =A0 =A0find_pci_cmd =3D 'lspci | tail -n1'

I tried block device hotplug, lspci doesn't show up the newly added
devices. Already tried with F8 and F9. Any idea why?

>> + =A0 =A0 =A0 =A0pci_test_cmd =3D 'nslookup www.redhat.com'
>> + =A0 =A0 =A0 =A0seconds_wait_for_device_install =3D 3
>> + =A0 =A0 =A0 =A0variants:
>> + =A0 =A0 =A0 =A0 =A0 =A0- @nic_8139:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_model =3D rtl8139
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "8139"
>> + =A0 =A0 =A0 =A0 =A0 =A0- nic_virtio:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_model =3D virtio
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "Virtio network de=
vice"
>> + =A0 =A0 =A0 =A0 =A0 =A0- nic_e1000:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_model =3D e1000
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "Gigabit Ethernet =
Controller"

Pretty much all block hotplug 'guest side check' is failing during the
stage where the output  of lspci | tail -n1 is being compared with the
match strings. Hypervisor is qemu 0.10.5 (kvm-87 upstream).

>> + =A0 =A0- block_hotplug:
>> + =A0 =A0 =A0 =A0type =3D pci_hotplug
>> + =A0 =A0 =A0 =A0pci_type =3D block
>> + =A0 =A0 =A0 =A0modprobe_acpiphp =3D yes
>> + =A0 =A0 =A0 =A0reference_cmd =3D lspci
>> + =A0 =A0 =A0 =A0find_pci_cmd =3D 'lspci | tail -n1'
>> + =A0 =A0 =A0 =A0images +=3D " stg"
>> + =A0 =A0 =A0 =A0boot_drive_stg =3D no
>> + =A0 =A0 =A0 =A0image_name_stg =3D storage
>> + =A0 =A0 =A0 =A0image_size =3D 1G
>> + =A0 =A0 =A0 =A0force_create_image_stg =3D yes
>> + =A0 =A0 =A0 =A0pci_test_cmd =3D 'dir'
>> + =A0 =A0 =A0 =A0seconds_wait_for_device_install =3D 3
>> + =A0 =A0 =A0 =A0variants:
>> + =A0 =A0 =A0 =A0 =A0 =A0- block_virtio:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_model =3D virtio
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "Virtio block devi=
ce"
>> + =A0 =A0 =A0 =A0 =A0 =A0- block_scsi:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pci_model =3D scsi
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "SCSI storage cont=
roller"
>> + =A0 =A0 =A0 =A0variants:
>> + =A0 =A0 =A0 =A0 =A0 =A0- fmt_qcow2:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_format_stg =3D qcow2
>> + =A0 =A0 =A0 =A0 =A0 =A0- fmt_raw:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_format_stg =3D raw
>> +
>> +
>> =A0 # NICs
>> =A0 variants:
>> =A0 =A0 =A0 - @rtl8139:
>> @@ -306,6 +353,22 @@ variants:
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 migration_test_command =3D ver&& =A0vol
>> =A0 =A0 =A0 =A0 =A0 stress_boot:
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 alive_test_cmd =3D systeminfo
>> + =A0 =A0 =A0 =A0nic_hotplug:
>> + =A0 =A0 =A0 =A0 =A0 =A0modprobe_acpiphp =3D no
>> + =A0 =A0 =A0 =A0 =A0 =A0reference_cmd =3D systeminfo
>> + =A0 =A0 =A0 =A0 =A0 =A0seconds_wait_for_device_install =3D 10
>> + =A0 =A0 =A0 =A0 =A0 =A0find_pci_cmd =3D ipconfig /all | find "Desc=
ription"
>> + =A0 =A0 =A0 =A0 =A0 =A0nic_e1000:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "Intel(R) PRO/1000=
 MT Network Connection"
>> + =A0 =A0 =A0 =A0block_hotplug:
>> + =A0 =A0 =A0 =A0 =A0 =A0use_telnet =3D yes
>> + =A0 =A0 =A0 =A0 =A0 =A0modprobe_acpiphp =3D no
>> + =A0 =A0 =A0 =A0 =A0 =A0reference_cmd =3D wmic diskdrive
>> + =A0 =A0 =A0 =A0 =A0 =A0find_pci_cmd =3D wmic diskdrive | find "dis=
k drives"
>> + =A0 =A0 =A0 =A0 =A0 =A0seconds_wait_for_device_install =3D 10
>> + =A0 =A0 =A0 =A0 =A0 =A0only block_scsi
>> + =A0 =A0 =A0 =A0 =A0 =A0block_scsi:
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0match_string =3D "SCSI"
>>
> It supports Windows block_hotplug now. But we need use_telnet to logi=
n
> Windows OS since command 'wmic' could only run on telnet session.
>>
>> =A0 =A0 =A0 =A0 =A0 variants:
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 - Win2000:
>> @@ -571,4 +634,4 @@ variants:
>> =A0 =A0 =A0 =A0 =A0 only rtl8139
>>
>> =A0 # Choose your test list
>> -only fc8_quick
>> +#only fc8_quick
>> diff --git a/client/tests/kvm/kvm_tests.py b/client/tests/kvm/kvm_te=
sts.py
>> index 2d11fed..230385a 100644
>> --- a/client/tests/kvm/kvm_tests.py
>> +++ b/client/tests/kvm/kvm_tests.py
>> @@ -585,3 +585,97 @@ def run_stress_boot(tests, params, env):
>> =A0 =A0 =A0 =A0 =A0 for se in sessions:
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 se.close()
>> =A0 =A0 =A0 =A0 =A0 logging.info("Total number booted: %d" % (num -1=
))
>> +
>> +
>> +def run_pci_hotplug(test, params, env):
>> + =A0 =A0"""
>> + =A0 =A0Test pci devices' hotplug
>> + =A0 =A01) pci_add a deivce (nic or storage)
>> + =A0 =A02) Compare 'info pci' output
>> + =A0 =A03) Compare $reference_cmd output
>> + =A0 =A04) Verify whether pci_model is shown in $pci_find_cmd
>> + =A0 =A05) pci_del the device, verify whether could remove the pci =
device

What about if the last steps were

5) Verify if the newly added devices are working properly
6) pci_del the device, verify whether could remove the pci device

6) is already happening in the code, but in reverse order than mentione=
d above.

>> + =A0 =A0@param test: =A0 kvm test object
>> + =A0 =A0@param params: Dictionary with the test parameters
>> + =A0 =A0@param env: =A0 =A0Dictionary with test environment.
>> + =A0 =A0"""
>> + =A0 =A0vm =3D kvm_utils.env_get_vm(env, params.get("main_vm"))
>> + =A0 =A0if not vm:
>> + =A0 =A0 =A0 =A0raise error.TestError("VM object not found in envir=
onment")
>> + =A0 =A0if not vm.is_alive():
>> + =A0 =A0 =A0 =A0raise error.TestError("VM seems to be dead; Test re=
quires a living VM")
>> +
>> + =A0 =A0logging.info("Waiting for guest to be up...")
>> +
>> + =A0 =A0session =3D kvm_utils.wait_for(vm.ssh_login, 240, 0, 2)
>> + =A0 =A0if not session:
>> + =A0 =A0 =A0 =A0raise error.TestFail("Could not log into guest")
>> +
>> + =A0 =A0logging.info("Logged in")
>> +
>> + =A0 =A0# modprobe the module that enable hotplug
>> + =A0 =A0if params.get("modprobe_acpiphp") =3D=3D "yes":
>> + =A0 =A0 =A0 =A0if session.get_command_status("modprobe acpiphp"):
>> + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestError("Modprobe module 'acp=
iphp' failed")
>> +
>> + =A0 =A0# get reference output
>> + =A0 =A0s, info_pci_ref =3D vm.send_monitor_cmd("info pci")
>> +
>> + =A0 =A0# compare the output of `reference_cmd`
>> + =A0 =A0ref_cmd =3D params.get("reference_cmd")
>> + =A0 =A0reference =3D session.get_command_output(ref_cmd)
>> +
>> + =A0 =A0# implement pci hotplug
>> + =A0 =A0tested_model =3D params.get("pci_model")
>> + =A0 =A0logging.info("Testing hotplug pci device:%s" % tested_model=
)
>> +
>> + =A0 =A0test_type =3D params.get("pci_type")
>> + =A0 =A0if test_type =3D=3D "nic":
>> + =A0 =A0 =A0 =A0pci_add_cmd =3D "pci_add pci_addr=3Dauto nic model=3D=
%s" % tested_model
>> + =A0 =A0 =A0 =A0s, add_output =3D vm.send_monitor_cmd(pci_add_cmd)

The command

s, add_output =3D vm.send_monitor_cmd(pci_add_cmd)

Can be moved out the if blocks since it will be executed in both
paths, we save one line of code with this (and it's cleaner anyway).

>> + =A0 =A0elif test_type =3D=3D "block":
>> + =A0 =A0 =A0 =A0image_name =3D params.get("image_name_stg")
>> + =A0 =A0 =A0 =A0image_format =3D params.get("image_format_stg", "qc=
ow2")
>> + =A0 =A0 =A0 =A0image_filename =3D "%s.%s" % (image_name, image_for=
mat)
>> + =A0 =A0 =A0 =A0image_dir =3D os.path.join(test.bindir, "images")
>> + =A0 =A0 =A0 =A0storage_name =3D os.path.join(image_dir, image_file=
name)
>> + =A0 =A0 =A0 =A0pci_add_cmd =3D "pci_add pci_addr=3Dauto storage fi=
le=3D%s,if=3D%s" % \
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0(storage_name, tested_model)

The above line would be better with implicit line continuation

pci_add_cmd =3D ("pci_add pci_addr=3Dauto storage file=3D%s,if=3D%s" %
                        (storage_name, tested_model))

>> + =A0 =A0 =A0 =A0s, add_output =3D vm.send_monitor_cmd(pci_add_cmd)
>> +
>> + =A0 =A0if not "OK domain" in add_output:
>> + =A0 =A0 =A0 =A0raise error.TestFail("Command failed: %s" % pci_add=
_cmd)

Since we're executing a command on the hypervisor to add the device, a
better way to put the above message would be

    if not "OK domain" in add_output:
        raise error.TestFail("Hypervisor command to add device '%s'fail=
ed."
                             "Output: %s" % (pci_add_cmd, add_output))

>> + =A0 =A0# compare the output of 'info pci'
>> + =A0 =A0s, after_add =3D vm.send_monitor_cmd("info pci")
>> + =A0 =A0if after_add =3D=3D info_pci_ref:
>> + =A0 =A0 =A0 =A0raise error.TestFail("No new pci device shown in 'i=
nfo pci'")

Another suggestion:

        raise error.TestFail("No new PCI device shown after executing "
                             "hypervisor command 'info pci'")


>> + =A0 =A0time.sleep(int(params.get("seconds_wait_for_device_install"=
)))
>> +
>> + =A0 =A0o =3D session.get_command_output(ref_cmd)
>> + =A0 =A0if reference =3D=3D o:
>> + =A0 =A0 =A0 =A0raise error.TestFail("No new device shown in cmd: %=
s" % ref_cmd)

I found useful to explain on which side of the check the test fail,
'hypervisor', or 'guest'

        raise error.TestFail("No new device shown on guest command %s o=
utput" %
                             ref_cmd)


>> + =A0 =A0cmd =3D params.get("find_pci_cmd")
>> + =A0 =A0output =3D session.get_command_output(cmd)
>> + =A0 =A0if not params.get("match_string") in output:
>> + =A0 =A0 =A0 =A0raise error.TestFail("Not found pci model: %s; Comm=
and is: %s" %
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(tested_model, cmd))

        raise error.TestFail("PCI device model '%s' not found on host "
                             "command '%s' ouput" % (tested_model, cmd)=
)

>> + =A0 =A0# del pci device
>> + =A0 =A0slot_id =3D "0" + add_output.split(",")[2].split()[1]
>> + =A0 =A0cmd =3D "pci_del pci_addr=3D%s" % slot_id
>> + =A0 =A0s, after_del =3D vm.send_monitor_cmd(cmd)
>> + =A0 =A0if after_del =3D=3D after_add:
>> + =A0 =A0 =A0 =A0raise error.TestFail("Failed to hot remove pci devi=
ce:%s; Command:%s" %
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (tested_model, cmd))

        raise error.TestFail("Failed to hot remove pci device '%s'. "
                             "Hypervisor command: '%s'" % (tested_model=
, cmd))

Hot remove doesn't work for me either... Since we are here, I still
didn't understand why we do a hot remove of a device and right below
it we try to verify if the newly added devices are working properly.
Shouldn't be the other way around (ie checking and then hot removing)?

>> + =A0 =A0# check whether VM's network& =A0disk work fine
>> + =A0 =A0if session.get_command_status(params.get("pci_test_cmd")):
>> + =A0 =A0 =A0 =A0raise error.TestFail("Check for %s device failed af=
ter PCI hotplug" %
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_type)
>> +
>> + =A0 =A0session.close()
>> diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
>> index 503f636..95b55eb 100644
>> --- a/client/tests/kvm/kvm_vm.py
>> +++ b/client/tests/kvm/kvm_vm.py
>> @@ -239,6 +239,8 @@ class VM:
>>
>> =A0 =A0 =A0 =A0 =A0 for image_name in kvm_utils.get_sub_dict_names(p=
arams, "images"):
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 image_params =3D kvm_utils.get_sub_dict(=
params, image_name)
>> + =A0 =A0 =A0 =A0 =A0 =A0if image_params.get("boot_drive") =3D=3D "n=
o":
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_cmd +=3D " -drive file=3D%s" % get_=
image_filename(image_params,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_di=
r)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if image_params.get("drive_format"):