Francesco Romani has posted comments on this change.
Change subject: virt: Distinguish between switching to and entering post-copy migration ......................................................................
Patch Set 7: Code-Review-1
(3 comments)
-1 for discussion
https://gerrit.ovirt.org/#/c/64143/7//COMMIT_MSG Commit Message:
Line 15: request phase and the post-copy switch phase. For instance, we should Line 16: block canceling migrations very early to avoid timing issues and Line 17: possibly canceling a post-copy migration. On the other hand we should Line 18: report a true VM state to Engine, so we can report post-copy only after Line 19: the VM actually enters it. do we need two separate variables or can we turn the boolean in one state attribute? (maybe post_copy_phase?)
I think in this case, given the rationale above, the state variable makes a bit more sense. Furthermore, in general I'm against attributes proliferation.
We could do like this:
class PostCopyPhase: # this should be a python >= 3.4 enum, really DISABLED = 0 # aka precopy REQUESTED = 1 RUNNING = 2
or something like it. What do you think? Line 20: Line 21: Change-Id: I88dddeab84a609ebd3d5d9139724adff9cdb1352 Line 22: Signed-off-by: Milan Zamazal mzamazal@redhat.com
https://gerrit.ovirt.org/#/c/64143/7/vdsm/virt/migration.py File vdsm/virt/migration.py:
PS7, Line 811: if ret >= 0: : self._post_copy_initiated = True not related to this patch, but I realized just now. Why do we need more state variables in the migration helper threads? Couldn't we just use one (or few) variables in the vm class?
In general, I have the feeling we are duplicating the state, which makes the code more fragile.
(see "here" in vm.py)
https://gerrit.ovirt.org/#/c/64143/7/vdsm/virt/vm.py File vdsm/virt/vm.py:
PS7, Line 1436: if self._migrationSourceThread.post_copy_initiated or \ : self._in_post_copy: here