New test cases for set CHS geometry for disk types:scsi, sata, virtio… #4207
Conversation
|
PASS 01-type_specific.io-github-autotest-libvirt.virtual_disks.geometry.coldplug.chs_scsi.positive_test |
chunfuwen
force-pushed
the
add_chs_geometry_cases
branch
3 times, most recently
from
2aad519
to
86b2c37
Compare
|
IDE controller is not supported on q35 machine type and afterward, so this automation doesn't cover ide bus type |
chunfuwen
force-pushed
the
add_chs_geometry_cases
branch
6 times, most recently
from
2b978ed
to
efc9402
Compare
|
@dzhengfy |
|
The geometry only includes the attributes cyls, heads, secs, trans literally. It is improper to put the tests of discard and iotune in the test set geometry. |
|
How about changing the name from geometry to metrics(virtual_disks.metrics or virtual_disk.telemetry)? Here we want to aggregate some couples and cohesive attributes into one cfg file, rather than maintain cyls, heads, secs, trans as independent cfg file. As you may know that, in remaining virtual disk plan, almost each of them comes from one bugzilla bug, we can not create one cfg file for one bugzilla,that will leads to huge maintainer efforts. Here we try to seek out some common points, which we think they can be aggregated into. |
I think virtual_disks.metrics is not a good name because it is so broad that could include all the sub-elements and attributes of disk. Following the scheme of disk XML is more reasonable, and it will be easier to debug. For example:
|
| backend_device = "chs_virtio" | ||
| chs_attrs = "{'cyls': '16383', 'heads': '16', 'secs': '63'}" | ||
| only coldplug..positive_test | ||
| - chs_sata: |
There was a problem hiding this comment.
Please remember to skip this sata cases on other arches in CI because other aarches only support sata in libvirt/qemu upstream.
|
|
||
| def check_chs_values(params, vm_name, test): | ||
| """ | ||
| Check cylinders, heads, sectors value from qemu line since there are on other ways to check this. |
There was a problem hiding this comment.
on other ways tono other ways
|
|
||
| def check_discard_detect_zeroes_values(params, vm_name, test): | ||
| """ | ||
| Check detect_zeroes and discard value from qemu line since there are on other ways to check this. |
| """ | ||
| chs_geometry_attrs_dict = eval(params.get('chs_attrs')) | ||
| cmd = ("ps -ef | grep %s | grep -v grep " % vm_name) | ||
| LOG.debug("VM cmdline: %s", process.system_output(cmd, shell=True)) |
There was a problem hiding this comment.
process.system_output(verbose=True) is enabled as default. Do we still need to log the output again?
There was a problem hiding this comment.
updated with using process.system_output(verbose=True) only
| :param test: test assert object | ||
| """ | ||
| cmd = ("ps -ef | grep %s | grep -v grep " % vm_name) | ||
| LOG.debug("VM cmdline: %s", process.system_output(cmd, shell=True)) |
There was a problem hiding this comment.
process.system_output(verbose=True) is enabled as default. Do we still need to log the output again?
There was a problem hiding this comment.
remove LOG, and add verbose=True when call process.system_output
| if detect_zeroes: | ||
| str_to_grep = "detect-zeroes.*%s.*" % (detect_zeroes) | ||
| if not libvirt.check_logfile(str_to_grep, log_file): | ||
| test.fail('Failed to check detect-zeroes as expected') |
There was a problem hiding this comment.
Better to include the str_to_grep in the message for debugging
| if discard: | ||
| str_to_grep = "discard.*%s.*" % (discard) | ||
| if not libvirt.check_logfile(str_to_grep, log_file): | ||
| test.fail('Failed to check discard as expected') |
There was a problem hiding this comment.
Better to include the str_to_grep in the message for debugging
| test.fail('Failed to check discard as expected') | ||
|
|
||
|
|
||
| def translate_blkiotune_raw_output_into_dict(output, default_delimiter="="): |
There was a problem hiding this comment.
The function name includes blkiotune, but the docstring doesn't. And only virsh.blkdeviotune output is passed into this function. So I am confusing the usage of blkiotune in the function name. We should make them consistent anyway.
There was a problem hiding this comment.
This function's purpose is to convert the output to a dict. So could you please have a look at
libvirt_misc.convert_to_dict(). Some python files also use it, like in virsh_emulatorpin_mix.py . I am feeling it could be helpful for you.
There was a problem hiding this comment.
remove this function, and use libvirt_misc.convert_to_dict() instead
| if hotplug: | ||
| LOG.info("attaching devices, expecting error...") | ||
| result = virsh.attach_device(vm_name, device_obj.xml, debug=True) | ||
| libvirt.check_result(result, expected_fails=[]) |
There was a problem hiding this comment.
I do not see this is expecting error. If we only need to check the command status, libvirt.check_exit_status(result, expect_error=False) is enough.
@qiankehan Telemetry means that data captured of states, events, performance in computing, storage and networking in the system. |
Yeah, openshift and OpenStack have clear definition about the concept of Telemetry. I disagree with such abstract names like metrix or telemetry until its scope is clear and documented. Otherwise, how to know if an element or an attribute is a part of this concept? And it will make no sense. Maybe we can draft the documents like openshift or OpenStack in tp-libvirt and then add the cases with such a name. |
…for disk types:scsi, sata, virtio, usb Set detect_zeroes attribute Set read_bytes_sec/write_bytes_sec/total_bytes_sec together Set max total_bytes_sec Signed-off-by: chunfuwen <[email protected]>
chunfuwen
force-pushed
the
add_chs_geometry_cases
branch
from
efc9402
to
6e4afc7
Compare
@qiankehan |
|
After log updated, new test result: PASS 01-type_specific.io-github-autotest-libvirt.virtual_disks.geometry.coldplug.chs_scsi.positive_test |
|
Let' merge it firstly, and after #4376 has conclusion, then we can go back to refactor code if needed |
New test cases for geometry: set cyls, heads, secs, trans attributes for disk types:scsi, sata, virtio, usb
set detect_zeroes attribute
set read_bytes_sec/write_bytes_sec/total_bytes_sec together
set max total_bytes_sec
Signed-off-by: chunfuwen [email protected]