Change in vdsm[master]: setVmTicket: add before and after hooks

Shahar Havivi has uploaded a new change for review.

Change subject: setVmTicket: add before and after hooks
......................................................................

setVmTicket: add before and after hooks

setVmTicket get two new additional params: user-name and user-id, for
the benefit of the hook scripts.

Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Signed-off-by: Shahar Havivi 
---
M vdsm.spec.in
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/hooks.py
M vdsm/libvirtvm.py
M vdsm/vdsmd.8.in
M vdsm_cli/vdsClient.py
M vdsm_hooks/Makefile.am
8 files changed, 29 insertions(+), 11 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi

Ayal Baron has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

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

(3 inline comments)

Just a few minor comments.

....................................................
Commit Message
Line 10: the benefit of the hook scripts.
Like the rest of the hooks, setVmTicket should get a custom properties dict and not 'user-name' and 'user-id'.
engine would always send user-name and user-id and the custom properties users have defined there to allow for normal hooks behaviour.

....................................................
File vdsm/API.py
Line 502:     def setTicket(self, password, ttl, userName, userId, existingConnAction):
As I mentioned on the commit message, should be custom properties the same as the rest of the hooks (I don't remember the name of the dictionary, but should be persistent)

....................................................
File vdsm.spec.in
Line 554: %dir %{_libexecdir}/%{vdsm_name}/hooks/before_set_ticket
please rename to include 'vm' as the rest of the hooks are named.
i.e. before_vm_set_ticket and after_vm_set_ticket

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 1: (3 inline comments)

....................................................
Commit Message
Line 10: the benefit of the hook scripts.
Done

....................................................
File vdsm/API.py
Line 502:     def setTicket(self, password, ttl, userName, userId, existingConnAction):
Done

....................................................
File vdsm.spec.in
Line 554: %dir %{_libexecdir}/%{vdsm_name}/hooks/before_set_ticket
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Ayal Baron has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

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

(3 inline comments)

....................................................
File vdsm_cli/vdsClient.py
Line 311:             params = {'userName': userName, 'userId': userId}
should be generic.  First 4 (mandatory) params should be passed by name, the rest should be just pushed into a dict.
vmId, otp64, secs, connAct = args[:4]
params = args[4:] (probably just a string like backend passes? or need to convert into a dict or something)
...

Line 1625:                         ('<vmId> <password> <sec> [disconnect|keep|fail]',
no mention of optional 'params'?

....................................................
File vdsm/libvirtvm.py
Line 1925:         userName = params.get('userName', '')
why do we expose the specific params here?
It should be up to the script to determine what it needs ?!?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File vdsm_cli/vdsClient.py
Line 311:             params = {'userName': userName, 'userId': userId}
yes, but me must check if it get 3 params since by now the forth param was optional...

Line 1625:                         ('<vmId> <password> <sec> [disconnect|keep|fail]',
Done

....................................................
File vdsm/libvirtvm.py
Line 1925:         userName = params.get('userName', '')
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Ayal Baron has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 3: (3 inline comments)

Minor nit picks (sorry)

....................................................
File vdsm/API.py
Line 518:         :param pamram additoinal parameters in dict format
s/pamram//
s/additoinal/additional/

....................................................
File vdsm_cli/vdsClient.py
Line 315:             params = {}
no need to define params here in the if... else, just:
params = {}
if len(args) >= 4:
    params = self...

Line 1632:                          'Optional adiitonal parameters in dictionary format, name:value,name:value'
s/adiitional/additional

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 3: (3 inline comments)

....................................................
File vdsm/API.py
Line 518:         :param pamram additoinal parameters in dict format
Done

....................................................
File vdsm_cli/vdsClient.py
Line 315:             params = {}
Done

Line 1632:                          'Optional adiitonal parameters in dictionary format, name:value,name:value'
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 7: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Ayal Baron has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 7: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

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

(4 inline comments)

....................................................
File vdsm/hooks.py
Line 37: def _runHooksDir(domxml, dir, vmconf={}, raiseError=True, params = {}):
no spaces around this '=' sign please.

can you think of a better name than "params"? I cannot....

Line 51:         scriptenv.update(params)
the custom params of vm  are strings, by design.
Is that what we want for these extra params, too? I am not sure. I would prefer that these params are left general, and are marshaled via json or pickle for the use of the script.

Line 135: def before_vm_set_ticket(domxml, params, vmconf={}):
all hooks have vmconf as the second arg (if they have it at all). I'd like to keep this consistency.

....................................................
File vdsm/vdsmd.8.in
Line 65: .B vmId.
the new hooks have extra environ variables, from the "params" param

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 7: (4 inline comments)

....................................................
File vdsm/hooks.py
Line 37: def _runHooksDir(domxml, dir, vmconf={}, raiseError=True, params = {}):
Done

Line 51:         scriptenv.update(params)
yes its string

Line 135: def before_vm_set_ticket(domxml, params, vmconf={}):
Done

....................................................
File vdsm/vdsmd.8.in
Line 65: .B vmId.
what do you mean?
please define the string you want me to add/change

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 7: (2 inline comments)

....................................................
File vdsm/hooks.py
Line 51:         scriptenv.update(params)
In my opinion it is a too limiting interface... but I'm not 100% sure.

....................................................
File vdsm/vdsmd.8.in
Line 74: 
suggested text: (not sure)

before_vm_set_ticket and after_vm_set_ticket hooks receive a variable called "params" with a json-encoded value that was passed by setVmTicket caller.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 7: (1 inline comment)

....................................................
File vdsm/vdsmd.8.in
Line 74: 
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

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

(1 inline comment)

....................................................
File vdsm/vdsmd.8.in
Line 76: "params" with a json-encoded value that was passed by setVmTicket caller.
but you did NOT agree with me that this should be the interface.

your current behavior is:
The environment of before_vm_set_ticket and after_vm_set_ticket hooks is augmented with a set of params passed by the caller of setVmTicket.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

Shahar Havivi has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 8: (1 inline comment)

....................................................
File vdsm/vdsmd.8.in
Line 76: "params" with a json-encoded value that was passed by setVmTicket caller.
?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setVmTicket: add before and after hooks
......................................................................

Patch Set 9: Verified; Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: setVmTicket: add before and after hooks
......................................................................

setVmTicket: add before and after hooks

setVmTicket gets additional params dictionary,
currently engine will send user-name and user-id, for
the benefit of the setVmTicket hooks scripts.

Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Signed-off-by: Shahar Havivi 
---
M vdsm.spec.in
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/hooks.py
M vdsm/libvirtvm.py
M vdsm/vdsmd.8.in
M vdsm_cli/vdsClient.py
M vdsm_hooks/Makefile.am
8 files changed, 42 insertions(+), 13 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I48cbc934e48055630b734a2d3c79c72b6c36f28f
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Shahar Havivi