Change in vdsm[master]: migration: avoid stacktrace on known errors

Francesco Romani has uploaded a new change for review.

Change subject: migration: avoid stacktrace on known errors
......................................................................

migration: avoid stacktrace on known errors

In migration code, if VM.migrationCreate() fails on destination host,
we raise RuntimeError to abort the normal flow.
However, in the recovery flow, we inconditionally log the stacktrace
of the exception we catched.

This is good for troubleshooiting, but it is a little pointless in
this specific use case: we know already and precisely what failed.

This patch adds a special-purpose exception for this specific, yet
important, failure scenario, and avoids the stacktrace only when
catching this exception.

Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Signed-off-by: Francesco Romani <fromani(a)redhat.com&gt;
---
M vdsm/virt/migration.py
1 file changed, 12 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/63/43063/1

diff --git a/vdsm/virt/migration.py b/vdsm/virt/migration.py
index b8e155f..7ea8753 100644
--- a/vdsm/virt/migration.py
+++ b/vdsm/virt/migration.py
@@ -52,6 +52,12 @@
 VIR_MIGRATE_PARAM_GRAPHICS_URI = 'graphics_uri'
 
 
+class MigrationDestinationSetupError(RuntimeError):
+    """
+    Failed to create migration destination VM.
+    """
+
+
 class SourceThread(threading.Thread):
     """
     A thread that takes care of migration on the source vdsm.
@@ -295,6 +301,9 @@
                 if '_migrationParams' in self._vm.conf:
                     del self._vm.conf['_migrationParams']
                 SourceThread._ongoingMigrations.release()
+        except MigrationDestinationSetupError as e:
+            self._recover(str(e))
+            # we know what happened, no need to dump hollow stack trace
         except Exception as e:
             self._recover(str(e))
             self.log.exception("Failed to migrate")
@@ -326,8 +335,9 @@
 
             if result['status']['code']:
                 self.status = result
-                raise RuntimeError('migration destination error: ' +
-                                   result['status']['message'])
+                raise MigrationDestinationSetupError(
+                    'migration destination error: ' +
+                    result['status']['message'])
             if config.getboolean('vars', 'ssl'):
                 transport = 'tls'
             else:


-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


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.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 2:

* 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.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 2: Verified+1

re-verified the same way as 42970, both on happy path and with failure injected on dest
side.

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 2: Code-Review+1

The docstring language sounds like it could be improved, but I'm not sure how. Minor
thing.

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: migration: avoid stacktrace on known errors
......................................................................


migration: avoid stacktrace on known errors

In migration code, if VM.migrationCreate() fails on destination host,
we raise RuntimeError to abort the normal flow.
However, in the recovery flow, we inconditionally log the stacktrace
of the exception we catched.

This is good for troubleshooiting, but it is a little pointless in
this specific use case: we know already and precisely what failed.

This patch adds a special-purpose exception for this specific, yet
important, failure scenario, and avoids the stacktrace only when
catching this exception.

Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Signed-off-by: Francesco Romani <fromani(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/43063
Reviewed-by: Martin Polednik <mpolednik(a)redhat.com&gt;
Continuous-Integration: Jenkins CI
Reviewed-by: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M vdsm/virt/migration.py
1 file changed, 12 insertions(+), 2 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Vinzenz Feenstra: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified
  Martin Polednik: Looks good to me, but someone else must approve



-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org

automation(a)ovirt.org has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/43063/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 302:                     del self._vm.conf['_migrationParams']
Line 303:                 SourceThread._ongoingMigrations.release()
Line 304:         except MigrationDestinationSetupError as e:
Line 305:             self._recover(str(e))
Line 306:             # we know what happened, no need to dump hollow stack trace
But we want a error log about the failure - right? Or at least an info log explaining that
migration failed and we are recovering.
Line 307:         except Exception as e:
Line 308:             self._recover(str(e))
Line 309:             self.log.exception("Failed to migrate")
Line 310: 


-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: migration: avoid stacktrace on known errors
......................................................................


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/43063/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 302:                     del self._vm.conf['_migrationParams']
Line 303:                 SourceThread._ongoingMigrations.release()
Line 304:         except MigrationDestinationSetupError as e:
Line 305:             self._recover(str(e))
Line 306:             # we know what happened, no need to dump hollow stack trace
>  But we want a error log about the failure - right? Or at least an
> info log  
We do, and that happens already inside recover()
Line 307:         except Exception as e:
Line 308:             self._recover(str(e))
Line 309:             self.log.exception("Failed to migrate")
Line 310: 


-- 
To view, visit https://gerrit.ovirt.org/43063
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib35cf08a305018912991a47d0de645395a177e9e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes