Hi, Thanks a lot for your comments, working on them, I have few comments below inline.
On Wed, 2012-07-25 at 11:32 -0400, Chris Evich wrote: > 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, [email protected] wrote: > > From: Satheesh Rajendran<[email protected]> > > > > 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<[email protected]> > > --- > > 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. > sure, Thanks, would take care of this comment in general. > > + 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. Sure, would do that. I would move this qemu-monitor-command to client/virt/libvirt_vm.py and would implement like vm.get_vcpu_pid() to get the pids of vcpu I guess that should be helpful. > > > + > > + 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? > Yes this can be placed in client/shared/base_utils.py would place it. > > + > > + 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. > Thanks. > > + > > + # 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. > Yes,I would place this in client/shared/base_utils.py > > + > > + # 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. > Thanks, would add virsh_vcpupin to VM class and can be used like vm.vcpupin(vcpu,cpu) > > 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. > In general, would take care this comment. > > + 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. > missed, would do it. > > + > > + > > +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 ? > same. > > + > > + > > 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 [email protected] > > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest > > > > _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
