memory: add case for hugepage mount path #4984

nanli1 · 2023-06-21T09:27:35Z

VIRT-297104: Huge page mount path
Signed-off-by: nanli [email protected]

 avocado run --vt-type libvirt --test-runner=runner --vt-machine-type q35 memory.backing.mount_path

 (1/3) type_specific.io-github-autotest-libvirt.memory.backing.mount_path.default: PASS (61.76 s)
 (2/3) type_specific.io-github-autotest-libvirt.memory.backing.mount_path.disabled: PASS (25.95 s)
 (3/3) type_specific.io-github-autotest-libvirt.memory.backing.mount_path.customized: PASS (29.31 s)

Depend on

add more flexible for consume_mem and check_qemu func avocado-framework/avocado-vt#3767
add uncheck service active in restart func avocado-framework/avocado-vt#3780

nanli1 · 2023-07-18T03:02:45Z

modular_redesign

dzhengfy · 2023-09-18T02:38:20Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        virsh.destroy(vm_name, ignore_status=False)
+
+        test.log.info("TEST_STEP13: Restart service")
+        process.run("systemctl restart %s" % libvirtd.service_name, shell=True)


For restarting a service, usually we do not use process.run(). Have you tried utils_libvirtd.Libvirtd() ?

Thanks dzheng, same as #4984 (comment) ,Could check ,thank you

Please see my reply #4984 (comment)

dzhengfy · 2023-09-18T02:40:46Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        test.log.info("TEST_STEP11: Check mem")
+        session = vm.wait_for_login()
+        mem_free = utils_memory.freememtotal(session)
+        status, stdout = session.cmd_status_output("swapoff -a; memhog %s" % (


Could you try libvirt_memory.consume_vm_freememory()?

Thanks , def consume_vm_freememory has a hard code and for memhog %10000k , so I make a enhance by avocado-framework/avocado-vt#3767

dzhengfy · 2023-09-18T02:45:18Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+            return
+
+        test.log.info("TEST_STEP7: Stop services")
+        utils_libvirtd.libvirtd_stop()


This function is already with deprecation warning. Please use utils_libvirtd.libvirtd().stop() instead.

Thanks ,done

dzhengfy · 2023-09-18T02:52:39Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        process.run("ps -ef | grep qemu | grep prealloc > %s" % out_file,
+                    shell=True)
+        for item in check_qemu:
+            libvirt.check_logfile(item, out_file)


Line 83 - 86, for checking qemu command line, there is libvirt.check_qemu_cmd_line(), but it only supports to search one pattern a time. Could you consider to reuse it? Or if you could enhance that function to support checking a list of patterns, that will be appreciated.

Thanks dzheng, enhance by avocado-framework/avocado-vt#3767

dzhengfy · 2023-09-18T02:53:03Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        if case == "disabled":
+            return
+
+        test.log.info("TEST_STEP3: Start guest guest")


Remove one 'guest'

dzhengfy · 2023-09-18T02:56:19Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+
+        test.log.info("TEST_STEP2: Restart libvirt service")
+        libvirtd = utils_libvirtd.Libvirtd()
+        process.run("systemctl restart %s" % libvirtd.service_name, shell=True)


Could you explain the reason you do not use libvirtd.restart() ?

Thanks dzheng, becausedef libvirtd.restart()has invoked result.append(self._wait_for_start()) , it is always checking the virsh list for until service active , however the TEST_STEP1 has modified by the huge page mount path invalid config for some scenarios , which makes the virsh list does not work

Ok, I understand.
But I think it is a good time to enhance this function.
For example, we add a new option to disable the checking. When we do not care about the result with wait_for_start=False, it will always return True. And then users need to check the service status using xxx.is_running() by themselves. How about?

def restart(self, reset_failed=True, wait_for_start=True): result = [] for daem_item in self.daemons: if reset_failed: daem_item.reset_failed() if not daem_item.restart(): return False if wait_for_start: result.append(self._wait_for_start()) return all(result)

@Yingshun @chloerh @chunfuwen What's your opinion?

@dzhengfy
The restart function expect to work in very simple way since we hope it can return as soon as possible.
Moreover, the return result expect simple as much as we can,otherwise it need upstream code(caller) to handle different return type.
Another alternatives is to override another function if we really find it is more of common use cases

Thanks all, Done

@dzhengfy The restart function expect to work in very simple way since we hope it can return as soon as possible. Moreover, the return result expect simple as much as we can,otherwise it need upstream code(caller) to handle different return type. Another alternatives is to override another function if we really find it is more of common use cases

I checked in tp-libvirt for the usage of restart(), and I did not find any caller to use the return value of this function. So I am wondering if we really need this return. Then I value this different return type as low risk. We can add docstring or comment to make the users be aware of this difference when they use this new parameter.

dzhengfy · 2023-09-18T02:58:53Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        if case != "default":
+            qemu_obj = libvirt.customize_libvirt_config(
+                {"hugetlbfs_mount": '"%s"' % hugetlbfs_mount}, "qemu",
+                restart_libvirt=False)


Could you explain the reason you do not use restart_libvirt=True in this function to restart libvirtd daemon?

Thanks dzheng, because we need to restart libvirt as a obvious step in Step2

dzhengfy · 2023-09-18T03:00:37Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
+        vmxml.setup_attrs(**vm_attrs)
+        virsh.define(vmxml.xml, debug=True, ignore_status=False)
+        test.log.debug("Define xml by :\n%s " % vmxml)


dzhengfy · 2023-09-18T03:07:26Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+
+        hp_cfg = test_setup.HugePageConfig(params)
+        hp_cfg.cleanup()
+        process.run("mount -t hugetlbfs hugetlbfs %s" % default_path,


Could you use

hp_cfg.hugepage_path = default_path hp_cfg.mount_hugepage_fs()

dzhengfy · 2023-10-06T04:12:48Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+
+        test.log.info("TEST_STEP4: Check the qemu cmd line")
+        for item in check_qemu:
+            # libvirt.check_logfile(item, out_file)


Remove this line, thanks

dzhengfy · 2023-10-06T04:17:33Z

libvirt/tests/src/memory/memory_backing/hugepage_mount_path.py

+        test.log.info("TEST_STEP4: Check the qemu cmd line")
+        for item in check_qemu:
+            # libvirt.check_logfile(item, out_file)
+            libvirt.check_qemu_cmd_line(item)


With avocado-framework/avocado-vt#3767, we should invoke this function by libvirt.check_qemu_cmd_line(check_qemu) directly because list is supported, right?

Thanks your reminder,Done

VIRT-297104: Huge page mount path Signed-off-by: nanli <[email protected]>

dzhengfy

LGTM

chunfuwen

lgtm

nanli1 marked this pull request as draft June 21, 2023 09:29

nanli1 force-pushed the add_case_for_huagepage_mount_path branch 3 times, most recently from b90d766 to 7a95e76 Compare June 21, 2023 12:40

nanli1 force-pushed the add_case_for_huagepage_mount_path branch from 7a95e76 to 4177644 Compare July 18, 2023 02:39

nanli1 marked this pull request as ready for review July 18, 2023 02:40

nanli1 force-pushed the add_case_for_huagepage_mount_path branch from 4177644 to b96985d Compare July 29, 2023 12:56

nanli1 marked this pull request as draft July 30, 2023 03:26

nanli1 marked this pull request as ready for review July 30, 2023 03:27

Yingshun added the modular_redesign label Aug 9, 2023

nanli1 force-pushed the add_case_for_huagepage_mount_path branch from b96985d to ee6dc4b Compare August 16, 2023 06:50

dzhengfy reviewed Sep 18, 2023

View reviewed changes

nanli1 force-pushed the add_case_for_huagepage_mount_path branch 2 times, most recently from c24702c to 848336c Compare September 19, 2023 07:44

nanli1 requested a review from dzhengfy September 19, 2023 07:45

dzhengfy added the depend on The PR has dependency on other PRs label Oct 6, 2023

dzhengfy reviewed Oct 6, 2023

View reviewed changes

nanli1 force-pushed the add_case_for_huagepage_mount_path branch from 848336c to c45bab7 Compare October 8, 2023 08:00

memory: add case for hugepage mount path

cf7da46

VIRT-297104: Huge page mount path Signed-off-by: nanli <[email protected]>

nanli1 force-pushed the add_case_for_huagepage_mount_path branch from c45bab7 to cf7da46 Compare October 16, 2023 07:53

nanli1 requested review from dzhengfy and chunfuwen October 16, 2023 07:54

dzhengfy approved these changes Oct 30, 2023

View reviewed changes

chunfuwen approved these changes Oct 30, 2023

View reviewed changes

chunfuwen merged commit c133fbd into autotest:master Oct 30, 2023
5 checks passed

memory: add case for hugepage mount path #4984

memory: add case for hugepage mount path #4984

Conversation

nanli1 commented Jun 21, 2023 • edited by dzhengfy Loading

nanli1 commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhengfy Oct 6, 2023 • 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

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

dzhengfy left a comment

Choose a reason for hiding this comment

chunfuwen left a comment

Choose a reason for hiding this comment

nanli1 commented Jun 21, 2023 •

edited by dzhengfy

Loading

dzhengfy Oct 6, 2023 •

edited

Loading