Skip to content
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.

Sign up for GitHub

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

Jump to bottom

migration: Add 3 migration cases to check disks port option #3343

Merged
merged 1 commit into from
Apr 12, 2021
Merged

migration: Add 3 migration cases to check disks port option #3343

merged 1 commit into from
Apr 12, 2021

Conversation

Yingshun
Copy link
Contributor

@Yingshun Yingshun commented Feb 24, 2021 •
edited
Loading

This PR adds 2 migration cases:

  1. RHEL-196401: Migrate vm with copy storage over TCP transport - Specified IP
  2. RHEL-196402: Migrate vm with copy storage over TCP transport - Specified IP+Port
  3. RHEL-196392: [--disks-uri] Migrate vm with copy storage over TCP transport - Specified IP+Port

depends on:
avocado-framework/avocado-vt#2958
avocado-framework/avocado-vt#2957

Test results:

# avocado run --vt-type libvirt --vt-machine-type q35 virsh.migrate_storage.migrateuri
WARNING:root:No python imaging library installed. Screendump and Windows guest BSOD detection are disabled. In order to enable it, please install python-imaging or the equivalent for your distro.
WARNING:root:No python imaging library installed. PPM image conversion to JPEG disabled. In order to enable it, please install python-imaging or the equivalent for your distro.
JOB ID     : 6bf0781ac4ed01231133bb84984585efcdc54ea8
JOB LOG    : /root/avocado/job-results/job-2021-03-17T05.01-6bf0781/job.log
 (1/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.default.ipv4.copy_storage_all: PASS (156.52 s)
 (2/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.default.ipv6.copy_storage_all: PASS (166.52 s)
 (3/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.disks_port.copy_storage_all: PASS (179.71 s)
 (4/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.disks_uri.custom_port.hostname.copy_storage_all: PASS (181.25 s)
 (5/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.disks_uri.custom_port.ipv4_addr.copy_storage_all: PASS (183.93 s)
 (6/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.disks_uri.custom_port.ipv6_addr.copy_storage_all: PASS (166.87 s)
 (7/7) type_specific.io-github-autotest-libvirt.virsh.migrate_storage.migrateuri.disks_uri.default_port.ipv4_addr.copy_storage_all: PASS (165.85 s)
RESULTS    : PASS 7 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 1205.15 s

@Yingshun
Copy link
Contributor Author

@dzhengfy Could you please help review this pr? Thanks!

@Yingshun Yingshun marked this pull request as draft February 25, 2021 01:00
@Yingshun Yingshun marked this pull request as ready for review February 25, 2021 10:30
@Yingshun Yingshun mentioned this pull request Mar 1, 2021
Open
@dzhengfy
Copy link
Contributor

dzhengfy commented Mar 3, 2021

@fangge1212 , could you help also review this PR, especially for the checkpoints?

dzhengfy
dzhengfy reviewed Mar 4, 2021
View reviewed changes
libvirt/tests/src/migration/migrate_storage.py
except Exception as err:
logging.error(err)

logging.info("Recovery VM XML configration")
if vm.is_alive():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose to move this part out of try block?

Copy link
Contributor Author

@Yingshun Yingshun Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When an exception is thrown while destroying a remote VM, the vm on local will not be destroyed in previous code.

libvirt/tests/src/migration/migrate_storage.py
except Exception as err:
logging.error(err)

logging.info("Recovery VM XML configration")
if vm.is_alive():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose to move this part out of try block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my answer above.

@Yingshun Yingshun requested a review from dzhengfy March 4, 2021 03:03
@chunfuwen
Copy link
Contributor

@dzhengfy
@kylazhang
Before this I never see that one module under test src is exported as python package, it may leads to confusion how we organize all codes in good order

@Yingshun
Copy link
Contributor Author

@dzhengfy
@kylazhang
Before this I never see that one module under test src is exported as python package, it may leads to confusion how we organize all codes in good order

I'm waiting for the conclusion of #3339. Please comment on that issue if you have any ideas

@Yingshun Yingshun force-pushed the mig_disks_port branch 3 times, most recently from d205109 to e427fb8 Compare March 17, 2021 10:06
@Yingshun
Copy link
Contributor Author

@dzhengfy I moved the common function to avocado-vt. Could you please help review it again?

dzhengfy
dzhengfy approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Yingshun Yingshun changed the title migration: Add 2 migration cases to check disks port option migration: Add 3 migration cases to check disks port option Mar 18, 2021
chunfuwen
chunfuwen reviewed Mar 29, 2021
View reviewed changes
libvirt/tests/src/migration/migrate_storage.py Outdated
@@ -13,6 +13,8 @@
from virttest.libvirt_xml import vm_xml
from virttest.utils_test import libvirt
from virttest.utils_libvirt import libvirt_config
from virttest.utils_libvirt import libvirt_misc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge those two lines import with previous one with ',' separate them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why we should do that? I think it's easier to maintain and read the code than you suggest.

@Yingshun Yingshun requested a review from chunfuwen March 29, 2021 07:29
@Yingshun
migration: Add 3 migration cases to check disks port option
45e0c4d 
This PR adds 3 migration cases:
RHEL-196401: Migrate vm with copy storage over TCP transport
    with a specified IP
RHEL-196402: Migrate vm with copy storage over TCP transport
    with specified IP+Port
RHEL-196392: [--disks-uri] Migrate vm with copy storage over
    TCP transport - Specified IP+Port

Signed-off-by: Yingshun Cui <[email protected]>
chunfuwen
chunfuwen approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@chunfuwen chunfuwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dzhengfy dzhengfy merged commit f210002 into autotest:master Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fangge1212 fangge1212 fangge1212 approved these changes

@chunfuwen chunfuwen chunfuwen approved these changes

@dzhengfy dzhengfy dzhengfy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yingshun @dzhengfy @chunfuwen @fangge1212