Change in vdsm[master]: virt: Start all VM related stuff after switching to post-cop...

Milan Zamazal has uploaded a new change for review.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................

virt: Start all VM related stuff after switching to post-copy migration

On the migration destination, we perform running-VM actions only after
the migration completes.  But when the migration switches to post-copy
mode, the VM starts fully running on the destination.  So switching to
post-copy should be considered as the actual start of the VM and
appropriate actions should be performed.

This patch implements unblocking the start up actions after switching to
post-copy mode.

The only exception is VM status, which must still signal migration until
the migration completes.

Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Signed-off-by: Milan Zamazal <mzamazal(a)redhat.com&gt;
Bug-Url: https://bugzilla.redhat.com/1354343
---
M vdsm/virt/vm.py
1 file changed, 25 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/51/63551/8

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index e24a90a..ed60354 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -264,6 +264,7 @@
         self._migrationSourceThread = migration.SourceThread(self)
         self._kvmEnable = self.conf.get('kvmEnable', 'true')
         self._incomingMigrationFinished = threading.Event()
+        self._incoming_migration_vm_running = threading.Event()
         self.id = self.conf['vmId']
         self._volPrepareLock = threading.Lock()
         self._initTimePauseCode = None
@@ -540,6 +541,11 @@
             if ('migrationDest' in self.conf or 'restoreState' in
self.conf) \
                     and self.lastStatus != vmstatus.DOWN:
                 self._completeIncomingMigration()
+            if self.lastStatus == vmstatus.MIGRATION_DESTINATION:
+                # Waiting for post-copy migration to finish before we can
+                # change status to UP.
+                # Should we set a timeout here?
+                self._incomingMigrationFinished.wait()
 
             self.lastStatus = vmstatus.UP
             if self._initTimePauseCode:
@@ -2921,7 +2927,8 @@
             if self._needToWaitForMigrationToComplete():
                 usedTimeout = self._waitForUnderlyingMigration()
                 self._attachLibvirtDomainAfterMigration(
-                    self._incomingMigrationFinished.isSet(), usedTimeout)
+                    self._incoming_migration_vm_running.is_set(),
+                    usedTimeout)
             # else domain connection already established earlier
             self._domDependentInit()
             del self.conf['migrationDest']
@@ -2973,7 +2980,7 @@
     def _waitForUnderlyingMigration(self):
         timeout = config.getint('vars', 'migration_destination_timeout')
         self.log.debug("Waiting %s seconds for end of migration", timeout)
-        self._incomingMigrationFinished.wait(timeout)
+        self._incoming_migration_vm_running.wait(timeout)
         return timeout
 
     def _attachLibvirtDomainAfterMigration(self, migrationFinished, timeout):
@@ -2991,7 +2998,7 @@
                         raise MigrationError("Migration Error - Timed out "
                                              "(did not receive success "
                                              "event)")
-                self.log.debug("NOTE: incomingMigrationFinished event has "
+                self.log.debug("NOTE: incoming_migration_vm_running event has
"
                                "not been set and wait timed out after %d "
                                "seconds. Current VM state: %d, reason %d. "
                                "Continuing with VM initialization anyway.",
@@ -3920,7 +3927,7 @@
             self.log.info('Release VM resources')
             self.lastStatus = vmstatus.POWERING_DOWN
             # Terminate the VM's creation thread.
-            self._incomingMigrationFinished.set()
+            self._incoming_migration_vm_running.set()
             self.guestAgent.stop()
             if self._dom.connected:
                 result = self._destroyVm(gracefulAttempts)
@@ -4155,6 +4162,8 @@
                     pass
                 else:
                     hooks.after_vm_pause(domxml, self.conf)
+            elif detail == libvirt.VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED:
+                pass  # will be handled in a followup patch
 
         elif event == libvirt.VIR_DOMAIN_EVENT_RESUMED:
             self._setGuestCpuRunning(True)
@@ -4171,9 +4180,19 @@
                     pass
                 else:
                     hooks.after_vm_cont(domxml, self.conf)
-            elif (detail == libvirt.VIR_DOMAIN_EVENT_RESUMED_MIGRATED and
-                  self.lastStatus == vmstatus.MIGRATION_DESTINATION):
+            elif (self.lastStatus == vmstatus.MIGRATION_DESTINATION and
+                  detail == libvirt.VIR_DOMAIN_EVENT_RESUMED_MIGRATED):
+                self._incoming_migration_vm_running.set()
                 self._incomingMigrationFinished.set()
+            elif (self.lastStatus == vmstatus.MIGRATION_DESTINATION and
+                  detail == libvirt.VIR_DOMAIN_EVENT_RESUMED_POSTCOPY):
+                # When we enter post-copy mode, the VM starts actually
+                # running on the destination, so we should unblock the
+                # start up processing here.  The only exception is status,
+                # which must still signal incoming migration to not confuse
+                # Engine.
+                self._incoming_migration_vm_running.set()
+                self.log.info("Migration switched to post-copy mode")
 
     def _updateDevicesDomxmlCache(self, xml):
         """


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

gerrit-hooks has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


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.6',
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


Patch Set 8: Code-Review-1

(1 comment)

-1 because inline questions

https://gerrit.ovirt.org/#/c/63551/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 263:             self._lastStatus = vmstatus.WAIT_FOR_LAUNCH
Line 264:         self._migrationSourceThread = migration.SourceThread(self)
Line 265:         self._kvmEnable = self.conf.get('kvmEnable', 'true')
Line 266:         self._incomingMigrationFinished = threading.Event()
Line 267:         self._incoming_migration_vm_running = threading.Event()
Why can't we just do incomingMigrationFinished.set() earlier?
I read the commit message reference about the status; I wonder if we can't just
unblock the VM here and run with status == MIGRATION_DESTINATION, instead of keeping the
creation thread around

Once libvirt delivers the event, we can just change state.

Would the above work?
Line 268:         self.id = self.conf['vmId']
Line 269:         self._volPrepareLock = threading.Lock()
Line 270:         self._initTimePauseCode = None
Line 271:         self._initTimeRTC = int(self.conf.get('timeOffset', 0))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Milan Zamazal has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/63551/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 263:             self._lastStatus = vmstatus.WAIT_FOR_LAUNCH
Line 264:         self._migrationSourceThread = migration.SourceThread(self)
Line 265:         self._kvmEnable = self.conf.get('kvmEnable', 'true')
Line 266:         self._incomingMigrationFinished = threading.Event()
Line 267:         self._incoming_migration_vm_running = threading.Event()
>  Why can't we just do incomingMigrationFinished.set() earlier?
Well, what (except for the status and the corresponding event) will change if we
simply finish the creation thread on switching to post-copy? I'd be worried about
unsetting self.recovery, but it seems to be safe. Then perhaps raising exceptions in the
rest of the creation thread, but there should be no exception in that path, right? And
finally there is the incoming_migrations semaphore, it can be handled together with
status. So it might work.

But the primary question is: What do we gain by exiting the creation thread early or what
do we lose by not exiting it early? Is it just to avoid introducing another
synchronization variable (for the price of slightly more complicated handling of the
status and the semaphore) or anything else?
Line 268:         self.id = self.conf['vmId']
Line 269:         self._volPrepareLock = threading.Lock()
Line 270:         self._initTimePauseCode = None
Line 271:         self._initTimeRTC = int(self.conf.get('timeOffset', 0))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


Patch Set 9:

* Update Tracker::#1354343::OK, status: POST
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::#1354343::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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

changing score - it was -1 for visibility only.
Looks ok, but I need to read it carefully again before to give +2 because of the new
synchronization steps.
Since I agree with the basic idea, provisional +1

https://gerrit.ovirt.org/#/c/63551/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 263:         else:
Line 264:             self._lastStatus = vmstatus.WAIT_FOR_LAUNCH
Line 265:         self._migrationSourceThread = migration.SourceThread(self)
Line 266:         self._kvmEnable = self.conf.get('kvmEnable', 'true')
Line 267:         self._incomingMigrationFinished = threading.Event()
>  Well, what (except for the status and the corresponding event) will
> change  
It's the tradeoff between complicating the existing flows and adding
more synchronization variables. I've no strong feelings however.
Line 268:         self._incoming_migration_vm_running = threading.Event()
Line 269:         self.id = self.conf['vmId']
Line 270:         self._volPrepareLock = threading.Lock()
Line 271:         self._initTimePauseCode = None


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has submitted this change and it was merged.

Change subject: virt: Start all VM related stuff after switching to post-copy migration
......................................................................


virt: Start all VM related stuff after switching to post-copy migration

On the migration destination, we perform running-VM actions only after
the migration completes.  But when the migration switches to post-copy
mode, the VM starts fully running on the destination.  So switching to
post-copy should be considered as the actual start of the VM and
appropriate actions should be performed.

This patch implements unblocking the start up actions after switching to
post-copy mode.

The only exception is VM status, which must still signal migration until
the migration completes.

Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Signed-off-by: Milan Zamazal <mzamazal(a)redhat.com&gt;
Bug-Url: https://bugzilla.redhat.com/1354343
---
M vdsm/virt/vm.py
1 file changed, 25 insertions(+), 6 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, approved
  Milan Zamazal: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8b17e58f63cbd9db09e4420871a562eaa0b3f3d
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(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: Milan Zamazal <mzamazal(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;