Change in vdsm[master]: vm: abort and report error if domain destroy fails

Francesco Romani has uploaded a new change for review.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

vm: abort and report error if domain destroy fails

The call of vm._dom.destroy() is not expected to fail.
However, if the failure is not impossible, and in
that case VDSM should detect that and report the
error instead of blindly go ahead.

This patch let releaseVm() bail out with an error
as soon as possible if destroy() fails.

Change-Id: Id6d5819321cd0cb28aa43678dc65ee83c215c2d7
Signed-off-by: Francesco Romani 
---
M vdsm/virt/vm.py
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/28025/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index b73d84d..e299654 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -4324,6 +4324,7 @@
             except libvirt.libvirtError as e:
                 self.log.warning("Failed to destroy VM '%s'",
                                  self.conf['vmId'])
+                return errCode['destroyErr']

if not self.cif.mom:
                 self.cif.ksmMonitor.adjust()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9184/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9327/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8396/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9140/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9925/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/807/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10080/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5007/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3164/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9227/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10011/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/842/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10166/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5093/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3250/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 4:

rebased

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9293/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10077/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/862/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10233/ : ABORTED

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5159/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3317/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 4: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/28025/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4573:                             self._dom.destroy()
Line 4574:                         except libvirt.libvirtError as e:
Line 4575:                             self.log.warning("Failed to destroy VM '%s'",
Line 4576:                                              self.conf['vmId'], exc_info=True)
Line 4577:                             return errCode['destroyErr']
also the "else" branch of line 4570 needs it. remove 8 spaces and I'm fine with this patch.
Line 4578: 
Line 4579:             if not self.cif.mom:
Line 4580:                 self.cif.ksmMonitor.adjust()
Line 4581:             self._cleanup()

Francesco Romani has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/28025/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4573:                             self._dom.destroy()
Line 4574:                         except libvirt.libvirtError as e:
Line 4575:                             self.log.warning("Failed to destroy VM '%s'",
Line 4576:                                              self.conf['vmId'], exc_info=True)
Line 4577:                             return errCode['destroyErr']
> also the "else" branch of line 4570 needs it. remove 8 spaces and I'm fine

not really I'm afraid, because if destroy() at line 4573 above succeeds, we want to go ahead with releaseVm. I need a couple of return values here.

I'll save the beautification for 28026 and keeps this patch really minimal at the price of some added ugliness.
Line 4578: 
Line 4579:             if not self.cif.mom:
Line 4580:                 self.cif.ksmMonitor.adjust()
Line 4581:             self._cleanup()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9376/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10160/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/898/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10316/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5242/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3400/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 5:

Verification:
verified the happy path together with 28023 and 28024

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 5: Code-Review+2

right.. a simple unindent is not enough.

Francesco Romani has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 5:

I'm having hard time to untangle the exception flow without rearranging the code like I did in 28026, so I'm going to squash this patch in 28026.

Francesco Romani has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 6: Verified+1

Squashed with former 28026 (now abandoned) to untangle the messy flow in case of destroy() failing.

Verified also the error path by manually injecting exceptions, after a few failed attempt to let QEMU hang in the destroy path.
Verified with 28023 and 28024

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 6:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9396/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10180/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/908/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10336/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5262/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3420/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 6: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

vm: abort and report error if domain destroy fails

The call of vm._dom.destroy() is not expected to fail.
However, if the failure is not impossible, and in
that case VDSM should detect that and report the
error instead of blindly go ahead.

This patch let releaseVm() bail out with an error
as soon as possible if destroy() fails.

Due to entanglement of the error floes, the more correct
way to handle this error conditions is also the nicer,
by splitting the destroy attempts into submethods.

Change-Id: Id6d5819321cd0cb28aa43678dc65ee83c215c2d7
Signed-off-by: Francesco Romani 
Reviewed-on: http://gerrit.ovirt.org/28025
Reviewed-by: Dan Kenigsberg 
---
M vdsm/virt/vm.py
1 file changed, 23 insertions(+), 14 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: abort and report error if domain destroy fails
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1475/ : SUCCESS