-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some cases to check migration over unix socket #3256
Conversation
42b4605
to
afb3fbf
Compare
Test results:
|
- positive_testing: | ||
status_error = "no" | ||
variants: | ||
- basic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per explanaiton:https://libvirt.org/migration.html#scenarionativepeer2peer
-basic change to -Hypervisor-native makes more meaningful.
Network data transports ¶
There are two options for the data transport used during migration, either the hypervisor's own native transport, or tunnelled over a libvirtd connection.
@smitterl , would you mind joining in VM migration related case review ? |
afb3fbf
to
9e95dc1
Compare
9e95dc1
to
6784261
Compare
@fangge1212 , we may need align with some migration terminology at the beginning point of feature cases design |
|
without either --direct or --p2p: Note:
non-live migration: offline migration: |
:param params: Dictionary with the test parameters | ||
:param env: Dictionary with test environment. | ||
""" | ||
def update_disk(vm, params): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider moving helper functions to top level, ie. same level as 'run' function, to a) avoid using variables that are not passed, b) improve readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. update_disk
updates a value that's declared further down the code after it's definition while nothing in the functions signature indicates that that value is being updated
for did in vm_did_list: |
assumes list declared in
vm_did_list = [] |
is already filled with values from
vm_did_list.append(target_dev) |
after the function has been called in
update_disk(vm, params) |
the signature being
def update_disk(vm, params): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing your option! I understand what you mean but in other hand, it's convenient to implement the operations. And we have both of them in our existing code.
Can I say it's a matter of coding style? It looks like we don't have a limitation about it.
There are pluses and minuses to all the solutions, I'm ok with both of them. But if you think it blocks this PR, we'd better to have a discussion about this problem with whole team. How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chunfuwen What do you think? I think in context of improving code quality we should insist on having non-nested functions.
@Yingshun You have to admit that the signature e.g. of update_disk is wrong - it uses a local variable 'vm_did_list' which is not shown in the signature. I think for consistency if we accept nested run functions at least the signature should either be empty and variables just consumed because they are available to scope, or the signature has to be correct, listing all parameters or accepting a dictionary holding all parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzhengfy @kylazhang @chloerh @kamilvarga Would you mind sharing your ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view it is better to move the helper functions to the top level as suggested by @smitterl especially due to first argument: "a) avoid using variables that are not passed". There might be also impact on readability, but not so significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with kamil and smitterl 's comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitterl @kamilvarga redefined such lists of images in this function. Would you please help review again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chunfuwen Would you mind adding some details to the review comments so that we can understand you clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitterl @kamilvarga @chunfuwen , thanks for your comments. Your opinion is valuable and a good point. In the past, we do not have a strict rule for this, so you may see both writings exist in many python files. I will create an issue to specifically discuss about this and hope we can reach an agreement finally, then we will follow the conclusion in the following pull requests.
Regarding this PR, it implemented many critical and high test cases and feature owner is waiting for its run in jenkins, so we decide to merge it now to speed up our tests. Thank you.
|
||
remote_session.close() | ||
|
||
def check_socket(params): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s. above
disks_uri_port = params.get("disks_uri_port", "44444") | ||
migrate_again = "yes" == params.get("migrate_again") | ||
func_params_exists = "yes" == params.get("func_params_exists", "no") | ||
func_name = params.get("action_during_mig") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, better name variable same as config parameter (like in all the other cases): action_during_mig = params.get("action_during_mig")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variant func_name's used in almost all of the scripts under src/migration, so let's keep it. Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chunfuwen What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yingshun To be honest, my preference would be to rather change it in all other occurrences, too. When I read the code further down I don't remember what func_name is intended to be used for. I mean you wouldn't want to name "disks_uri_port" "int_as_string_param_1" either (I admit I'm exaggerating a bit :P). I mean, we already have this in our review guide IIUC:
variable name issue:
name should be meaningful
https://github.com/autotest/tp-libvirt/blob/master/tp-libvirt_review_comment_summary.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my memory, 'func_name' is used in migrate_options_shared.py as a common name to accommodate different function names in different cases. But in this py file, 'func_name' is used to always read the parameter of 'action_during_mig', so I think maybe we do not need to use a common name any more here. But I think that is only better to change, not a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below are the files we used 'func_name', if we need another name, I prefer to do it separately.
migrate_bandwidth.py
migrate_gluster.py
migrate_mem.py
migrate_network.py
migrate_options_shared.py
migrate_over_unix.py
migrate_storage.py
sriov_migrate.py
vm_session_after_mig.cmd( | ||
"echo mytest >> /mnt/%s1/mytest" % did) | ||
finally: | ||
logging.info("Recover test environment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is logging this message necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I often check this message when debugging migration jobs so let's keep it.
"test%s.img" % str(idx)) | ||
libvirt.create_local_disk("file", disk_format=disk_format, | ||
path=disk_path, size="500M") | ||
target_dev = 'vd' + chr(idx + 96) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using magic number here. Might be worth creating function and moving to avocado-vt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing it to target_dev = 'vd' + chr(idx + ord('a') - 1)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yingshun I believe it's still cryptical and should really live in a reusable function but I wouldn't block approval on it.
disk_path, | ||
new_disk_dict, False) | ||
if result.exit_status: | ||
raise test.fail("Attach device %s failed." % target_dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For setup issues like this the recommendation, IIUC, is to use test.error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
||
local_image_list.append(disk_path) | ||
|
||
disk_cmd = ("qemu-img create -f %s %s 500M" % |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to consider creating an analog to libvirt.create_local_disk
that you used above - e.g. libvirt.create_remote_disk
. This would improve readability and helps avoid duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I know, only storage migration cases need to create disks on remote host. and we have some functions to execute command lines on a remote host, such as utils_misc.cmd_status_output(), remote.run_remote_cmd() and session.cmd/cmd_status_output() and so on. So I just generate a command line and send it to utils_misc.cmd_status_output(). We may run different commands on a remote host and if they're not quite common, I'd rather call the above functions than define each one individually. Can you please tell me what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yingshun I'm sorry, I'm not sure I understand. Not having the _cmds but rather an equivalent create_remote_disk or more general create_disk would make your code more readable besides being useful. Something that reads like
libvirt.create_disk("file", disk_format=disk_format,
path=disk_path, size="500M", session=session)
This way you can also use it in a local guest or even in a remote guest. For safe refactoring, libvirt.create_local_disk
could either delegate to create_disk
- the code would have to run cmd_status_output on session and process.run or process.getstatusoutput for local access without session - or create_disk
delegates simply to create_local_disk
if session==None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please check avocado-framework/avocado-vt#2915.
eefb6a2
to
7434248
Compare
@smitterl I've updated the code, please help review again. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Yingshun I mainly have some more questions. Everything is relative and I agree that having a discussion helps to understand what expectations there are to the test code.
fe538ed
to
9e241e4
Compare
disk_path, | ||
new_disk_dict, False) | ||
if result.exit_status: | ||
raise test.error("Attach device %s failed." % target_dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include detailed error message for better debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
if result.exit_status: | ||
raise test.error("Attach device %s failed." % target_dev) | ||
|
||
local_image_list.append(disk_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure this image can be removed in most cases, we'd better put this line after line 68.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, updated.
|
||
def check_socket(params): | ||
""" | ||
Update interfaces for guest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the docstring , please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
disks_uri_port = params.get("disks_uri_port", "44444") | ||
migrate_again = "yes" == params.get("migrate_again") | ||
func_params_exists = "yes" == params.get("func_params_exists", "no") | ||
func_name = params.get("action_during_mig") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my memory, 'func_name' is used in migrate_options_shared.py as a common name to accommodate different function names in different cases. But in this py file, 'func_name' is used to always read the parameter of 'action_during_mig', so I think maybe we do not need to use a common name any more here. But I think that is only better to change, not a must.
9e241e4
to
ecb12c3
Compare
This PR adds below cases: 1) Migrate vm over unix socket 2) Migrate vm over unix socket - libvirt tunnelled(--tunnelled) 3) Migrate vm over unix socket - enable multifd(--parallel) 4) Migrate vm with copy storage over unix socket - one disk 5) Migrate vm with copy storage over unix socket - multiple disks 6) Abort migration over unix socket and migrate again 7) Abort migration with copy storage over unix socket, and migrate again 8) Migrate vm with copy storage over unix socket - multiple disks - enable multifd(--parallel) Signed-off-by: Yingshun Cui <yicui@redhat.com>
ecb12c3
to
fa74c1a
Compare
mig_result = None | ||
local_image_list = [] | ||
remote_image_list = [] | ||
vm_did_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above assignments seems useless because in Line 193, they will be reassigned by update_disk() function which already declares local variables with same names. Maybe you can set them to None or pass them to update_disk(), I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an exception is raised before calling update_disk() there will be a problem. And local_image_list and remote_image_list will be checked in 'finally' block as below:
tp-libvirt/libvirt/tests/src/migration/migrate_over_unix.py
Lines 270 to 273 in 6784261
for source_file in local_image_list: | |
libvirt.delete_local_disk("file", path=source_file) | |
for img in remote_image_list: | |
remote.run_remote_cmd("rm -rf %s" % img, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok regarding your words. But I do not think it is good practice. However, it is not a must to change.
mig_result = None | ||
local_image_list = [] | ||
remote_image_list = [] | ||
vm_did_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok regarding your words. But I do not think it is good practice. However, it is not a must to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds below cases:
1) Migrate vm over unix socket
2) Migrate vm over unix socket - libvirt tunnelled(--tunnelled)
3) Migrate vm over unix socket - enable multifd(--parallel)
4) Migrate vm with copy storage over unix socket - one disk
5) Migrate vm with copy storage over unix socket - multiple disks
6) Abort migration over unix socket and migrate again
7) Abort migration with copy storage over unix socket, and migrate again
8) Migrate vm with copy storage over unix socket - multiple disks
- enable multifd(--parallel)
depends on:
avocado-framework/avocado-vt#2891
avocado-framework/avocado-vt#2892
avocado-framework/avocado-vt#2894
avocado-framework/avocado-vt#2915
Signed-off-by: Yingshun Cui yicui@redhat.com