-
Notifications
You must be signed in to change notification settings - Fork 173
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
vioser: Supplement the correct function #4212
base: master
Are you sure you want to change the base?
Conversation
f2a458c
to
6802507
Compare
Avoid case calling case, so add corresponding functions Signed-off-by: Dehan Meng <[email protected]>
6802507
to
f7c6c69
Compare
@leidwang Could you please help to review? |
@vivianQizhu please help to review and merge this patch. 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.
LGTM:
RHEL.9.6.0.s390x.io-github-autotest-qemu.virtio_port_hotplug.hotplug_port_pci.with_live_migration_after_unplug.s390-virtio: PASS (85.23 s)
RHEL.10.0.s390x.io-github-autotest-qemu.virtio_port_hotplug.hotplug_port_pci.with_live_migration_after_unplug.s390-virtio: PASS (63.77 s)
BTW, please involve me for reviewing in the future, thanks
which feature are you responsible for now? leave me a list so that I could know when I should involve u. |
qemu_migration.set_speed(vm, params.get("mig_speed", "1G")) | ||
vm.migrate() | ||
|
||
|
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.
Sorry, I don't see the point why we need to implement them again here. Why not from vioser_in_use import live_migration_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.
what message I got from the maintainer is better to avoid calling case by case.
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.
@6-dehan This is not calling case, just importing independent functions from other module, calling case means you call the run()
function in another module. Please help at the maintainer here, we could discuss on it.
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.
globals().get(interrupt_test)(test, params, vm, session)
Here is the "calling case by case", if you want to avoid it, this is what you should re-work on. Currently modification of moving those independent functions does not resolve anything.
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.
Though, all the functions are steps, e.g. vm.migrate(), vm.reboot(), those are steps instead of a 'case', the test scenario requires this combined steps, I don't see how we can implement it another way.
Hi @6-dehan @vivianQizhu Not sure if you are discussing this issue offline, please update the MR status when you have time, as it already pending 2 weeks.Thanks. |
Avoid case calling case, so add corresponding functions
ID: 2995
Signed-off-by: Dehan Meng [email protected]