Hi, Thanks for the contribution and updates. Here are some comments that hopefully will help polish this up further. Overall it's coming together nicely!
Tip: This is a fairly short set of patches, so don't bother here. But in the future for anything more complex, it's helpful to reviewers if you use multiple discrete commits. Then use git format-patch cover-letter option to make multiple patch e-mails. On 07/24/2012 05:30 AM, sathn...@linux.vnet.ibm.com wrote: > From: Satheesh Rajendran<sathn...@linux.vnet.ibm.com> > > libvirt autotest patch to test virsh vcpupin command which in turn covers > vcpuinfo,vcpucount commands > This test covers the following git hub issue > https://github.com/autotest/autotest/issues/464 > Includes the test for cpu affinity in the respective proc entry also. > > Signed-off-by: Satheesh Rajendran<sathn...@linux.vnet.ibm.com> > --- > client/tests/libvirt/tests/virsh_vcpupin.py | 126 > +++++++++++++++++++++++++++ > client/virt/libvirt_vm.py | 32 +++++++ > client/virt/subtests.cfg.sample | 3 + > 3 files changed, 161 insertions(+), 0 deletions(-) > create mode 100644 client/tests/libvirt/tests/virsh_vcpupin.py > > diff --git a/client/tests/libvirt/tests/virsh_vcpupin.py > b/client/tests/libvirt/tests/virsh_vcpupin.py > new file mode 100644 > index 0000000..1f6cfc3 > --- /dev/null > +++ b/client/tests/libvirt/tests/virsh_vcpupin.py > @@ -0,0 +1,126 @@ > +import logging, re, os, commands, string, math > +from autotest.client.shared import utils, error > +from autotest.client.virt import libvirt_vm > + > +def run_virsh_vcpupin(test, params, env): > + > + """ > + Test the command virsh hostname > + > + (1) Get the host and guest cpu count > + (2) Call virsh vcpupin for each vcpu with pinning of each cpu > + (3) Check whether the virsh vcpupin has pinned the respective vcpu to cpu > + (4) TODO: Right now the testcase covers the pinning one cpu at a time > + this can be improved by a random number of cpus > + """ > + > + # Initialize the variables > + expected_affinity = [] > + total_affinity = [] > + actual_affinity = [] > + > + def build_actual_info(domname,vcpu): > + > + """ > + This function returns list of the vcpu's affinity from > + virsh vcpuinfo output > + > + @param: domname: VM Name to operate on > + @param: vcpu: vcpu number for which the affinity is required > + """ Style nit-pick: Function/Method headers should have one space between parameter names. There shouldn't be a blank line in between header and docstring. However, one blank line should be after docstring. > + output = libvirt_vm.virsh_vcpuinfo(domname) > + cmd = re.findall('[^Affinity:][-y]+', str(output)) > + total_affinity = cmd[vcpu].lstrip() > + actual_affinity = list(total_affinity) > + return actual_affinity > + > + def build_expected_info(vcpu,cpu): > + > + """ > + This function returns the list of vcpu's expected affinity build > + > + @param: vcpu: vcpu number for which the affinity is required > + @param: cpu: cpu details for the affinity > + """ > + expected_affinity = [] > + Same style issue here. > + for i in range(int(host_cpu_count)): > + expected_affinity.append('y') > + > + for i in range(int(host_cpu_count)): > + if cpu != i: > + expected_affinity[i] = '-' > + > + expected_affinity_proc = int(math.pow(2,cpu)) > + return expected_affinity,expected_affinity_proc > + > + def guest_vcpu_pids(domname): > + > + """ > + This function gets the pids of vcpu > + """ Style. > + cmd = "virsh qemu-monitor-command %s --hmp 'info cpus'" % domname > + output_2 = utils.run(cmd, ignore_status=False) > + output_3 = re.findall(r'thread_id=(\d+)', output_2.stdout) > + return output_3 It would be nice to have more descriptive names than 'output_2' and 'output_3'. It's not a show-stopper though. > + > + def virsh_check_vcpupin(domname,vcpu,cpu,pid): > + > + """ > + This function checks the actual and the expected affinity of given > vcpu > + and raises error if not matchs > + > + @param: domname: VM Name to operate on > + @param: vcpu: vcpu number for which the affinity is required > + @param: cpu: cpu details for the affinity > + """ Style. > + expected_output,expected_output_proc = build_expected_info(vcpu,cpu) > + actual_output = build_actual_info(domname,vcpu) > + > + # Get the vcpus pid > + vcpus_pid = guest_vcpu_pids(domname) > + vcpu_pid=vcpus_pid[vcpu] > + > + # Get the actual cpu affinity value in the proc entry > + cmd = "cat /proc/%s/task/%s/status|grep Cpus_allowed:| awk '{print > $2}'" % (pid,vcpu_pid) > + output = str(utils.run(cmd, ignore_status=False).stdout) > + actual_output_proc = int(output,16) This is really cool functionality! I can see how it might be useful in other tests, even beyond the virt. autotest subtests. Maybe consider adding a cpu_affinity_by_task() function into a client/shared/ module somewhere? > + > + if expected_output == actual_output: > + logging.info("successfully pinned cpu: %s --> vcpu: %s", > cpu,vcpu) > + else: > + raise error.TestFail("Command 'virsh vcpupin %s %s %s'not > succeeded" > + ", cpu pinning details not updated properly > in" > + " virsh vcpuinfo command output" > + % > (vm_name,vcpu,cpu)) > + > + if expected_output_proc == actual_output_proc: > + logging.info("successfully pinned cpu: %s --> vcpu: %s" > + " in respective proc entry" > + ,cpu,vcpu) > + else: > + raise error.TestFail("Command 'virsh vcpupin %s %s %s'not > succeeded" > + " cpu pinning details not updated properly > in" > + "/proc/%s/task/%s/status" > + > %(vm_name,vcpu,cpu,pid,vcpu_pid)) > + This is good, I like this easy-to-read check and feedback to the logs and/or user. Nice job. > + > + # Get the vm name, pid of vm and check for alive > + vm_name = params.get("main_vm") > + vm = env.get_vm(params["main_vm"]) > + vm.verify_alive() > + pid = vm.get_pid() > + > + # Get the host cpu count > + cmd = "cat /proc/cpuinfo |grep processor|wc -l" > + host_cpu_count_result = utils.run(cmd, ignore_status=False) > + host_cpu_count = host_cpu_count_result.stdout.strip() I've seen a few tests that query the # of host CPUs. Maybe consider adding this as a generic function to a client/virt or client/shared module? It's good to share generic code like this between tests for maintenance/enhancement reasons. > + > + # Get the guest vcpu count > + guest_vcpu_count = libvirt_vm.virsh_vcpucount_live(vm_name) > + > + # Run test case > + for vcpu in range(int(guest_vcpu_count)): > + for cpu in range(int(host_cpu_count)): > + libvirt_vm.virsh_vcpupin(vm_name,vcpu,cpu) > + virsh_check_vcpupin(vm_name,vcpu,cpu,pid) This is good, I like the simplicity of doing this in a loop. > diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py > index 4e27482..a631249 100644 > --- a/client/virt/libvirt_vm.py > +++ b/client/virt/libvirt_vm.py > @@ -116,6 +116,38 @@ def virsh_cmd(cmd, uri="", ignore_status=False, > print_info=False): > return ret > > > +def > virsh_vcpupin(domname,vcpu,cpu,uri="",ignore_status=False,print_info=False): > + > + """ > + Changes the cpu affinity for respective vcpu. > + """ Style. > + try: > + cmd_vcpupin = "vcpupin %s %s %s" % (domname,vcpu,cpu) > + virsh_cmd(cmd_vcpupin,uri) I think you intended to pass ignore_status and print_info parameters here. > + > + except error.CmdError, detail: > + logging.error("Virsh vcpupin VM %s failed:\n%s", name, detail) > + return False > + > + > +def virsh_vcpuinfo(domname,uri="",ignore_status=False,print_info=False): > + > + """ > + Prints the vcpuinfo of a given domain. > + """ Style. I'll stop complaining, you get the idea by now :) > + cmd_vcpuinfo = "vcpuinfo %s" % domname > + return virsh_cmd(cmd_vcpuinfo,uri).stdout.strip() Again, I think maybe you intended to pass ignore_status and print_info parameters here. > + > + > +def > virsh_vcpucount_live(domname,uri="",ignore_status=False,print_info=False): > + > + """ > + Prints the vcpucount of a given domain. > + """ > + cmd_vcpucount = "vcpucount --live --active %s" % domname > + return virsh_cmd(cmd_vcpucount,uri).stdout.strip() ignore_status and print_info ? > + > + > def virsh_freecell(uri = "", ignore_status=False, extra = ""): > """ > Prints the available amount of memory on the machine or within a NUMA > cell. > diff --git a/client/virt/subtests.cfg.sample b/client/virt/subtests.cfg.sample > index 785077b..320a577 100644 > --- a/client/virt/subtests.cfg.sample > +++ b/client/virt/subtests.cfg.sample > @@ -212,6 +212,9 @@ variants: > status_error = "yes" > libvirtd = "off" > > + - virsh_vcpupin: install setup image_copy unattended_install.cdrom > + type = virsh_vcpupin > + > - virsh_version: install setup image_copy unattended_install.cdrom > type = virsh_version > vms = '' > -- 1.7.5.4 _______________________________________________ Autotest > mailing list Autotest@test.kernel.org > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest > -- Chris Evich, RHCA, RHCE, RHCDS, RHCSS Quality Assurance Engineer e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214 _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
