guest os booting: add hotplug case with boot order element #5222

meinaLi · 2023-10-12T02:11:02Z

This PR mainly implements the hotpluging of disk/usb/filesystem devices with boot order elements.

meinaLi · 2023-10-12T02:11:59Z

# avocado run --vt-type libvirt --test-runner=runner --vt-machine-type q35 guest_os_booting.boot_order.hotplug
JOB ID     : bc70ab994c4d27c90c0eb08ba786374e1b831ba0
JOB LOG    : /var/lib/avocado/job-results/job-2023-10-11T22.06-bc70ab9/job.log
 (1/3) type_specific.io-github-autotest-libvirt.guest_os_booting.boot_order.hotplug.disk_device: PASS (35.29 s)
 (2/3) type_specific.io-github-autotest-libvirt.guest_os_booting.boot_order.hotplug.usb_device: PASS (35.34 s)
 (3/3) type_specific.io-github-autotest-libvirt.guest_os_booting.boot_order.hotplug.filesystem_device: PASS (42.43 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML   : /var/lib/avocado/job-results/job-2023-10-11T22.06-bc70ab9/results.html
JOB TIME   : 113.78 s

meinaLi · 2023-10-12T02:15:15Z

Depend on avocado-framework/avocado-vt#3778.

cliping

Others LGTM

cliping · 2023-10-25T04:31:50Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+    def prepare_device_xml(vm_xml, device_type):
+        """
+        Prepare the hot-plugged device xml.
+        :params device_type: the device type


Miss vm_xml and return value docstring.

cliping · 2023-10-25T04:34:08Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+    def check_dumpxml(vmxml, device_type, new_image_path):
+        """
+        Check if the device is plugged and included the boot order element.
+        :params vmxml: the current guest xml


miss device_type and new_image_path docstring.

Yingshun · 2023-10-31T07:16:45Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        Check if the device is plugged and included the boot order element.
+        :params vmxml: the current guest xml
+        :params device_type: the device type
+        :params new_image_path: the disk path


Can we directly use 'image_path'? I don't quite understand why we need a 'new_' param in this function.

Sure, we can also directly use 'image_path'.

Yingshun · 2023-10-31T07:27:23Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        Prepare the hot-plugged device xml.
+        :params vm_xml: the instance of VMXML class


Plz add a blank line between description and params, thx.

Yingshun · 2023-10-31T07:27:40Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        Check if the device is plugged and included the boot order element.
+        :params vmxml: the current guest xml


Plz add a blank line between description and params, thx.

Yingshun · 2023-10-31T07:29:06Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        check_dumpxml(vmxml, device_type, new_image_path)
+
+        test.log.info(f"STEP4: Hot-unplug the {device_type}.")
+        virsh.detach_device(vm_name, device_xml.xml, **virsh_dargs)


Do we need to wait for the events to make sure the devices are removed correctly?

Yes, it's better to wait to avoid the special situation. I've updated it.

I can't find the updates about waiting for the events, anything missed?

Sorry, I've added wait_for_event again.

Yingshun · 2023-10-31T07:48:51Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        try:
+            libvirt_vmxml.check_guest_xml_by_xpaths(vmxml, order_xpath)
+        except exceptions.TestFail as detail:
+            test.log.debug(f"The device has been detached successfully because {detail}.")


Instead of using 'try-except', it's better to set 'ignore_status' in libvirt_vmxml.check_guest_xml_by_xpaths().

Here I mainly check that the xpath is not in vmxml. So I used try-except. I think we can't use ignore_status. Could you help to check it again? Thanks.

Can you plz explain why you can't use 'ignore_status'? It was designed to avoid raising an exception when not matched. I'm not sure what your problem is, plz show us, thanks!

In my understanding, we mainly use libvirt_vmxml.check_guest_xml_by_xpaths() to check it the xml is matched. If matched, it will pass. If unmatched, it will fail automatically. Is it right? Here I mainly check the detached xml, and hope it will pass when it unmatches. So I use exception.

Replace with if statement.

Yingshun

See the comment above

Yingshun · 2023-11-03T09:35:00Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        check_dumpxml(vmxml, device_type, new_image_path)
+
+        test.log.info(f"STEP4: Hot-unplug the {device_type}.")
+        virsh.detach_device(vm_name, device_xml.xml, **virsh_dargs)


I can't find the updates about waiting for the events, anything missed?

Yingshun · 2023-11-03T09:51:50Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+            if not vm.is_alive():
+                virsh.start(vm_name, **virsh_dargs)


How about moving L84~86 into L90 and deleting these 2 lines? I think we can merge these 2 vm startup steps into one. what do you think?

I've merged the virsh.start in L84~86 before virsh.attach_device and removed the duplicated part in the function prepare_device_xml. I guess this is wat you are meaning. Please have a look again.

Yingshun · 2023-11-03T09:52:20Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+                                               hpgs=False)
+            if not vm.is_alive():
+                virsh.start(vm_name, **virsh_dargs)
+            vmxml = vm_xml.VMXML.new_from_dumpxml(vm_name)


it's not used, please remove it.

Yingshun · 2023-11-03T10:20:16Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+
+        test.log.info(f"STEP4: Hot-unplug the {device_type}.")
+        virsh.detach_device(vm_name, device_xml.xml, **virsh_dargs)
+        time.sleep(10)


I think it can be removed if you update detach-device to wait for the events,right?

Yingshun · 2023-11-03T10:23:08Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+        for xpath_text in order_xpath:
+            for boot_attrs in xpath_text['element_attrs']:
+                if vmxml.xmltreefile.findall(boot_attrs):
+                    test.fail("The device hasn't been detached.")


Is it safe to only check order_xpath? Should it be checked like check_dumpxml() in this script? I wonder if you need to check the devices instead of 'boot attrs'.

I've also updated the function check_dumpxml here to involve the hot-unplugging checking to replace these codes. In the functions, I directly used existed functions to check if the device existed, and also focused on the checking of boot order attributes.

Yingshun · 2023-11-09T03:13:55Z

libvirt/tests/cfg/guest_os_booting/boot_order/hotplug.cfg

@@ -0,0 +1,20 @@
+- guest_os_booting.boot_order.hotplug:
+    type = hotplug


@meinaLi Can we have a proper name of these files? the current one is too common. what do you think?

It is better to be much specific.

Thanks. And I've updated it to hotplug_device_with_boot_order.

Yingshun · 2023-11-09T03:35:24Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+            device_xml, image_path = disk_obj.prepare_disk_obj("file", device_dict)
+        return device_xml, image_path
+
+    def check_dumpxml(device_type, image_path, hotplug=True):


The parameter 'hotplug' is not quite easy to understand in this function, how about using expect,expected or status_error?

I didn't change it because here hotplug=True means we check the result after hotplugging. False means checking the result after hot-unplugging. So maybe no need to update it.

Sorry @meinaLi , I think I can't agree with it. It is a common checking xml function, so I still think we should just focus on whether we expect the device to exist or not, rather than qualifying it at which test step it is executed. And also, hotplug=False does not directly mean to hotunplug, I think.

After discussing, I've updated it based on what we discussed.

Yingshun · 2023-11-09T03:36:21Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug.py

+
+    def check_dumpxml(device_type, image_path, hotplug=True):
+        """
+        Check if the device is plugged and included the boot order element.


plugged -> existed ?

Yingshun

See my comment above.

Yingshun · 2023-11-17T05:47:00Z

libvirt/tests/src/guest_os_booting/boot_order/hotplug_device_with_boot_order.py

+        """
+        Check if the device is existed and included the boot order element.
+
+        :params vmxml: the current guest xml


There is no vmxml, plz remove it.

This PR mainly implements the hotpluging of disk/usb/filesystem devices with boot order elements. Signed-off-by: Meina Li <[email protected]>

cliping reviewed Oct 25, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch from 9595e59 to f03cf2f Compare October 25, 2023 08:29

meinaLi requested a review from cliping October 26, 2023 08:29

cliping approved these changes Oct 26, 2023

View reviewed changes

dzhengfy requested a review from Yingshun October 31, 2023 03:34

Yingshun reviewed Oct 31, 2023

View reviewed changes

Yingshun added the depend on The PR has dependency on other PRs label Oct 31, 2023

Yingshun reviewed Oct 31, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch from f03cf2f to 4f26d58 Compare November 3, 2023 02:04

meinaLi requested a review from Yingshun November 3, 2023 02:13

Yingshun reviewed Nov 3, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch 2 times, most recently from 73668cf to b999ba0 Compare November 3, 2023 08:22

Yingshun reviewed Nov 3, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch from b999ba0 to 2f819f0 Compare November 7, 2023 03:02

meinaLi requested a review from Yingshun November 7, 2023 03:12

Yingshun reviewed Nov 9, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch from 2f819f0 to ba8b289 Compare November 16, 2023 02:34

meinaLi requested a review from Yingshun November 16, 2023 02:44

Yingshun reviewed Nov 16, 2023

View reviewed changes

meinaLi force-pushed the hotplug branch from ba8b289 to 5bedb02 Compare November 16, 2023 08:38

meinaLi requested a review from Yingshun November 16, 2023 08:39

Yingshun reviewed Nov 17, 2023

View reviewed changes

guest os booting: add hotplug case with boot order element

5ed5366

This PR mainly implements the hotpluging of disk/usb/filesystem devices with boot order elements. Signed-off-by: Meina Li <[email protected]>

meinaLi force-pushed the hotplug branch from 5bedb02 to 5ed5366 Compare November 20, 2023 02:09

meinaLi requested a review from Yingshun November 20, 2023 02:15

Yingshun approved these changes Nov 27, 2023

View reviewed changes

Yingshun merged commit 4fe9ba2 into autotest:master Nov 27, 2023
5 checks passed

meinaLi deleted the hotplug branch November 27, 2023 02:33

		Prepare the hot-plugged device xml.
		:params vm_xml: the instance of VMXML class

		Check if the device is plugged and included the boot order element.
		:params vmxml: the current guest xml

		@@ -0,0 +1,20 @@
		- guest_os_booting.boot_order.hotplug:
		type = hotplug

guest os booting: add hotplug case with boot order element #5222

guest os booting: add hotplug case with boot order element #5222

Conversation

meinaLi commented Oct 12, 2023

meinaLi commented Oct 12, 2023

meinaLi commented Oct 12, 2023

cliping 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

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

Yingshun 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

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

Yingshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment