Change in vdsm[master]: vm: small cleanup of _completeIncomingMigration()

Martin Betak has uploaded a new change for review.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

vm: small cleanup of _completeIncomingMigration()

Extracted the check for recovery completion into separate method to simplify
consequent migration enhancements.

Change-Id: Ia1afb406e3e2ca7b37a621fc1bcc8bc1bf37031b
Wiki: http://www.ovirt.org/Features/Migration_Enhancements
Signed-off-by: Martin Betak 
---
M vdsm/virt/vm.py
1 file changed, 15 insertions(+), 10 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/86/47086/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index b8ec3d4..39c0303 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2753,16 +2753,7 @@
             hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf,
                                        {'FROM_SNAPSHOT': fromSnapshot})
         elif 'migrationDest' in self.conf:
-            waitMigration = True
-            if self.recovering:
-                try:
-                    if self._isDomainRunning():
-                        waitMigration = False
-                        self.log.info('migration completed while recovering!')
-                except libvirt.libvirtError:
-                    self.log.exception('migration failed while recovering!')
-                    raise MigrationError()
-            if waitMigration:
+            if not self._checkMigrationCompletedDuringRecovery():
                 usedTimeout = self._waitForUnderlyingMigration()
                 self._attachLibvirtDomainAfterMigration(
                     self._incomingMigrationFinished.isSet(), usedTimeout)
@@ -2784,6 +2775,20 @@
         self.saveState()
         self.log.info("End of migration")

+    def _checkMigrationCompletedDuringRecovery(self):
+        if not self.recovering:
+            return False
+
+        try:
+            if self._isDomainRunning():
+                self.log.info('migration completed while recovering!')
+                return False
+            else:
+                return True
+        except libvirt.libvirtError:
+            self.log.exception('migration failed while recovering!')
+            raise MigrationError()
+
     def _waitForUnderlyingMigration(self):
         timeout = config.getint('vars', 'migration_destination_timeout')
         self.log.debug("Waiting %s seconds for end of migration", timeout)

[e-mail] has posted comments on this change.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

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'])

Francesco Romani has posted comments on this change.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

Patch Set 1:

(1 comment)

I like this direction but we need to clarify a bit the code here.

https://gerrit.ovirt.org/#/c/47086/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2752:             fromSnapshot = self.conf.pop('restoreFromSnapshot', False)
Line 2753:             hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf,
Line 2754:                                        {'FROM_SNAPSHOT': fromSnapshot})
Line 2755:         elif 'migrationDest' in self.conf:
Line 2756:             if not self._checkMigrationCompletedDuringRecovery():
let's find a better (= more expressive) name
maybe

_needToWaitForMigrationToComplete

or

_migrationStillNotComplete

or something along these lines
Line 2757:                 usedTimeout = self._waitForUnderlyingMigration()
Line 2758:                 self._attachLibvirtDomainAfterMigration(
Line 2759:                     self._incomingMigrationFinished.isSet(), usedTimeout)
Line 2760:             # else domain connection already established earlier

Francesco Romani has posted comments on this change.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

Patch Set 1: Code-Review-1

(2 comments)

-1 for visibility

https://gerrit.ovirt.org/#/c/47086/1//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-10-07 14:14:39 +0200
Line 4: Commit:     Martin Betak 
Line 5: CommitDate: 2015-10-07 16:16:21 +0200
Line 6: 
Line 7: vm: small cleanup of _completeIncomingMigration()
let's have a more descriptive subject line, something like

migration: extract helper to check if migration completed

or something like this.
Line 8: 
Line 9: Extracted the check for recovery completion into separate method to simplify
Line 10: consequent migration enhancements.
Line 11:

https://gerrit.ovirt.org/#/c/47086/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2776:         self.log.info("End of migration")
Line 2777: 
Line 2778:     def _checkMigrationCompletedDuringRecovery(self):
Line 2779:         if not self.recovering:
Line 2780:             return False
you can just extract this snippet into a function without changing the logic. Maybe a little bit clearer.
Line 2781: 
Line 2782:         try:
Line 2783:             if self._isDomainRunning():
Line 2784:                 self.log.info('migration completed while recovering!')

Martin Betak has posted comments on this change.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47086/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2752:             fromSnapshot = self.conf.pop('restoreFromSnapshot', False)
Line 2753:             hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf,
Line 2754:                                        {'FROM_SNAPSHOT': fromSnapshot})
Line 2755:         elif 'migrationDest' in self.conf:
Line 2756:             if not self._checkMigrationCompletedDuringRecovery():
> let's find a better (= more expressive) name

Done
Line 2757:                 usedTimeout = self._waitForUnderlyingMigration()
Line 2758:                 self._attachLibvirtDomainAfterMigration(
Line 2759:                     self._incomingMigrationFinished.isSet(), usedTimeout)
Line 2760:             # else domain connection already established earlier

Martin Betak has posted comments on this change.

Change subject: vm: small cleanup of _completeIncomingMigration()
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47086/1//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-10-07 14:14:39 +0200
Line 4: Commit:     Martin Betak 
Line 5: CommitDate: 2015-10-07 16:16:21 +0200
Line 6: 
Line 7: vm: small cleanup of _completeIncomingMigration()
> let's have a more descriptive subject line, something like

Done
Line 8: 
Line 9: Extracted the check for recovery completion into separate method to simplify
Line 10: consequent migration enhancements.
Line 11:

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

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'])

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 3:

* 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'])

Change in vdsm[master]: vm: extract migration completion check

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/47086/3//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: vm: extract migration completion check
Line 8: 
Line 9: Extracted the check for recovery completion into separate method to simplify
Line 10: consequent migration enhancements.
Not currently used in the patch series, is it still relevant?
Line 11: 
Line 12: Change-Id: Ia1afb406e3e2ca7b37a621fc1bcc8bc1bf37031b
Line 13: Wiki: http://www.ovirt.org/Features/Migration_Enhancements

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 4:

* 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'])

Change in vdsm[master]: vm: extract migration completion check

Martin Betak has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/47086/3//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: vm: extract migration completion check
Line 8: 
Line 9: Extracted the check for recovery completion into separate method to simplify
Line 10: consequent migration enhancements.
> Not currently used in the patch series, is it still relevant?

yes, it must have dropped out during patch splitting. fixed
Line 11: 
Line 12: Change-Id: Ia1afb406e3e2ca7b37a621fc1bcc8bc1bf37031b
Line 13: Wiki: http://www.ovirt.org/Features/Migration_Enhancements

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 5:

* 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'])

Change in vdsm[master]: vm: extract migration completion check

Martin Betak has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 5: Verified+1

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 6:

* 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'])

Change in vdsm[master]: vm: extract migration completion check

Francesco Romani has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 6: Code-Review-1

(3 comments)

I'm fine with this concept, makes the code nicer. -1 for visibility, see inline comments.

https://gerrit.ovirt.org/#/c/47086/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2775:             del self.conf['username']
Line 2776:         self.saveState()
Line 2777:         self.log.info("End of migration")
Line 2778: 
Line 2779:     def _needToWaitForMigrationToComplete(self):
ok, this helper can indeed be extracted and I'm Ok with this move.
Line 2780:         if not self.recovering:
Line 2781:             return True
Line 2782: 
Line 2783:         try:

Line 2776:         self.saveState()
Line 2777:         self.log.info("End of migration")
Line 2778: 
Line 2779:     def _needToWaitForMigrationToComplete(self):
Line 2780:         if not self.recovering:
please add a comment to explain this early return, something like

# if it is not recovering, we are in the base flow, so we need to wait for the migration to complete
Line 2781:             return True
Line 2782: 
Line 2783:         try:
Line 2784:             if self._isDomainRunning():

Line 2780:         if not self.recovering:
Line 2781:             return True
Line 2782: 
Line 2783:         try:
Line 2784:             if self._isDomainRunning():
Since we are factoring out this code with some changes, it is a bit nicer and more consistent this way:

try:
      if not self._isDomainRunning():
          return True
  except ...
       ...
  else:
      self.log.info(...)
      return False

otherwise, you can take the opposite direction and just copy/paste the above code into a new helper.
Whatever fits you best.
Line 2785:                 self.log.info('migration completed while recovering!')
Line 2786:                 return False
Line 2787:             else:
Line 2788:                 return True

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 7:

* 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'])

Change in vdsm[master]: vm: extract migration completion check

Martin Betak has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/47086/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2776:         self.saveState()
Line 2777:         self.log.info("End of migration")
Line 2778: 
Line 2779:     def _needToWaitForMigrationToComplete(self):
Line 2780:         if not self.recovering:
> please add a comment to explain this early return, something like

Done
Line 2781:             return True
Line 2782: 
Line 2783:         try:
Line 2784:             if self._isDomainRunning():

Line 2780:         if not self.recovering:
Line 2781:             return True
Line 2782: 
Line 2783:         try:
Line 2784:             if self._isDomainRunning():
> Since we are factoring out this code with some changes, it is a bit nicer a

Done, thanks :-)
Line 2785:                 self.log.info('migration completed while recovering!')
Line 2786:                 return False
Line 2787:             else:
Line 2788:                 return True

Change in vdsm[master]: vm: extract migration completion check

Martin Betak has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 7: Verified+1

Change in vdsm[master]: vm: extract migration completion check

Francesco Romani has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 7: Code-Review+2

Change in vdsm[master]: vm: extract migration completion check

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/47086/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2775:             del self.conf['username']
Line 2776:         self.saveState()
Line 2777:         self.log.info("End of migration")
Line 2778: 
Line 2779:     def _needToWaitForMigrationToComplete(self):
please consider pep_8_names for future new functions.
Line 2780:         if not self.recovering:
Line 2781:             # if not recovering, we are in a base flow and need
Line 2782:             # to wait for migration to complete
Line 2783:             return True

Change in vdsm[master]: vm: extract migration completion check

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: extract migration completion check
......................................................................

vm: extract migration completion check

Extracted the check for recovery completion into separate method to simplify
consequent migration enhancements.

Change-Id: Ia1afb406e3e2ca7b37a621fc1bcc8bc1bf37031b
Wiki: http://www.ovirt.org/Features/Migration_Enhancements
Signed-off-by: Martin Betak 
Reviewed-on: https://gerrit.ovirt.org/47086
Continuous-Integration: Jenkins CI
Reviewed-by: Francesco Romani 
---
M vdsm/virt/vm.py
1 file changed, 18 insertions(+), 10 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Martin Betak: Verified
  Francesco Romani: Looks good to me, approved

Change in vdsm[master]: vm: extract migration completion check

[e-mail] has posted comments on this change.

Change subject: vm: extract migration completion check
......................................................................

Patch Set 8:

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