Change in vdsm[master]: vdsm: new verb: migration cancel

Shahar Havivi has uploaded a new change for review.

Change subject: vdsm: new verb: migration cancel
......................................................................

vdsm: new verb: migration cancel

the verb migrateCancel will take vmid and try to stop migration,
if there is no migration in progress it will return error.

Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
---
M vdsm/clientIF.py
M vdsm/define.py
M vdsm/libvirtvm.py
M vdsm/vm.py
4 files changed, 23 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/80/480/1
--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 442:     def stop(self):
apparently, this is not good enough, if migration thread is still in migrationCreate (line
408 above) or anywhere before _dom.migrateToURI2 takes effect.

I do not see a way to avoid synchronizing between MigrationSourceThread and the new
migrationCancel verb.

(thanks Peter for helping to find this issue)

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 6: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 442:     def stop(self):
How do you think we can do that?
I try to put a lock but self._vm._dom.migrateToURI2() is a blocking method, which mean
that the cancel is irrelevant

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 7: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 7: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 436:                 self._migCancelEvt1.clear()
The race here has to be documented with a big FIXME (if it survives the next version of
this patch)

Line 451:             self._vm._dom.abortJob()
how about always calling abortJob, but ignoring the exception if we are in
migrationPreparation? It is much better to fail migrateCancel, than to wrongly report its
success.

....................................................
File vdsm/vm.py
Line 109:         self._migCancelEvt1 = threading.Event()
the names are really bad :-(

how about _preparingMigrationEvt and and _migrationCanceledEvt?

btw, no one waits on these events, so nothing compels us from using plain booleans
instead.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 7: (3 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 436:                 self._migCancelEvt1.clear()
Done

Line 451:             self._vm._dom.abortJob()
Done

....................................................
File vdsm/vm.py
Line 109:         self._migCancelEvt1 = threading.Event()
Done

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Peter V. Saveliev has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 9: Verified; Looks good to me, but someone else must approve

verified, works for me.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: new verb: migration cancel
......................................................................


new verb: migration cancel

the verb migrateCancel will take vmid and try to stop migration,
if there is no migration in progress it will return error.

when migrateCancel stop the migration progress, the migrate verb will
return error "Migration canceled"

Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
---
M vdsm/clientIF.py
M vdsm/define.py
M vdsm/libvirtvm.py
M vdsm/vm.py
4 files changed, 59 insertions(+), 5 deletions(-)

Approvals:
  Shahar Havivi: Verified
  Dan Kenigsberg: Looks good to me, approved
  Peter V. Saveliev: Verified; Looks good to me, but someone else must approve


--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 9: Looks good to me, approved

I'm not happy about this inherently racy code, but I do not have a good idea to fix
it.. Let's put it in, and tweak it in the future.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 9: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 8: Looks good to me, approved

I'm not happy about this inherently racy code, but I do not have a good idea to fix
it.. Let's put it in, and tweak it in the future.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 6: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 6: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 6: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 5: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/vm.py
Line 235:                         'message': 'Migration canceled'}}
exception is not re-raised here, _recover is not called, and nothing resets Vm state.

this block should probably moved out to line 240 below, or better be moved into
_startUnderlyingMigration() with a reraise.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 5: Fails

unfortunately the Vm object stays in "migration source" state after
migrateCancel (instead of returning back to Up)

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 5: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 4: I would prefer that you didn't submit this

Shahar, why did you uploaded the same patch set?

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 4: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 3: (4 inline comments)

....................................................
Commit Message
Line 7: vdsm: new verb: migration cancel
Done

....................................................
File vdsm/define.py
Line 45:             'migCancelErr': {'status': {'code':45,
'message': 'Migration not in progress'}},
Done

....................................................
File vdsm/vm.py
Line 229:                 # yes ugly searching in the error message,
Done

Line 233:                         e.get_error_message().find('canceled by client')
> 0:
Done

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(1 inline comment)

Should we change VM statuses from 'Migration Source' to prior value after
migration cancellation?

....................................................
File vdsm/define.py
Line 45:             'migCancelErr': {'status': {'code':45,
'message': 'Migration not in progress'}},
I am already using error codes 45,46 for hotplug.
I know that first merged patch should win, but because hotplug patch already in gerrit and
to avoid unnecessary rebases in the feature please consider to change error code to 47

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 3: (1 inline comment)

....................................................
Commit Message
Line 7: vdsm: new verb: migration cancel
oh, no need for the vdsm: prefix, we know it is vdsm git.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(2 inline comments)

very minor comments, generally looking good.

....................................................
File vdsm/vm.py
Line 229:                 # yes ugly searching in the error message,
Putting a TODO marker here may be helpful

Line 233:                         e.get_error_message().find('canceled by client')
> 0:
I find

 'canceled by client' in e.get_error_message()

more elegant. and you?

P.S I would very much prefer new code with lines shorter than 80 chars. It is easier for
me to review.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 3: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 2: (3 inline comments)

....................................................
Commit Message
Line 10: if there is no migration in progress it will return error.
Done

....................................................
File vdsm/define.py
Line 32:             'migCancelErr': {'status': {'code':12,
'message': 'Migration not in progress'}},
Done

....................................................
File vdsm/vm.py
Line 890:                 return errCode['migCancelErr']
Done

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

....................................................
Commit Message
Line 10: if there is no migration in progress it will return error.
please mention what would migrateStatus return after migration has been canceled.

....................................................
File vdsm/define.py
Line 32:             'migCancelErr': {'status': {'code':12,
'message': 'Migration not in progress'}},
using the same errorcode twice is impolite

....................................................
File vdsm/vm.py
Line 890:                 return errCode['migCancelErr']
error content is swallowed here. you may simply re-raise the exception.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 2: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: vdsm: new verb: migration cancel
......................................................................


Patch Set 1: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Peter V. Saveliev has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 8: Verified; Looks good to me, but someone else must approve

Works for me.

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Change in vdsm[master]: new verb: migration cancel

Shahar Havivi has posted comments on this change.

Change subject: new verb: migration cancel
......................................................................


Patch Set 8: Verified

--
To view, visit http://gerrit.ovirt.org/480
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f503c444c4ca1469c18480d6ca29fba8f011d4c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;