+def get_value(server, cmd, log_msg, fieldname):
I wouldn't make a separate function out of this. it probably won't be used except by the eafp_dpool_cap_reserve_val() function.
+ msg = log_msg % fieldname + ret, value = run_remote(server, cmd) + if ret != 0: + logger.error("%s", log_msg, fieldname) + return FAIL, value + return PASS, value + +def eafp_dpool_cap_reserve_val(server, virt, poolname):
The capabilities and reserved values are properties of the ResourcePools - I'd drop eafp from the name since these values aren't related to the EAFP association. Also, I'd move these functions to the logicaldevices.py file. Or, you could create a resourcepool.py file and add the verify_common_pool_values() function to it. The common_util.py is becoming quite large - so, it might be a good idea to try to place more specific functions like these (that deal with a specific provider's values) into a file specific for that provider.
+ libvirt_version = virsh_version(server, virt) + capacity = reserved = None + if libvirt_version >= '0.4.1': + # get the value from pool-info + log_msg= "Failed to get the '%s' info from pool-info" + dp_name, pname = poolname.split("/") + + cmd = "virsh pool-info %s | awk '/Capacity/ { print \$2}'" \ + % pname + status, cap_val = get_value(server, cmd, log_msg, 'Capacity') + if status != PASS: + return FAIL, capacity, reserved + cap_val = float(cap_val) + capacity = int(cap_val * 1024 * 1024 * 1024) >> 20
This calculation is odd. You're converting GB into bytes and then from bytes into megabytes. You can instead convert straight to megabytes: capacity = int(cap_val * 1024)
+ + cmd = "virsh pool-info %s | awk '/Allocation/ { print \$2}'" \ + % pname + status, alloc_val = get_value(server, cmd, log_msg, 'Allocation') + if status != PASS: + return FAIL, capacity, reserved + alloc_val = float(alloc_val) + reserved = int(alloc_val * 1024 * 1024 *1024) >> 20
Same here - convert straight to megabytes
+ + else: + # get info from stat -filesystem + log_msg = "Stat on the '%s' file failed" + + cmd = "stat -f %s | awk '/size/ {print \$7}'" % disk_file + status, f_bsize = get_value(server, cmd, log_msg, disk_file) + if status != PASS: + return FAIL, capacity, reserved + + cmd = " stat -f %s | awk '/Blocks/ {print \$3}'" % disk_file + status, b_total = get_value(server, cmd, log_msg, disk_file) + if status != PASS: + return FAIL, capacity, reserved + cap_val = (int(f_bsize) * int(b_total)) + capacity = (int(f_bsize) * int(b_total)) >> 20 + + cmd = "stat -f %s | awk '/Blocks/ {print \$5}'" % disk_file + status, b_free = get_value(server, cmd, log_msg, disk_file) + if status != PASS: + return FAIL, capacity, reserved + reserved = (cap_val - (int(f_bsize) * int(b_free))) >> 20 + + return PASS, capacity, reserved
If you want to break up the size of this function some, you could do something like: dpool_cap_reserve_val(): libvirt_version = virsh_version(server, virt) capacity = reserved = None if libvirt_version >= '0.4.1': libvirt_dpool_cap_res() else: fs_dpool_cap_res() Having sub functions for each of these breaks up the work nicely, and it's possible (although unlikely) that a test would need to call one of these functions explicitly. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com