Add blockcopy xml test cases #2912

chunfuwen · 2020-07-07T03:26:50Z

Cover different backend storage such as file,block,iscsi,rbd,nbd,ceph and gluster

Signed-off-by: chunfuwen [email protected]

chunfuwen · 2020-07-07T03:44:46Z

PASS 1-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.block_test.pivot
PASS 2-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.block_test.finish
PASS 3-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.block_test.reuse-external

PASS 1-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.nbd_test.pivot
PASS 2-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.nbd_test.finish
PASS 3-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.nbd_test.reuse-external

PASS 1-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.file_test.pivot
PASS 2-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.file_test.finish
PASS 3-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.file_test.reuse-external

PASS 1-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.iscsi_test.pivot
PASS 2-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.iscsi_test.finish
PASS 3-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.iscsi_test.reuse-external

PASS 1-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.ceph_test.pivot
PASS 2-type_specific.io-github-autotest-libvirt.virsh.blockcopy_xml.positive_test.ceph_test.finish

dzhengfy · 2020-07-24T06:10:52Z

libvirt/tests/cfg/virsh_cmd/domain/virsh_blockcopy_xml.cfg

+    virt_disk_device_target = "vdb"
+    virt_disk_device_format = "raw"
+    virt_disk_device_bus = "virtio"
+    variants blockcopy_option:


Typo? Please correct.

no, see usage from https://avocado-vt.readthedocs.io/en/latest/CartesianConfig.html

Thanks for the reference.

dzhengfy · 2020-07-24T06:16:25Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+    virsh_dargs = {'debug': True, 'ignore_status': True}
+    ignore_check = False
+
+    def check_blockcopy_xml(vm_name, source_image, ignore_check=False):


'ignore_check' seems unnecessary. If we do not check, we need not to call this function.

this is tip to make code follow consistent workflow, rather than see too many if ... else

dzhengfy · 2020-07-24T06:20:55Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+        if backend_storage_type == "file":
+            image_filename = params.get("image_filename", "raw.img")
+            disk_path = os.path.join(data_dir.get_tmp_dir(), image_filename)
+            if blockcopy_option in ['reuse_external']:


If only one item, is it better to use blockcopy_option == 'reuse_external' simply?

reserved for further extension

dzhengfy · 2020-07-24T06:24:05Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                             "hosts":  [{"name": gluster_host_ip,
+                                         "port": "24007"}]}
+            checkout_device_source = gluster_img_name
+            test.cancel("gluster permission denied issue,see https://bugzilla.redhat.com/show_bug.cgi?id=1447694")


I am not sure the reason to raise an exception here directly. Don't we need to run some commands and filter the output to decide if cancel?

should be some problem here, even have bug, we still need to give test.fail and let CI judge it is skip or not. And the bug info can shown in test.fail

Since we now know Bugzilla 1447694 is currently-existed known issue, using test.cancel directly can save prepare effort, which is nothing useful when case is cancelled eventually.
tp-libvirt is standalone upstream project, does it make sense that additional CI is necessarily used to judge skip or not. How can we assume that any team ,which want to use tp-libvirt, need setup CI as pre-request.

Dealing with the skip case in CI will keep the code consistent even the bug fixed, so we prefer not deal with the bug in code. Else, you may forget to update it even the bug fixed.

Upstream first!
As tp-libvirt is standalone upstream project, if issue can be handled in upstream, we CAN NOT leave it to be handled by downstream , such as CI, which you mentioned here.

Please make sure that this bug is RHEL bug but not upstream bug, it is not suitable to show non upstream bug number here. The case fail with bug is acceptable for upstream testing as it have fail/bug indeed. Also we set in CI not change the failure result of tp-libvirt test, it just SHOW SKIP in jenkins, but still failure in test.

And also, a test.cancel here without any situation is not a reasonable logic. Please think and check again, thanks

delete this part test logic (gluster)to avoid PR stuck due to some controversy.
Once condition is met, this logic can be brought back.

dzhengfy · 2020-07-24T06:33:39Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+            logging.debug("disk auth dict is: %s" % disk_auth_dict)
+            disk_source.auth = disk_xml.new_auth(**disk_auth_dict)
+        disk_xml.source = disk_source
+        logging.debug("new disk xml is: %s", disk_xml)


Could you move L252-L265 to L285 where it is referenced for better readability.

all code lines before 251 prepare disk source with different backing end, just after that, one disk_xml is provisioned to consume disk source. Therefore, the workflow sounds very natural.

dzhengfy · 2020-07-24T06:37:47Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                try:
+                    nbd.cleanup()
+                except Exception as ndbEx:
+                    logging.info("Clean Up nbd failed: %s" % str(ndbEx))


logging.error is better?

Maybe forget to update?

thanks, updated now

kylazhang · 2020-07-24T11:19:36Z

libvirt/tests/cfg/virsh_cmd/domain/virsh_blockcopy_xml.cfg

+            virt_disk_device_type = "network"
+            nbd_server_port = "10001"
+    variants:
+        - positive_test:		    


nothing more for it? If you want to reserve a interface here, maybe you also need a parameter to show how to judge it is positive or negative

This is reserved for extension. Once it happen, one variant will be added for that.

kylazhang · 2020-07-24T11:21:35Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+        for line in blklist:
+            if line.strip().startswith(('hd', 'vd', 'sd', 'xvd')):
+                source_imge_list.append(line.split()[-1])
+        logging.debug('vm list is :')


what you want to debug here?

Maybe forget to update?

updated now

kylazhang · 2020-07-24T11:32:48Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                             "hosts":  [{"name": gluster_host_ip,
+                                         "port": "24007"}]}
+            checkout_device_source = gluster_img_name
+            test.cancel("gluster permission denied issue,see https://bugzilla.redhat.com/show_bug.cgi?id=1447694")


should be some problem here, even have bug, we still need to give test.fail and let CI judge it is skip or not. And the bug info can shown in test.fail

kylazhang · 2020-07-24T11:35:32Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                                                      size=size, disk_format="raw")
+        s_attach = virsh.attach_disk(vm_name, tmp_device_source, device_target,
+                                     "--config", debug=True)
+        libvirt.check_exit_status(s_attach)


please use ignore_status in virsh, thanks

it is common usage.

ok, acceptable as discussed

kylazhang · 2020-07-24T11:37:10Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+        result = virsh.blockcopy(vm_name, device_target, "--xml %s" % disk_xml.xml,
+                                 options=options,
+                                 debug=True)
+        libvirt.check_exit_status(result)


this is common usage

ok, acceptable as discussed

kylazhang

please check the comments and give update, thanks

dzhengfy · 2020-08-17T02:35:00Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                             "hosts":  [{"name": gluster_host_ip,
+                                         "port": "24007"}]}
+            checkout_device_source = gluster_img_name
+            test.cancel("gluster permission denied issue,see https://bugzilla.redhat.com/show_bug.cgi?id=1447694")


Two points I'd like to say:

As the team discussed before, we use CI blacklist to filter the expected error message to decide if the failure should be ignored or not. This method has been used for long time within the team. For this bug, if we use blacklist way, we need raise test.fail(xxx) if checking fails and blacklist will check 'xxx' to decide 'skip' or 'fail'. And when the bug is fixed, the case will PASS without any other updates by us.
But if you would not like this way, shall we discuss if this way's good for us? Would you like to launch the discussion?

Using your current way, would you like to track the status of this bug and update the case when the bug is fixed in future?

As I mention previously, upstream first is our senior principle and Red Hat's value proposition.
Please keep mind that what upstream does and what downstream does.
If CI blacklist can have better handling for bugzilla status, we can consider move those logic into tp-libvirt.
For example, we can check whether the bugzilla is valid before we can test.cancel.

Moreover, even checking current CI blacklist ,you will find that there are nearly 3000 lines there,and all mess up and become more and more hard to maintain.

kylazhang

please check comments, thanks

dzhengfy · 2020-09-02T06:01:09Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+            checkout_device_source = ceph_disk_name
+        elif backend_storage_type == "nbd":
+            # Get server hostname.
+            hostname = process.run('hostname', ignore_status=False, shell=True, verbose=True).stdout_text.strip()


It is better to use python built-in function to get host name according to we discussed before.

dzhengfy · 2020-09-02T06:29:59Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                # Create an local image and make FS on it.
+                disk_cmd = ("qemu-img create -f %s %s %s" %
+                            (device_format, img_file, storage_size))
+                process.run(disk_cmd, ignore_status=False, shell=True)


libvirt.create_local_disk() can easily work for L187-189.

dzhengfy · 2020-09-02T06:33:49Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+                                           "name": "%s/%s" % (iscsi_target, lun_num)},
+                                 "hosts": [{"name": iscsi_host, "port": iscsi_port}]}
+                checkout_device_source = 'emulated-iscsi'
+        elif backend_storage_type == "ceph":


I strongly recommend we create some ceph utility functions because it seems many work can be reuse, for example, convert the image to remote storage, clear disks in advance, prepare config file/package and so on.

create one issue to track the progress:#3055

dzhengfy · 2020-09-02T06:41:41Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+
+        logging.debug("device source is: %s", device_source)
+        # Add disk xml.
+        vmxml = vm_xml.VMXML.new_from_dumpxml(vm_name)


libvirt.create_disk_xml() should work for Line 230 - 242.

if check the implementation libvirt.create_disk_xml(), it use nearly hardcoded source_params to create disk_source,which will not satisfy current situation covering different backend storage.
source_params = {"attrs": source_attrs, "seclabels": source_seclabel,
"hosts": source_host}
src_config_file = params.get("source_config_file")
if src_config_file:
source_params.update({"config_file": src_config_file})
# If we use config file, "hosts" isn't needed
if "hosts" in source_params:
source_params.pop("hosts")
snapshot_name = params.get('source_snap_name')
if snapshot_name:
source_params.update({"snapshot_name": snapshot_name})
disk_source = diskxml.new_disk_source(**source_params)

dzhengfy · 2020-09-02T06:43:41Z

libvirt/tests/src/virsh_cmd/domain/virsh_blockcopy_xml.py

+            libvirt.setup_or_cleanup_iscsi(is_setup=False)
+        elif backend_storage_type == "ceph":
+            # Remove ceph configure file if created.
+            if ceph_cfg:


For ceph client clean up, it is better to create a reusable function

use one issue to track the progress:#3055

Cover different backend storage such as file,block,iscsi,rbd,nbd,ceph and gluster Signed-off-by: chunfuwen <[email protected]>

chunfuwen · 2020-09-17T01:15:09Z

@dzhengfy
comments are updated accordingly, please take a look.

dzhengfy

We already have issues tracking for some discussion points.

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from 89b564b to 9501324 Compare July 7, 2020 03:29

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from 9501324 to e343026 Compare July 7, 2020 04:21

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch 2 times, most recently from 3db8d07 to e35da6e Compare July 23, 2020 02:28

dzhengfy reviewed Jul 24, 2020

View reviewed changes

kylazhang reviewed Jul 24, 2020

View reviewed changes

chunfuwen requested review from kylazhang and dzhengfy August 6, 2020 04:54

kylazhang reviewed Aug 12, 2020

View reviewed changes

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from e35da6e to 62eea85 Compare August 13, 2020 01:20

chunfuwen requested a review from kylazhang August 13, 2020 01:24

dzhengfy reviewed Aug 17, 2020

View reviewed changes

chunfuwen requested a review from dzhengfy August 17, 2020 02:58

kylazhang reviewed Aug 18, 2020

View reviewed changes

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from 62eea85 to 1ecc69f Compare September 1, 2020 02:58

chunfuwen requested a review from kylazhang September 1, 2020 03:02

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from 1ecc69f to eb472fe Compare September 1, 2020 03:07

dzhengfy reviewed Sep 2, 2020

View reviewed changes

Add blockcopy xml test cases

e64131e

Cover different backend storage such as file,block,iscsi,rbd,nbd,ceph and gluster Signed-off-by: chunfuwen <[email protected]>

chunfuwen force-pushed the add_virsh_blockcopy_xml_cases branch from eb472fe to e64131e Compare September 10, 2020 03:15

chunfuwen requested a review from dzhengfy September 10, 2020 03:19

kylazhang approved these changes Sep 16, 2020

View reviewed changes

dzhengfy approved these changes Sep 17, 2020

View reviewed changes

dzhengfy merged commit 82822a1 into autotest:master Sep 17, 2020

Add blockcopy xml test cases #2912

Add blockcopy xml test cases #2912

Conversation

chunfuwen commented Jul 7, 2020

chunfuwen commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylazhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

kylazhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

chunfuwen commented Sep 17, 2020

dzhengfy left a comment

Choose a reason for hiding this comment

chunfuwen Sep 1, 2020 •

edited

Loading

chunfuwen Jul 29, 2020 •

edited

Loading

chunfuwen Aug 17, 2020 •

edited

Loading

chunfuwen Sep 10, 2020 •

edited

Loading