Francesco Romani has uploaded a new change for review.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
vm: ignore more errors in isDomainReadyForCommands
Due to race on domain shutdown, and after migration end, we may receive errors from libvirt which makes the Vm.isDomainReadyForCommands() method fail with scary stacktrace.
This patches makes the method cope with VIR_ERR_OPERATION_UNSUPPORTED. In the context of this method, this error can come only if the domain is shutting down, so it is safe to swallow it in this case.
Please note that this is admittedly a paliative fix, because the real fix may only happen inside libvirt.
Change-Id: I89ff61e0cd3bbb977833897c250ea337c86b9f80 Signed-off-by: Francesco Romani fromani@redhat.com --- M vdsm/virt/vm.py 1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/31/65131/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 0a767f6..f210373 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -2887,8 +2887,11 @@ # to avoid racy checks. return False except libvirt.libvirtError as e: - if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: - # same as NotConnectedError above: possible race on shutdown + IGNORED_ERRORS = ( + libvirt.VIR_ERR_NO_DOMAIN, # race on shutdown + libvirt.VIR_ERR_OPERATION_INVALID, # race on migration end + ) + if e.get_error_code() in IGNORED_ERRORS: return False else: raise
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Milan Zamazal has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 1:
(2 comments)
https://gerrit.ovirt.org/#/c/65131/1//COMMIT_MSG Commit Message:
PS1, Line 13: patches patch
Line 14: In the context of this method, this error can come only if the Line 15: domain is shutting down, so it is safe to swallow it in this case. Line 16: Line 17: Please note that this is admittedly a paliative fix, because the Line 18: real fix may only happen inside libvirt. What should be the real fix? Line 19: Line 20: Change-Id: I89ff61e0cd3bbb977833897c250ea337c86b9f80
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 2:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Martin Polednik has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://gerrit.ovirt.org/#/c/65131/2//COMMIT_MSG Commit Message:
PS2, Line 17: ussue issue?
https://gerrit.ovirt.org/#/c/65131/2/vdsm/virt/vm.py File vdsm/virt/vm.py:
PS2, Line 2898: IGNORED_ERRORS I'm not entirely sure about a) case of the identifier and b) it's purpose.
Really minor, _up to you_ stuff though.
Milan Zamazal has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://gerrit.ovirt.org/#/c/65131/2//COMMIT_MSG Commit Message:
PS2, Line 17: one You probably mean "a" here ("one" confused me a bit)?
PS2, Line 18: domain domains
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 3:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 2:
(2 comments)
https://gerrit.ovirt.org/#/c/65131/2//COMMIT_MSG Commit Message:
PS2, Line 17: one
You probably mean "a" here ("one" confused me a bit)?
yes to both :)
https://gerrit.ovirt.org/#/c/65131/2/vdsm/virt/vm.py File vdsm/virt/vm.py:
PS2, Line 2898: IGNORED_ERRORS
I'm not entirely sure about
Just an attempt to make the code looks nicer, but not so succesfully :\
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 4:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/65131/2//COMMIT_MSG Commit Message:
PS2, Line 18: domain
domains
Done
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 5:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Jenkins CI has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 5: Continuous-Integration+1
Propagate review hook: Continuous Integration value inherited from patch 3
Jenkins CI has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 5:
Propagate review hook: Continuous Integration value inherited from patch 4
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 6:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 7:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 7:
The test failure is interesting but unrelated
00:15:56.269 ====================================================================== 00:15:56.269 ERROR: test_schedule_after(<function monotonic_time at 0x7fc653ec89d8>) (scheduleTests.SchedulerTests) 00:15:56.269 ---------------------------------------------------------------------- 00:15:56.269 Traceback (most recent call last): 00:15:56.269 File "/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/tests/testlib.py", line 86, in wrapper 00:15:56.269 return f(self, *args) 00:15:56.269 File "/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/tests/scheduleTests.py", line 59, in test_schedule_after 00:15:56.269 self.assertTrue(deadline <= task1.call_time) 00:15:56.270 TypeError: unorderable types: float() <= NoneType() 00:15:56.270 -------------------- >> begin captured logging << -------------------- 00:15:56.270 2016-10-20 09:23:51,221 DEBUG (MainThread) [Scheduler] Starting scheduler Scheduler (schedule:98) 00:15:56.270 2016-10-20 09:23:51,227 DEBUG (Scheduler) [Scheduler] started (schedule:140) 00:15:56.270 2016-10-20 09:23:51,625 DEBUG (MainThread) [Scheduler] Stopping scheduler Scheduler (schedule:110) 00:15:56.270 2016-10-20 09:23:51,955 DEBUG (Scheduler) [Scheduler] stopped (schedule:14
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 8:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 9:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Milan Zamazal has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 9: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 10:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Martin Polednik has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 10: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 11:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 12:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 13:
* Update Tracker::#1382578::IGNORE, not relevant for Red Hat classification * Check Bug-Url::IGNORE, not relevant for 'Red Hat' classification * Check Public Bug::#1382578::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 13: Verified+1
copied score
Dan Kenigsberg has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 13: Code-Review+2
raising
Dan Kenigsberg has submitted this change and it was merged.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
vm: ignore more errors in isDomainReadyForCommands
Due to race on domain shutdown, and after migration end, we may receive errors from libvirt which makes the Vm.isDomainReadyForCommands() method fail with scary stacktrace.
This patch makes the method cope with VIR_ERR_OPERATION_UNSUPPORTED. In the context of this method, this error can come only if the domain is shutting down, so it is safe to swallow it in this case.
The real, big issue is how to properly handle the shutdown of a domain. Currently, we have to deal with domains which are partially shut down, so they are reported by libvirt and can return error (like VIR_ERR_OPERATION_UNSUPPORTED) to methods which normally succeed.
One way to solve this is to make the shutdown (more) atomic: either a domain is completely operative, or it is gone. It is still to be decided if this could be done without libvirt changes.
Change-Id: I89ff61e0cd3bbb977833897c250ea337c86b9f80 Backport-To: 4.0 Bug-Url: https://bugzilla.redhat.com/1382578 Signed-off-by: Francesco Romani fromani@redhat.com Reviewed-on: https://gerrit.ovirt.org/65131 Reviewed-by: Milan Zamazal mzamazal@redhat.com Reviewed-by: Martin Polednik mpolednik@redhat.com Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M tests/vmOperationsTests.py M vdsm/virt/vm.py 2 files changed, 11 insertions(+), 5 deletions(-)
Approvals: Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Francesco Romani: Verified Martin Polednik: Looks good to me, but someone else must approve Milan Zamazal: Looks good to me, but someone else must approve
gerrit-hooks has posted comments on this change.
Change subject: vm: ignore more errors in isDomainReadyForCommands ......................................................................
Patch Set 14:
* update_tracker: OK * Set MODIFIED::bug 1382578::::#1382578::IGNORE, skipping for branch 'master'
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Dan Kenigsberg -
fromani＠redhat.com -
Jenkins CI RO -
Martin Polednik -
mzamazal＠redhat.com