Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Libvirt: to test virsh nodecpustats #524

Closed
sathnaga opened this Issue · 12 comments

3 participants

@sathnaga
Collaborator

(1) Call the virsh nodecpustats command for all cpu host cpus separately
(2) Get the output
(3) Check the against /proc/stats output(o) for respective cpu
user: o[0] + o[1]
system: o[2] + o[5] + o[6]
idle: o[3]
iowait: o4 Call the virsh nodecpustats command with an unexpected option
(5) Call the virsh nodecpustats command with libvirtd service stop

@sathnaga
Collaborator

Adding to tracking issue #497

@sathnaga sathnaga was assigned
@cevich
Owner

FYI: I'll be reviewing this within in the next few days.

@chuanchang
Collaborator

@sathnaga, I simply go through your codes, it's a right way to check /proc/stat then compare corresponding items with virsh nodecpustats, and I saw you gave a ‘delta' value to count deviation range, and user can modify it based on actual cpu load, so it's also fine for me. for more, I think @cevich will help review them.

In addition, it's a common function, to consider it should be useful for others, so we had better to wrapper a function to parse '/proc/stat' then share it under the client/shared or something like that.

Finally, it should be a typo in comment of the virsh_check_nodecpustats(), s/\/proc\/stats/\/proc\/stat/.

@cevich
Owner

Okay, here's a thorough review:

import logging
from autotest.client.shared import utils, error
from autotest.client.virt import libvirt_vm
from autotest.client import *

Last import line has to go.

def get_cpu_stat(key):
...

Alex's advice applies here as well. This seems like a generally useful function, please feel free to consider adding it to a module under client/shared/ so other tests can use it.

def virsh_check_nodecpustats(actual_stats, expected_stats, delta):
...

I think testing the delta is a workable approach for memory, but I'm not sure it's a good idea for CPU. IIRC, those values can wrap around, but even so, It can take a LOT of effort to quiet CPUs down for testing like this. Consider things like clock and I/O IRQ handlers are using CPU, along with whatever misc. service decides to run during the test. So, in the practical sense, I don't think there's much value in comparing the delta's here. Plus, in all likelihood virsh is just obtaining the data from proc anyway.

I think it's good enough to just test that the output can be parsed, and do some very simplistic checks. For example, make sure idle > user > system > 0. Total is smaller than system uptime. Maybe check the --percent output as well, and make sure they all add up to 100% * no. of CPUs. Things like that will generate far fewer false-failures and be easier to maintain. What do you think?

name_stats = ['user', 'system', 'idle', 'io_wait']
...
def virsh_check_nodecpustats(actual_stats, expected_stats, delta):
...

Same advice here as in nodememstats. Using a dictionary and regex's are easier to read and maintain. They also are easier to deal with if/when the output format changes.

        if status == 0:
            if libvirtd == "off":
                raise error.TestFail("Command 'virsh nodecpustats' succeeded "
                                 "with libvirtd service stopped, incorrect")
            else:
                raise error.TestFail("Command 'virsh nodecpustats %s' succeeded"
                                 "(incorrect command)" % option)

An attempt to recover the libvirtd service should be made here.

                ex_user = int(expected_stats[1]) / 10000000
                ex_system = int(expected_stats[3]) / 10000000
                ex_idle = int(expected_stats[5]) / 10000000
                ex_iowait = int(expected_stats[7]) / 10000000

Could use a comment here on why divide by 10000000 here. Also, since you're doing it for every number, how about just put that math into get_cpu_stat() ?

            else:
                raise error.TestFail("Command 'virsh nodecpustats %s' not succeeded" % option)

Need libvirtd service recovery here.

            option = "--cpu %d" % cpu
            output = libvirt_vm.virsh_nodecpustats(ignore_status=True, extra=option)
            expected_stats = output.stdout.strip().split()
            status = output.exit_status

            if status == 0:
                ex_user = int(expected_stats[1]) / 10000000
                ex_system = int(expected_stats[3]) / 10000000
                ...
        ...
        option = params.get("virsh_cpunodestats_options")
        output = libvirt_vm.virsh_nodecpustats(ignore_status=True, extra=option)
        expected_stats = output.stdout.strip().split()
        status = output.exit_status

        if status == 0:
            ex_user = int(expected_stats[1]) / 10000000
            ex_system = int(expected_stats[3]) / 10000000
            ...

Quite a bit of code duplication here with only small differences. Consider using a function for these blocks with parameters & conditions to handle the different use-cases.

    for i in range(itr):
        logging.info("iteration %d", i+1)
        logging.info("cpu %s", name_stats)
        temp = 0
        for cpu in range(host_cpu_count + 1):
            logging.info("%s: %s", delta_all[i][temp], delta_all[i][temp+1])
            temp = (cpu+1) * 2

Same advice here as nodememstats. This info is useful when the test fails, otherwise consider making it only debug level.

    variants:
        - no_option:
            virsh_cpunodestats_options = ""
            status_error = "no"nanoseconds.
            libvirtd = "on"
        - unexpect_option:
            virsh_cpunodestats_options = "xyz"
            status_error = "yes"
            libvirtd = "on"
        - with_libvirtd_stop:
            virsh_cpunodestats_options = ""
            status_error = "yes"
            libvirtd = "off"

It'd be good to see a variant for the '--percent' parameter, or make this part of the test module as suggested above.

Overall, it's a nice straight-forward test and the code looks good. Polish it up as best as you can, and I'm looking forward to the next version. Thanks!

@sathnaga
Collaborator
@sathnaga
Collaborator
@cevich
Owner

Satheesh,

I appreciate you're continuing to work on this.

FYI- Lucas has just posted a schedule for the upcoming changes to the project. Right now, we're only taking bigfixes for the framework/tests. Assuming all goes well, October first is the soonest we can add new code.

I'll try to give this a review before then so we can keep moving forward despite the freeze. Thanks again.

@sathnaga
Collaborator
@cevich
Owner

This is on it's way to being committed to next shortly.

c2b8f30
autotest/virt-test@645515c
autotest/virt-test@de9007a
autotest/virt-test@51ca394

@cevich cevich closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.