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<yz...@redhat.com> wrote:
> On 07/03/2009 01:57 PM, Yolkfull Chow wrote:
>> Signed-off-by: Yolkfull Chow<yz...@redhat.com>
>> ---
>> client/tests/kvm/kvm.py | 1 +
>> client/tests/kvm/kvm_tests.cfg.sample | 65 ++++++++++++++++++++++-
>> client/tests/kvm/kvm_tests.py | 94
>> +++++++++++++++++++++++++++++++++
>> client/tests/kvm/kvm_vm.py | 2 +
>> 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):
>> "kvm_install": test_routine("kvm_install",
>> "run_kvm_install"),
>> "linux_s3": test_routine("kvm_tests", "run_linux_s3"),
>> "stress_boot": test_routine("kvm_tests",
>> "run_stress_boot"),
>> + "pci_hotplug": test_routine("kvm_tests",
>> "run_pci_hotplug"),
>> }
>>
>> # Make it possible to import modules from the test's bindir
>> diff --git a/client/tests/kvm/kvm_tests.cfg.sample
>> b/client/tests/kvm/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:
>> max_vms = 5
>> alive_test_cmd = ps aux
>>
>> +
>> + - nic_hotplug:
>> + type = pci_hotplug
>> + pci_type = nic
>> + modprobe_acpiphp = yes
>> + reference_cmd = lspci
>> + find_pci_cmd = '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?
>> + pci_test_cmd = 'nslookup www.redhat.com'
>> + seconds_wait_for_device_install = 3
>> + variants:
>> + - @nic_8139:
>> + pci_model = rtl8139
>> + match_string = "8139"
>> + - nic_virtio:
>> + pci_model = virtio
>> + match_string = "Virtio network device"
>> + - nic_e1000:
>> + pci_model = e1000
>> + match_string = "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).
>> + - block_hotplug:
>> + type = pci_hotplug
>> + pci_type = block
>> + modprobe_acpiphp = yes
>> + reference_cmd = lspci
>> + find_pci_cmd = 'lspci | tail -n1'
>> + images += " stg"
>> + boot_drive_stg = no
>> + image_name_stg = storage
>> + image_size = 1G
>> + force_create_image_stg = yes
>> + pci_test_cmd = 'dir'
>> + seconds_wait_for_device_install = 3
>> + variants:
>> + - block_virtio:
>> + pci_model = virtio
>> + match_string = "Virtio block device"
>> + - block_scsi:
>> + pci_model = scsi
>> + match_string = "SCSI storage controller"
>> + variants:
>> + - fmt_qcow2:
>> + image_format_stg = qcow2
>> + - fmt_raw:
>> + image_format_stg = raw
>> +
>> +
>> # NICs
>> variants:
>> - @rtl8139:
>> @@ -306,6 +353,22 @@ variants:
>> migration_test_command = ver&& vol
>> stress_boot:
>> alive_test_cmd = systeminfo
>> + nic_hotplug:
>> + modprobe_acpiphp = no
>> + reference_cmd = systeminfo
>> + seconds_wait_for_device_install = 10
>> + find_pci_cmd = ipconfig /all | find "Description"
>> + nic_e1000:
>> + match_string = "Intel(R) PRO/1000 MT Network Connection"
>> + block_hotplug:
>> + use_telnet = yes
>> + modprobe_acpiphp = no
>> + reference_cmd = wmic diskdrive
>> + find_pci_cmd = wmic diskdrive | find "disk drives"
>> + seconds_wait_for_device_install = 10
>> + only block_scsi
>> + block_scsi:
>> + match_string = "SCSI"
>>
> It supports Windows block_hotplug now. But we need use_telnet to login
> Windows OS since command 'wmic' could only run on telnet session.
>>
>> variants:
>> - Win2000:
>> @@ -571,4 +634,4 @@ variants:
>> only rtl8139
>>
>> # Choose your test list
>> -only fc8_quick
>> +#only fc8_quick
>> diff --git a/client/tests/kvm/kvm_tests.py b/client/tests/kvm/kvm_tests.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):
>> for se in sessions:
>> se.close()
>> logging.info("Total number booted: %d" % (num -1))
>> +
>> +
>> +def run_pci_hotplug(test, params, env):
>> + """
>> + Test pci devices' hotplug
>> + 1) pci_add a deivce (nic or storage)
>> + 2) Compare 'info pci' output
>> + 3) Compare $reference_cmd output
>> + 4) Verify whether pci_model is shown in $pci_find_cmd
>> + 5) 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 mentioned above.
>> + �...@param test: kvm test object
>> + �...@param params: Dictionary with the test parameters
>> + �...@param env: Dictionary with test environment.
>> + """
>> + vm = kvm_utils.env_get_vm(env, params.get("main_vm"))
>> + if not vm:
>> + raise error.TestError("VM object not found in environment")
>> + if not vm.is_alive():
>> + raise error.TestError("VM seems to be dead; Test requires a living
>> VM")
>> +
>> + logging.info("Waiting for guest to be up...")
>> +
>> + session = kvm_utils.wait_for(vm.ssh_login, 240, 0, 2)
>> + if not session:
>> + raise error.TestFail("Could not log into guest")
>> +
>> + logging.info("Logged in")
>> +
>> + # modprobe the module that enable hotplug
>> + if params.get("modprobe_acpiphp") == "yes":
>> + if session.get_command_status("modprobe acpiphp"):
>> + raise error.TestError("Modprobe module 'acpiphp' failed")
>> +
>> + # get reference output
>> + s, info_pci_ref = vm.send_monitor_cmd("info pci")
>> +
>> + # compare the output of `reference_cmd`
>> + ref_cmd = params.get("reference_cmd")
>> + reference = session.get_command_output(ref_cmd)
>> +
>> + # implement pci hotplug
>> + tested_model = params.get("pci_model")
>> + logging.info("Testing hotplug pci device:%s" % tested_model)
>> +
>> + test_type = params.get("pci_type")
>> + if test_type == "nic":
>> + pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
>> + s, add_output = vm.send_monitor_cmd(pci_add_cmd)
The command
s, add_output = 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).
>> + elif test_type == "block":
>> + image_name = params.get("image_name_stg")
>> + image_format = params.get("image_format_stg", "qcow2")
>> + image_filename = "%s.%s" % (image_name, image_format)
>> + image_dir = os.path.join(test.bindir, "images")
>> + storage_name = os.path.join(image_dir, image_filename)
>> + pci_add_cmd = "pci_add pci_addr=auto storage file=%s,if=%s" % \
>> + (storage_name, tested_model)
The above line would be better with implicit line continuation
pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
(storage_name, tested_model))
>> + s, add_output = vm.send_monitor_cmd(pci_add_cmd)
>> +
>> + if not "OK domain" in add_output:
>> + raise 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'failed."
"Output: %s" % (pci_add_cmd, add_output))
>> + # compare the output of 'info pci'
>> + s, after_add = vm.send_monitor_cmd("info pci")
>> + if after_add == info_pci_ref:
>> + raise error.TestFail("No new pci device shown in 'info pci'")
Another suggestion:
raise error.TestFail("No new PCI device shown after executing "
"hypervisor command 'info pci'")
>> + time.sleep(int(params.get("seconds_wait_for_device_install")))
>> +
>> + o = session.get_command_output(ref_cmd)
>> + if reference == o:
>> + raise 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 output" %
ref_cmd)
>> + cmd = params.get("find_pci_cmd")
>> + output = session.get_command_output(cmd)
>> + if not params.get("match_string") in output:
>> + raise error.TestFail("Not found pci model: %s; Command is: %s" %
>> + (tested_model, cmd))
raise error.TestFail("PCI device model '%s' not found on host "
"command '%s' ouput" % (tested_model, cmd))
>> + # del pci device
>> + slot_id = "0" + add_output.split(",")[2].split()[1]
>> + cmd = "pci_del pci_addr=%s" % slot_id
>> + s, after_del = vm.send_monitor_cmd(cmd)
>> + if after_del == after_add:
>> + raise error.TestFail("Failed to hot remove pci device:%s;
>> Command:%s" %
>> + (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)?
>> + # check whether VM's network& disk work fine
>> + if session.get_command_status(params.get("pci_test_cmd")):
>> + raise error.TestFail("Check for %s device failed after PCI hotplug"
>> %
>> + test_type)
>> +
>> + session.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:
>>
>> for image_name in kvm_utils.get_sub_dict_names(params, "images"):
>> image_params = kvm_utils.get_sub_dict(params, image_name)
>> + if image_params.get("boot_drive") == "no":
>> + continue
>> qemu_cmd += " -drive file=%s" %
>> get_image_filename(image_params,
>> image_dir)
>> if image_params.get("drive_format"):
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
