On 09/17/12 10:29am, Chris Evich wrote:
>
> Hey,
>
> This is looking good. A few minor notes that will save you some
> effort in the future:
>
> + There's some 'funny' 6-space indentation in this module. You can
> use autotest/utils/reindend.py to fix this automatically.
>
> + We ship a wrapper for pylint in autotest/utils/run_pylint.py which
> can help find problems automatically. It's very verbose by default,
> but does find stuff our eyeballs miss sometimes. For example, here
> are some things that should be investigated:
>
> # ../utils/run_pylint.py tests/virt/libvirt/tests/virsh_cpu_stats.py
> ...
> W0311:317: Bad indentation. Found 8 spaces, expected 12
> ...
> W0613: 89:run_virsh_cpu_stats.getlist_expected_cpus: Unused argument
> 'actual_cmd_stdout'
> ...
> W0622:100:run_virsh_cpu_stats.get_cpu_time: Redefining built-in 'type'
> ...
> W0613:118:run_virsh_cpu_stats.get_cgroup_cputime: Unused argument
> 'actual_cmd_stdout'
> ...
> W0101:165:run_virsh_cpu_stats.compute_and_test_results: Unreachable code
> ...
> W0101:192:run_virsh_cpu_stats.test_total_cpustats: Unreachable code
> ...
> E9900:305:run_virsh_cpu_stats: Unsupported format character "'"
> (0x27) at index 26
>
> On 09/13/2012 01:38 PM, Prem Karat wrote:
> >Add virsh_cpu_stats.py to test virsh cpu-stats command
> >
> > Test the virsh cpu-stats command
> >
> > (1) Invoke virsh cpu-stats<domain> <options>
> > (2) Invoke virsh cpu-stats with following possible combination of
> > options
> > a. None e. --start --count
> > b. --start f. --start --total
> > c. --count g. --count --total
> > d. --total h. --start --count --total
> > (3) Invoke virsh cpu-stats with a numeric argument
> > (4) Invoke virsh cpu-stats with an invalid string "xyz"
> > (5) Invoke virsh cpu-stats with above combinations with libvitrd service
> >stop
> >
> >Pass Condition:
> >==============
> >1. When option = 'total'
> > a. Displayed Total cpu_time> User cpu_time> System cpu_time
> > b. Displayed Total cpu_time>= User + System cpu_time
> >2. When option = 'start' or 'count'
> > a. Displayed CPU's == Expected CPU's
> > b. Cgroup cpu_time*>= sum of cpu_time of each cpu's displayed
> >3. When option has a combination of 'start', 'count', 'total'
> > All the above conditions 1& 2 are checked.
> >4. When option is None
> > a. Displayed Total cpu_time>= sum of cpu_time of all cpu's displayed
> > b. Pass condition when option = 'total' (1) is checked.
> >
> >*Cgroup cpu_time is computed by reading domain specific cpuacct.usage_percpu
> >Delay in reading the percpu file will lead up to cgropu cpu_time being
> >slightly
> >higher (in ns) than the displayed time.
> >
>
> Nice detailed commit message, well done.
>
> >Signed-off-by: Prem Karat<[email protected]>
> >---
> > client/tests/virt/libvirt/tests/virsh_cpu_stats.py | 318
> > ++++++++++++++++++++
> > 1 file changed, 318 insertions(+)
> > create mode 100644 client/tests/virt/libvirt/tests/virsh_cpu_stats.py
> >
> >diff --git a/client/tests/virt/libvirt/tests/virsh_cpu_stats.py
> >b/client/tests/virt/libvirt/tests/virsh_cpu_stats.py
> >new file mode 100644
> >index 0000000..5ed3a5d
> >--- /dev/null
> >+++ b/client/tests/virt/libvirt/tests/virsh_cpu_stats.py
> >@@ -0,0 +1,318 @@
> >+import re, logging
> >+from autotest.client.shared import utils, error
> >+from virttest import libvirt_vm, virsh, utils_test
> >+from autotest.client import utils
> >+
> >+def run_virsh_cpu_stats(test, params, env):
> >+ """
> >+ Test the virsh cpu-stats command
> >+
> >+ (1) Invoke virsh cpu-stats<domain> <options>
> >+ (2) Invoke virsh cpu-stats with following possible combination of
> >options
> >+ a. None e. --start --count
> >+ b. --start f. --start --total
> >+ c. --count g. --count --total
> >+ d. --total h. --start --count --total
> >+ (3) Invoke virsh cpu-stats with a numeric argument
> >+ (4) Invoke virsh cpu-stats with an invalid string "xyz"
> >+ (5) Invoke virsh cpu-stats with above combinations libvitrd service stop
> >+ """
>
> Non-critical Suggestion: This can be less-verbose. Since
> params/options are likely to change over time, no need to specify
> them. You could just say "Invoke virsh cpu-stats with options"
>
> >+
> >+ # Prepare libvirtd service
> >+ libvirtd = params.get("libvirtd")
> >+ if libvirtd == "off":
> >+ libvirt_vm.service_libvirtd_control("stop")
> >+ elif not libvirt_vm.service_libvirtd_control("status"):
> >+ libvirt_vm.service_libvirtd_control("start")
> >+
> >+ # Get the vm name and check if its active. Run the test only if its
> >active
> >+ vm_name = params.get("main_vm")
> >+ if libvirtd == "on":
> >+ state = virsh.domstate(vm_name, ignore_status=True)
> >+ if state not in ['running', 'paused', 'idle']:
>
> Why not
>
> vm = env.get_vm(params[vm_name])
> vm.verify_alive()
>
> ?
>
> >+ raise error.TestFail("Cannot Execute Test. "
> >+ "%s domain not active" % vm_name)
>
>
> >+
> >+
> >+ # Get the actual output by running cpu-stats command
> >+ def run_cpu_stat_cmd(parameters):
> >+ cmd_result = virsh.cpu_stats(vm_name, extra=parameters, \
> >+ ignore_status=True, uri="")
>
> No need to specify an empty URI here since that's equivalent to the default.
>
> >+ logging.info("Command Output:\n%s", cmd_result.stdout.strip())
> >+ logging.info("Command Exit Status: %d", cmd_result.exit_status)
> >+ logging.error("Command Error:\n\t %s", cmd_result.stderr.strip())
>
> Calls to this wrapper function can just be replaced with:
>
> virsh.cpu_stats(vm_name, extra, debug=True)
>
> I think we can stick the ignore_status=True into the virsh function
> directly since that's probably how it will be called most of the
> time.
>
> >+ return cmd_result
> >+
> >+
> >+ # Compute start and end values for options
> >+ # Build the command output with final options. Pass appropriate
> >+ # param values
> >+ def build_output(option):
> >+ possible_options = ['--start', '--count', '--start --count', \
> >+ '--start --total', '--count --total', \
> >+ '--start --count --total']
> >+ if option in possible_options:
> >+ start_value = utils.count_cpus()/2
>
> Why is this /2? Could use a comment here if it's necessary.
>
> >+ count_value = start_value - 1
> >+ else:
> >+ count_value = utils.count_cpus()/2 - 1
> >+ if option == "--start":
> >+ option = option + " " + str(start_value)
> >+ return run_cpu_stat_cmd(option)
> >+ elif option == "--count":
> >+ option = option + " " + str(count_value)
> >+ return run_cpu_stat_cmd(option)
> >+ elif option == "--start --count":
> >+ option = "--start " + str(start_value) + " --count " \
> >+ + str(count_value)
> >+ return run_cpu_stat_cmd(option)
> >+ elif option == "--start --total":
> >+ option = "--start " + str(start_value) + " --total"
> >+ return run_cpu_stat_cmd(option)
> >+ elif option == "--count --total":
> >+ option = "--count " + str(count_value) + " --total"
> >+ return run_cpu_stat_cmd(option)
> >+ elif option == "--start --count --total":
> >+ option = "--start " + str(start_value) + " --count " \
> >+ + str(count_value) + " --total"
> >+ return run_cpu_stat_cmd(option)
> >+ else:
> >+ return run_cpu_stat_cmd(option)
> >+
> >+
> >+ # Generate a list of displayed cpus in cmd stdout
> >+ def getlist_displayed_cpus(actual_cmd_stdout):
> >+ return re.findall(r"(CPU\d+):", actual_cmd_stdout)
>
> Since this one-line function is only called once, consider just
> putting it inline with the code.
>
> >+
> >+
> >+ # Generate a list of expected cpus from start_cpu to end_cpu
>
> These descriptive comments for functions can be put into docstrings,
> since that's what they're intended for.
>
> >+ def getlist_expected_cpus(actual_cmd_stdout, start_cpu, end_cpu):
> >+ expected_cpus = []
> >+ for index in range(start_cpu, end_cpu):
> >+ cpu = "CPU" + str(index)
> >+ expected_cpus.append(cpu)
> >+ return expected_cpus
> >+
> >+
> >+ # compute cpu time based on the type of cpu_time requested
> >+ # Type = Total or sum of each cpus. If Total cpu time is
> >+ # requested compute user& system time
> >+ def get_cpu_time(type, actual_cmd_stdout, condition):
> >+ cpu_time = re.findall(r"%s\s*cpu_time\s*(\d*.\d*)\sseconds"
> >%condition,
> >+
> >actual_cmd_stdout)
> >+ if type == "Total":
> >+ user_time = re.findall(r"\s*user_time\s*(\d*.\d*)\sseconds", \
> >+
> >actual_cmd_stdout)
> >+ system_time = re.findall(r"\s*system_time\s*(\d*.\d*)\sseconds", \
> >+
> >actual_cmd_stdout)
> >+ return cpu_time + user_time + system_time
> >+ elif type == "sum_of_each":
> >+ cmdout_cpu_time = 0.0
> >+ for time in cpu_time:
> >+ cmdout_cpu_time += float(time)
> >+ return cmdout_cpu_time
> >+
> >+
> >+ # compute the cpu_time of start_cpu to end_cpu by reading the cgroup
> >+ # cpuacct.usage_percpu file from cgroup cpuacct controller mount point
> >+ def get_cgroup_cputime(actual_cmd_stdout, start_cpu, end_cpu):
> >+ cgrp_cpu_time = 0.0
> >+ # Generate a list of cpu_time of corresponding cpus from cgroup
> >file
> >+ cpuacct_percpu = utils_test.domstat_cgroup_cpuacct_percpu(vm_name)
> >+ # Compute the sum of cpu_time of correspondig cpus from cgroup file
> >+ for cpu_time in cpuacct_percpu[start_cpu: end_cpu ]:
> >+ cgrp_cpu_time += float(float(cpu_time)/float(1000000000))
> >+ return cgrp_cpu_time
> >+
> >+
> >+ # Extract the displayed cpu's in cmd stdout
> >+ # Build the expected cpus list using start_cpu and end_cpu param
> >+ # Extract and compute the sum of cpu_time of each cpus
> >+ # Extract and compute the sum of corresponding cpus from cgroup cpuacct
> >+ # controller file (cpuacct.usage_percpu)
> >+ # Displayed cpus should be equal to expected cpus
> >+ # cgrop cpu time>= displayed cpu time (the value could increase due the
> >+ # delay in reading the file
> >+ def compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu):
> >+ # Gather the actual and expected output
> >+ displayed_cpus = getlist_displayed_cpus(actual_cmd_stdout)
> >+ expected_cpus = getlist_expected_cpus(actual_cmd_stdout, \
> >+ start_cpu,
> >end_cpu)
> >+ cmdout_cpu_time = get_cpu_time("sum_of_each", actual_cmd_stdout, \
> >+ "(?<=CPU)\d*:\n")
> >+ cgrp_cpu_time = get_cgroup_cputime(actual_cmd_stdout, \
> >+ start_cpu,
> >end_cpu)
> >+ logging.info("\n\t\tPass condition:" \
> >+ "\n\t\t\tDisplayed cpus == Expected cpus" \
> >+ "\n\t\t\tCgroup cpu_time>= Displayed cpu_time")
> >+ # compare the actual and expected output
> >+ if expected_cpus == displayed_cpus and \
> >+ cgrp_cpu_time>= cmdout_cpu_time:
> >+ logging.info("\n\t\t\tDisplayed cpus = %s" \
> >+ "\n\t\t\tExpected cpus = %s" \
> >+ "\n\t\t\tCgroup cpu_time = %s" \
> >+ "\n\t\t\tDisplayed cpu_time = %s" \
> >+ %(displayed_cpus, expected_cpus, cgrp_cpu_time, \
> >+ cmdout_cpu_time))
> >+ return True
> >+ else:
> >+ raise error.TestFail("\n\t\t\tDisplayed cpus = %s" \
> >+ "\n\t\t\tExpected cpus = %s" \
> >+ "\n\t\t\tCgroup cpu_time = %s\n" \
> >+ "\n\t\t\tDisplayed cpu_time = %s" \
> >+ %(displayed_cpus, expected_cpus, cgrp_cpu_time, \
> >+ cmdout_cpu_time))
> >+ return False
> >+
> >+
> >+ # The Total cpu time should be always greater than the sum of User time
> >+ # (time spend in user mode) and system time (kernel mode). The total cpu
> >+ # time will be a sum of user + system + steal time + irq etc..
> >+ # The user time will always be more than the system time.
> >+ def test_total_cpustats(cpu_stat):
> >+ logging.info("\n\t\tPass Condition:" \
> >+ "\n\t\t\tTotal cpu_time> User cpu_time> " \
> >+ "System cpu_time" \
> >+ "\n\t\t\tTotal cpu_time> User cpu_time + " \
> >+ "System cpu_time")
> >+ if float(cpu_stat[0])> float(cpu_stat[1]) and \
> >+ float(cpu_stat[0])> float(cpu_stat[2]) and \
> >+ float(cpu_stat[1])> float(cpu_stat[2]) and \
> >+ float(cpu_stat[0])>= (float(cpu_stat[1]) + float(cpu_stat[2])):
> >+ logging.info("\n\t\t\tTotal cpu_time = %s" \
> >+ "\n\t\t\tUser cpu_time = %s" \
> >+ "\n\t\t\tSystem cpu_time = %s" \
> >+ %(cpu_stat[0], cpu_stat[1], cpu_stat[2]))
> >+ return True
> >+ else:
> >+ raise error.TestFail("\n\t\t\tTotal cpu_time = %s" \
> >+ "\n\t\t\tUser cpu_time = %s" \
> >+ "\n\t\t\tSystem cpu_time = %s" \
> >+ %(cpu_stat[0], cpu_stat[1], cpu_stat[2]))
>
> Why not save the result of the logging/exception string to a
> variable, then reference it to logging.info() and exception? Since
> the format is fairly complex, this will make maintaining it easier.
>
> >+ return False
> >+
> >+ def validate_output(actual_cmd_stdout, option):
> >+ option_has_total = ['', '--total', '--start --total', '--count
> >--total',
> >+ '--start --count
> >--total']
> >+ if option in option_has_total:
> >+ cpu_stat = get_cpu_time("Total", actual_cmd_stdout, "(?<=Total:)")
> >+
> >+ # If option == null, total_precpu_time = sum of cpu_time of all cpus
> >+ # (count_cpus, i.e CPU0-N) in cmd_stdout. For test to pass,
> >+ # Total cpu_time (displayed in stdout)>= total_percpu_time
> >+ # Tota cpu_time> user_time> system_time
> >+ # Total_cpu_time>= user_time + system_time.
> >+ # user_time> system_time. Else o/p is invalid
> >+
> >+ if option == "":
> >+ total_percpu_time = get_cpu_time("sum_of_each", actual_cmd_stdout, \
> >+ "(?<=CPU)\d*:\n")
> >+ if float(cpu_stat[0])>= total_percpu_time and \
> >+ test_total_cpustats(cpu_stat):
> >+ logging.info( "Total cpu_time>= Sum of each cpu_time")
> >+ logging.info("\n\t\t\tTotal cpu_time = %s" \
> >+ "\n\t\t\tSum of each cpu_time = %s" \
> >+ %(cpu_stat[0], total_percpu_time))
> >+ return True
> >+ else:
> >+ raise error.TestFail("\n\t\t\tTotal cpu_time = %s" \
> >+ "\n\t\t\tUser cpu_time = %s" \
> >+ "\n\t\t\tSystem cpu_time = %s" \
> >+ "\n\t\t\tSum of cpu_time of individual cpus = %s" \
> >+ %(cpu_stat[0], cpu_stat[1], cpu_stat[2], total_percpu_time))
> >+
> >+ # if option == start || count, then test if expected cpus are
> >+ # displayed in command stdout.
> >+ # compute the sum of cpu_time of displayed cpus in stdout and compare
> >+ # the value with sum of cpu_time computed from cgroup
> >+ # cpuacct.usage_percpu file for the corresponding cpus
> >+
> >+ elif option == "--start":
> >+ start_cpu = utils.count_cpus()/2
> >+ if start_cpu< 0:
> >+ start_cpu = 0
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+
> >+ elif option == "--count":
> >+ start_cpu = 0
> >+ end_cpu = utils.count_cpus()/2 - 1
> >+ if end_cpu< 0:
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+
> >+ # if option == total, for test to pass, values from stdout should be
> >+ # Tota cpu_time> user_time> system_time
> >+ # Total_cpu_time>= user_time + system_time.
> >+ # user_time> system_time. Else o/p is invalid
> >+
> >+ elif option == "--total":
> >+ test_total_cpustats(cpu_stat)
> >+
> >+ elif option == "--start --count":
> >+ start_cpu = utils.count_cpus()/2
> >+ if start_cpu< 0:
> >+ start_cpu = 0
> >+ end_cpu = start_cpu + start_cpu - 1
> >+ if end_cpu< 0:
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+
> >+ elif option == "--start --total":
> >+ start_cpu = utils.count_cpus()/2
> >+ if start_cpu< 0:
> >+ start_cpu = 0
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+ test_total_cpustats(cpu_stat)
> >+
> >+ elif option == "--count --total":
> >+ start_cpu = 0
> >+ end_cpu = utils.count_cpus()/2 - 1
> >+ if end_cpu< 0:
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+ test_total_cpustats(cpu_stat)
> >+
> >+ elif option == "--start --count --total":
> >+ start_cpu = utils.count_cpus()/2
> >+ if start_cpu< 0:
> >+ start_cpu = 0
> >+ end_cpu = start_cpu + start_cpu - 1
> >+ if end_cpu< 0:
> >+ end_cpu = len(utils_test.domstat_cgroup_cpuacct_percpu(vm_name))
> >+ compute_and_test_results(actual_cmd_stdout, start_cpu, end_cpu)
> >+ test_total_cpustats(cpu_stat)
> >+
> >+ return True
> >+
> >+ # Run the test cases based on the appropriate cmd options.
> >+ option = params.get("virsh_cpu_stats_options")
> >+
> >+ actual_cmd_output = build_output(option)
> >+ actual_cmd_stdout = actual_cmd_output.stdout.strip()
> >+ actual_status = actual_cmd_output.exit_status
> >+
> >+ # Recover libvirtd Service which will be turned off by error check code.
> >+ if libvirtd == "off":
> >+ libvirt_vm.service_libvirtd_control("start")
> >+
> >+ # Check status_error
> >+ status_error = params.get("status_error")
> >+ if status_error == "yes":
> >+ if actual_status == 0:
> >+ if libvirtd == "off":
> >+ raise error.TestFail("Command 'virsh cpu-stats %' succeeded "
> >+ "with libvirtd service stopped, "
> >+ "incorrect" % option)
> >+ else:
> >+ raise error.TestFail("Command 'virsh cpu-stats %s' succeeded"
> >+ " (incorrect command)" % option)
> >+ else:
> >+ if actual_status != 0:
> >+ raise error.TestFail("Command 'virsh cpu-stats %s' failed "
> >+ "(correct command)" % option)
> >+ else:
> >+ validate_output(actual_cmd_stdout, option)
> >+ return True
> >+
>
> Otherwise this is looking good. Please add this test to the
> tracking issue here: https://github.com/autotest/autotest/issues/497
> this way someone else won't duplicate your work. Thanks.
>
Thank You for reviewing. I have made the necessary changes and testing is
underway. I'll send you the v2 asap.
I have raised the topic/issues in github for this and things am working on.
--
-prem
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
