Automate blockresize cases #5585
Conversation
|
PASS 01-type_specific.io-github-autotest-libvirt.virtual_disks.blockresize.coldplug.virtio.capacity |
chunfuwen
force-pushed
the
automate_blockresize_case
branch
from
290c67c
to
da35048
Compare
| "Error: %s" % str(details)) | ||
| else: | ||
| session = vm.wait_for_login() | ||
| time.sleep(20) |
There was a problem hiding this comment.
Is it necessary to wait here , or could we use utils_misc.wait_for(
There was a problem hiding this comment.
The wait is to allow VM to show up disk ,and following call new_disk, _ = libvirt_disk.get_non_root_disk_name(session) can succeed.
So here it is for guest, and hard to find matched pattern for utils_misc.wait_for().
| def check_image_virtual_size(image_file, matched_size, test): | ||
| """ | ||
| Check whether image virtual size queried by qemu-img is matched | ||
| :param image_file: image file path | ||
| :param matched_size: anticipated size | ||
| :param test: test case itself | ||
|
|
||
| """ | ||
| disk_info_dict = utils_misc.get_image_info(image_file) | ||
| actual_size_in_bytes = str(disk_info_dict.get("vsize")) | ||
| if actual_size_in_bytes != matched_size: | ||
| test.fail(f"actual size of block disk is {actual_size_in_bytes}, but expected is : {matched_size}") |
There was a problem hiding this comment.
You could reuse check_obj.check_image_info() if you want
There was a problem hiding this comment.
This is only one line code, looks like it is not necessary introduce another dependence: check_obj.check_image_info()
| target_dev = params.get('target_dev') | ||
| status_error = "yes" == params.get("status_error") | ||
| try: | ||
| device_obj = create_customized_disk(params, test) |
There was a problem hiding this comment.
You could reuse setup_attrs if you want
There was a problem hiding this comment.
This issue has been discussed multiple round before. Whether use setup_attrs or previously base utils. Setup_attrs need input lots of long and nested config information as raw in cfg file, sometimes it look not easily to understand clearly. Base utils build up from some disperse parameter, and those parameter can share across variants in cfg file. Finally, I would like to say it is real balance here which one your prefer. I would like author choose his or her preferable ways.
There was a problem hiding this comment.
OK, I'm fine with it. I was recently requested to use a dictionary in the cfg though, but I agree the author should be able to decide. #5287 (comment)
| variants target_bus: | ||
| - virtio: | ||
| - sata: | ||
| only coldplug |
There was a problem hiding this comment.
Please add no s390-virtio for 'sata'
| :param image_file: image file path | ||
| :param matched_size: anticipated size | ||
| :param test: test case itself | ||
|
|
There was a problem hiding this comment.
The empty line from l48 should be before the param listing in l45 IMO.
| device_format = params.get("target_format") | ||
| source_file_path = setup_scsi_debug_block_device() | ||
| overlay_source_file_path = params.get("overlay_source_file_path") | ||
| matched_size = params.get("block_size_in_bytes") |
There was a problem hiding this comment.
Here and in the rest of the code (function params), why not call it in a more standard way "expected_size"?
There was a problem hiding this comment.
change name to expected_size
| vmxml_backup.sync() | ||
| libvirt.delete_scsi_disk() | ||
| # Clean up files | ||
| for file_path in cleanup_files: |
There was a problem hiding this comment.
IMO, the two comments are not necessary the xml.sync and file_path in cleanup_file already conveys what's done.
There was a problem hiding this comment.
remove those two comments
| except virt_vm.VMStartError as details: | ||
| test.fail("VM failed to start." | ||
| "Error: %s" % str(details)) | ||
| else: |
There was a problem hiding this comment.
What if does this correspond to? Is this a typo?
There was a problem hiding this comment.
this is try exception...else ..finally mechanism
| target_dev = params.get('target_dev') | ||
| status_error = "yes" == params.get("status_error") | ||
| try: | ||
| device_obj = create_customized_disk(params, test) |
chunfuwen
force-pushed
the
automate_blockresize_case
branch
from
da35048
to
9b3c586
Compare
|
|
||
|
|
||
| def check_image_virtual_size(image_file, matched_size, test): | ||
| """ |
There was a problem hiding this comment.
matched_size -> expected_size?
| except virt_vm.VMStartError as details: | ||
| test.fail("VM failed to start." | ||
| "Error: %s" % str(details)) | ||
| else: |
There was a problem hiding this comment.
@chunfuwen Just a minor nitpick about the usage of "matched_size", rest LGTM. Thanks
chunfuwen
force-pushed
the
automate_blockresize_case
branch
from
9b3c586
to
a8cefce
Compare
Cover blockresize in various scenarios Signed-off-by: Chunfu Wen <chwen@redhat.com>
chunfuwen
force-pushed
the
automate_blockresize_case
branch
from
a8cefce
to
51c1a69
Compare
Cover blockresize in various scenarios