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.
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
