disk_utilities: new functions provider #4219

mcasquer · 2024-11-26T13:55:33Z

disk_utilities: new functions provider

Creates a new file containing a set of functions
useful for managing disks and filesystems in guest
environments

Signed-off-by: mcasquer [email protected]
ID: 3118

mcasquer · 2024-11-26T13:58:20Z

@qingwangrh please, could you help reviewing this PR? Thanks !

mcasquer · 2024-11-26T14:01:05Z

provider/disk_utilities.py

+        guest_cmd = guest_cmd.format(output_path)
+
+    session.cmd(guest_cmd)
+    vm.reboot()


@qingwangrh I've tried many things after the reboot but it always complains about: Shell process terminated while waiting for command to complete
I am not sure if either the file doesn't exist or the the command is not working, do you have any suggestion here? I tried with a single dir {output_path}

Removed the file check as first, it doesn't belong to the intention of this provider and second, it is not really needed if the dd command succeeds
FYI @qingwangrh

qingwangrh · 2024-11-28T02:56:24Z

provider/disk_utilities.py

+    idx_info = get_disk_props_by_serial_number(session, serial, ["Index"])
+    if idx_info:
+        return idx_info["Index"]
+


I suggest adding a function like get_disk_by_id or get_disk_by_serial. The ID may be the serial/www/device ID. It may handle different OSs. for windows it return disk idx, for linux , it return disk name

@qingwangrh done!

qingwangrh · 2024-11-28T03:11:05Z

provider/disk_utilities.py

+    if idx_info:
+        return idx_info["Index"]
+
+


after the first function is ready, We can provide more functional encapsulation. like as
fake code:
init_disk_by_id(mountpoint=“1“,fstype=none):
“”“
mountpoint:
0 ：it does not need mount

auto select the mountpoint
other: user specify the mountpoint

“”“
id=get_disk_by_id
if not fstype:
fstype= ntfs if windows else xfs
format_disk_with_fstype
mount disk->mountpoint
return mountpoint

@qingwangrh done!

qingwangrh · 2024-11-28T03:17:52Z

provider/disk_utilities.py

+    return guest_cmd
+
+
+def guest_fs_mount_and_dd_cmd(os_type, tmp_dir):


same thinking. we may consider more powerful encapsulation, like as

execute_cmd_on_disk_by_id():
drv= init_disk_by_id
cmd=xxxxx
session_cmd ()

the utils provide different granularity functions, most cases may use execute_cmd_on_disk_by_id directly.

@qingwangrh
Just one comment here, we may want to directly execute the command? or just return it? Because execution could be done on the test side e.g. after rebooting the VM and renewing the session

the session and cmd as parameters, it could have two functions, 1 hardcode execute dd 2. execute cmd come from parameter.
I prefer the function 2. based on the function 2 , you may wrap the function 1
.

mcasquer · 2024-11-29T07:19:28Z

@qingwangrh pleasae could you review again this PR? Thanks !

mcasquer · 2024-12-05T07:25:07Z

This is a kindly reminder, @qingwangrh please could you review again this PR? Thanks !

mcasquer · 2024-12-05T07:25:50Z

@yanan-fu and @YongxueHong could you also help reviewing this PR? Thanks !

qingwangrh · 2024-12-06T01:27:21Z

provider/disk_utilities.py

+    session.cmd(cmd)
+
+
+def execute_cmd_on_disk_by_id(params, disk_id, session, tmp_dir):


this function hardcodes dd cmd, so I suggest to change the name to execute_dd_on_disk_by_id.
this function may cover the most user cases.

And could you please add a new execute_cmd_on_disk_by_id(params, disk_id, session, cmd) to support user defined cmd ?

@qingwangrh renamed the existing function and added a new one

qingwangrh · 2024-12-06T01:29:32Z

provider/disk_utilities.py

+        tmp_dir=tmp_dir,
+    )
+    if os_type == "windows":
+        cmd = "dd if=/dev/urandom of={}:\\test.dat bs=1M count=10"


count=10 ->count=100 to keep consistency with linux.

@qingwangrh updated !

qingwangrh · 2024-12-06T01:35:51Z

provider/disk_utilities.py

+    session.cmd(cmd)
+
+
+def execute_cmd_on_disk_by_id(params, disk_id, session, tmp_dir):


parameter change to (params, disk_id, session, tmp_dir=None): most user they do not need to care the tmp_dir

the implementation may help to do the following:
if not tmp_dir:
tmp_dir="/var/tmp/"+disk_id

@qingwangrh updated !

Creates a new file containing a set of functions useful for managing disks and filesystems in guest environments Signed-off-by: mcasquer <[email protected]>

mcasquer · 2024-12-16T11:30:33Z

We're approaching to the end of the quarter... again a kindly reminder, @yanan-fu @YongxueHong could you help reviewing this PR? Thanks !

YongxueHong · 2024-12-10T06:11:53Z

provider/disk_utilities.py

+from virttest.utils_windows.drive import get_disk_props_by_serial_number
+
+
+def get_disk_by_id(os_type, disk_id, session):


Suggested change

def get_disk_by_id(os_type, disk_id, session):

def get_disk_by_id(vm, os_type, disk_id):

From my understanding, we want to provde an interface of VM for the user, so I suggest that we could specify the scope with the vm argument, and then get the session object by the interface wait_for_login of the vm object inside the function.
Pls let me know your thougths.
Thanks.

Hi @mcasquer
I am a bit confused about the definition of the disk object by calling get_disk_by_id.
How do you define it? I think we are better to clarify what kind of information the user will get with the naming of the function. The concept disk of get_disk_by_id compares with it returns is a bit of generalization
Here, we can get the disk index inside Windows guest, and the disk path inside Linux guest.
So I think we could figure out and abstract out a name to represent such return value, for example: get_disk_xxxx_by_id to let the users know how and where they can call it instead of reading the code to get to know which information they will get.
Feel free to share your thoughts. Thank you!

@YongxueHong what about get_disk_reference_by_id() ?

YongxueHong · 2024-12-17T05:08:00Z

provider/disk_utilities.py

+        return utils_disk.configure_empty_disk(session, disk, img_size, "windows")[0]
+
+
+def mount_disk(disk, session, tmp_dir):


It looks like it does not support the Windows guest session.

YongxueHong · 2024-12-17T05:21:35Z

provider/disk_utilities.py

+    :param img_size: The size of the image to be configured in Windows.
+    :param session: The guest session.
+    """
+    if fstype == "xfs":


For Linux, there are several fs type it support, such as ext3, ext4, Btrfs, and others.
Do we just cover the xfs fs type? I suggest raising an error to let the user know we do not support other types besides xfs.

YongxueHong · 2024-12-17T05:34:06Z

provider/disk_utilities.py

+    return driver
+
+
+def format_disk_by_fstype(disk, fstype, img_size, session):


Hi @mcasquer
Could you explain more about the purpose?
From your code, I see the different behaviors between Linux guest and Window guest.
Thanks.

Yeah, disk formatting is needed if we want to access to the disk inside the guest, please @qingwangrh correct me if I am wrong

YongxueHong · 2024-12-17T05:40:11Z

provider/disk_utilities.py

+        session.cmd(f"umount {tmp_dir}")
+
+
+def execute_cmd_on_disk_by_id(params, disk_id, session, cmd, tmp_dir=None):


Hi @mcasquer
Could you provide more information about it?
Which scenario do you want to apply?
Thanks.

This function won't be used right now, but the idea is for the users to execute a command that applies to the specific guest disk, cc @qingwangrh

This function won't be used right now, but the idea is for the users to execute a command that applies to the specific guest disk, cc @qingwangrh

Thanks for your explanation.
I suggest not adding it at this moment as there is no such place to call it, we could add it when we really need it.
The reason is that we need to collect the requirement for the interface and then we could design it well in order to satisfy the user's scenarios.
Please let me know your opinion.
Thanks.

@YongxueHong I agree with you :) removing it

YongxueHong · 2024-12-17T05:53:37Z

provider/disk_utilities.py

+    session.cmd(cmd)
+
+
+def execute_dd_on_disk_by_id(params, disk_id, session, tmp_dir=None):


It looks like not only executes the dd command but also other jobs, like mount and umount here, but the name of the function mainly focuses on executing the dd command by the user's requirement, I think.
From my perspective, we just execute the dd command, not provide to customize the dd option for the user.

And we should make sure this function is the common interface as the provider module if we really want to create it.
Hi @mcasquer
Would you help to explain the reason for this function?
Thanks.

@YongxueHong here are all the steps needed to execute a dd command on a specific guest disk. If the function only does the dd it shouldn't be necessary because it will be the same as session.cmd("dd ...") so that's the reason of the function. I will add more info in the docstring, please @qingwangrh correct me if I am wrong

YongxueHong · 2024-12-17T05:59:25Z

Hi @peixiu @menli820 @fbq815 @maxujun
Would you like to help to review it?
Thanks a lot.

YongxueHong · 2024-12-18T14:23:01Z

Hi @mcasquer
Would you like to help to refine the docstring for each function? even each parameter?
Thanks a lot.

maxujun · 2024-12-26T14:20:02Z

provider/disk_utilities.py

+from virttest.utils_windows.drive import get_disk_props_by_serial_number
+
+
+def get_disk_by_id(os_type, disk_id, session):


Suggest use the same way to get os_type, os_type = params["os_type"]

maxujun · 2024-12-26T14:22:42Z

provider/disk_utilities.py

+from virttest.utils_windows.drive import get_disk_props_by_serial_number
+
+
+def get_disk_by_id(os_type, disk_id, session):


Suggest use the same way to get os_type, os_type = params["os_type"]

maxujun · 2024-12-26T14:23:11Z

provider/disk_utilities.py

+def init_disk_by_id(
+    mount_point="1",
+    fstype=None,
+    os_type="linux",


Suggest use the same way to get os_type, os_type = params["os_type"]

maxujun · 2024-12-26T14:25:30Z

provider/disk_utilities.py

+    return driver
+
+
+def format_disk_by_fstype(disk, fstype, img_size, session):


Suggest set default value None for img_size.

mcasquer force-pushed the 3118_new_disk_functions_provider branch from a9a7080 to b126ffe Compare November 26, 2024 13:55

mcasquer mentioned this pull request Nov 26, 2024

block_disk_not_default_options: creates new test case #4209

Open

mcasquer marked this pull request as ready for review November 26, 2024 13:57

mcasquer commented Nov 26, 2024

View reviewed changes

mcasquer force-pushed the 3118_new_disk_functions_provider branch 3 times, most recently from ee6ef9a to 997fc06 Compare November 27, 2024 09:03

qingwangrh reviewed Nov 28, 2024

View reviewed changes

mcasquer force-pushed the 3118_new_disk_functions_provider branch from 997fc06 to f126ecf Compare November 29, 2024 07:00

qingwangrh reviewed Dec 6, 2024

View reviewed changes

disk_utilities: new functions provider

41c59b2

Creates a new file containing a set of functions useful for managing disks and filesystems in guest environments Signed-off-by: mcasquer <[email protected]>

mcasquer force-pushed the 3118_new_disk_functions_provider branch from f126ecf to 41c59b2 Compare December 10, 2024 10:21

YongxueHong reviewed Dec 17, 2024

View reviewed changes

maxujun reviewed Dec 26, 2024

View reviewed changes

		return guest_cmd


		def guest_fs_mount_and_dd_cmd(os_type, tmp_dir):

		session.cmd(cmd)


		def execute_cmd_on_disk_by_id(params, disk_id, session, tmp_dir):

		from virttest.utils_windows.drive import get_disk_props_by_serial_number


		def get_disk_by_id(os_type, disk_id, session):

	def get_disk_by_id(os_type, disk_id, session):
	def get_disk_by_id(vm, os_type, disk_id):

		return utils_disk.configure_empty_disk(session, disk, img_size, "windows")[0]


		def mount_disk(disk, session, tmp_dir):

		return driver


		def format_disk_by_fstype(disk, fstype, img_size, session):

		session.cmd(f"umount {tmp_dir}")


		def execute_cmd_on_disk_by_id(params, disk_id, session, cmd, tmp_dir=None):

		session.cmd(cmd)


		def execute_dd_on_disk_by_id(params, disk_id, session, tmp_dir=None):

disk_utilities: new functions provider #4219

Are you sure you want to change the base?

disk_utilities: new functions provider #4219

Conversation

mcasquer commented Nov 26, 2024

mcasquer commented Nov 26, 2024

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

qingwangrh Nov 28, 2024 • 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

qingwangrh Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

mcasquer commented Nov 29, 2024

mcasquer commented Dec 5, 2024

mcasquer commented Dec 5, 2024

qingwangrh Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

qingwangrh Dec 6, 2024 • 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

mcasquer commented Dec 16, 2024

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

YongxueHong commented Dec 17, 2024

YongxueHong commented Dec 18, 2024

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

qingwangrh Nov 28, 2024 •

edited

Loading

qingwangrh Nov 29, 2024 •

edited

Loading

qingwangrh Dec 6, 2024 •

edited

Loading

qingwangrh Dec 6, 2024 •

edited

Loading