8:25 a.m.
Adam Litke has uploaded a new change for review.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
API: getAllTasks{Info|Statuses} does not return all tasks
As discussed on the list, I found that the getAllTasks* APIs do not return all
tasks but instead filter to return only spm tasks. This is a requirement for
ovirt-engine. To make it possible to view all tasks while preserving
compatibility for ovirt-engine I propose the following changes to the API:
The public API in API.py will be modified to add a 'tag' parameter which
defaults to None. This will be passed all the way down into the taskManager
(which already expects such a parameter). In this way, the new default mode of
operation will be fixed to return all tasks as the name of the API states.
To maintain compatibility for ovirt-engine, the XMLRPC binding will pass the tag
"spm" to these APIs to get the current filtering behavior. Note that I have
also changed vdsClient so that it will return all tasks.
Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Signed-off-by: Adam Litke <agl(a)us.ibm.com>
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm_cli/vdsClient.py
4 files changed, 33 insertions(+), 19 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/13/3613/1
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
1:44 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Dan Kenigsberg has submitted this change and it was merged.
Change subject: API: Provide a working getAllTasks API
......................................................................
API: Provide a working getAllTasks API
Changes since last iteration:
- Dropped 'tags' filtering from the API. Users can do client-side filtering as
they see fit.
The current APIs for retrieving all task information do not actually return all
task information. I would like to introduce a new API that corrects this and
other issues with the current API while preserving backwards compatibility with
ovirt-engine for as long as is necessary.
The current APIs:
getAllTasksInfo(spUUID=None, options = None):
- Returns a dictionary that maps a task UUID to a task verb.
- Despite having 'all' in the name, this API only returns tasks that have an
'spm' tag.
- This call returns only one piece of information for each task.
- The spUUID parameter is deprecated and ignored.
getAllTasksStatuses(spUUID=None, options = None):
- Returns a dictionary of task status information.
- Despite having 'all' in the name, this API only returns tasks that have an
'spm' tag.
- The spUUID parameter is deprecated and ignored.
I propose the following new API:
getAllTasks(options=None):
- Returns a dictionary of task information. The info from both of the above
functions would be merged into a single result set.
- The spUUID parameter is dropped. options is for future extension and is
currently not used.
This new API includes all functionality that is available in the old calls. In
the future, ovirt-engine could switch to this API and preserve the current
semantics by ignoring tasks where task['tag'] != 'spm'. Meanwhile, API
users
that really want all tasks (gluster and the REST API) can get what they need.
Signed-off-by: Adam Litke <agl(a)us.ibm.com>
Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/task.py
M vdsm/storage/taskManager.py
M vdsm_api/vdsmapi-schema.json
M vdsm_cli/vdsClient.py
7 files changed, 106 insertions(+), 3 deletions(-)
Approvals:
Adam Litke: Verified
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
3:05 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2:
Any chance I could get some review on this?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
1:44 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Dan Kenigsberg has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 4: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
11:21 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 4:
Hi Dan. In your overall comment for the previous patch I read:
"Adam, your suggested API is great. However, I feel that the current mess that is
task.tag is not ready for API exposure."
-and-
"I would very much like not to expose this kind of "tags" property of a
task."
Taken together I read this to mean, don't expose the task tag in the API...
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
6:32 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Dan Kenigsberg has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 4:
Have I really suggested this?
Would it be possible to add server-side filtering semantics to this verb, sometime in the
future?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
4:33 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
oVirt Jenkins CI Server has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/599/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
4:33 p.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 4: Verified
In this version, I dropped the tags filtering from the API in favor of letting clients
manipulate the task list as they see fit. This was suggested by Dan.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
4:01 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Dan Kenigsberg has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
Adam, your suggested API is great. However, I feel that the current mess that is task.tag
is not ready for API exposure.
If someone creates a task and tags it "aspmushmush", it would be considered an
spm task, since "spm" is a substring of it. I find the string search logic of
if not tag or tag in task.getTags():
really messy and unintentional. Currently, a task may have only one string tag. The
multiple form of Task.getTags() serves only as a distraction. I would very much like not
to expose this kind of "tags" property of a task.
Again, this is NOT your fault.
....................................................
File vdsm/storage/taskManager.py
Line 119: """
Line 120: self.log.debug("Entry.")
Line 121: subRes = {}
Line 122: for taskID, task in self._tasks.items():
Line 123: if not tags or set(tags) & set([task.getTags()]):
I'd prefer to calculate set(tags) only once
also, I found the code misleading - all it does is
if not tags or task.getTags() in tags
yes, I know that the original code is not any better.
Line 124: try:
Line 125: subRes[taskID] = task.getDetails()
Line 126: except se.UnknownTask:
Line 127: # Return info for existing tasks only.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
7 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 3: Verified
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
2:32 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Bala.FA has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
(1 inline comment)
....................................................
File vdsm_cli/vdsClient.py
Line 1176: keys = [x.strip() for x in args[0].split(',')]
In that case, I would like to have list of keys as less of params eg. keys = args[1:]
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
8:48 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm_cli/vdsClient.py
Line 1176: keys = [x.strip() for x in args[0].split(',')]
Because we might want to add additional parameters someday so args[1] might be some flags
or something.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
5:09 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Bala.FA has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm_cli/vdsClient.py
Line 1176: keys = [x.strip() for x in args[0].split(',')]
As args is a list, why not passing it as keys?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
9:16 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Igor Lvovsky has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2:
hmmm, looks like you are right. Indeed it both almost same.
Still have minor comment inline.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
8:43 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2:
Igor, thanks for your review. In looking at getAllTasksInfo() and getAllTasksStatuses() I
don't see how task info could be more expensive to get than task status. Today
getTaskInfo() simply returns task.name and getTaskStatus returns a dict of values:
self.id, self.state, self.code, self.message. Aside from the data they return, these two
functions seem to behave identically. If anything, this will improve performance in some
cases by eliminating the need for a second API call when you also want to task verb. Am I
missing something?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
3:51 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Igor Lvovsky has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(1 inline comment)
Only minor comment inside.
In generally as I remember the previous behaviour was:
1. poll the tasks statuses with getAllTaskStatuses because it's cheap
2. only if task finished/failed call getAllTaskInlfo
Now you will call a new getAllTasks every time that is pretty expensive
....................................................
File vdsm/storage/task.py
Line 1282:
what about task id?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
1:50 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Bala.FA has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
9:54 a.m.
New subject: Change in vdsm[master]: API: Provide a working getAllTasks API
Adam Litke has posted comments on this change.
Change subject: API: Provide a working getAllTasks API
......................................................................
Patch Set 2: Verified
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal(a)redhat.com>
1:41 p.m.
Adam Litke has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1:
I have proposed a new API on vdsm-devel that "fixes" the behavior of these old
calls. Please continue the discussion there. Thanks!
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
3:20 a.m.
Doron Fediuck has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: No score
(1 inline comment)
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
On a first check (see above) it looked like it may used it.
After some further lookups in the internals I do not see any use of it, but I prefer this
is verified against existing installations, to verify it wasn't in use previously,
which will
break current installations.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
7:59 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
Doron, does this java code use the "tag" argument?
was that argument ever used by Engine?
I cannot say that it is clear to me.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
7:51 a.m.
Doron Fediuck has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
It looks like it's going to break the API with engine-core.
Please verify it.
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
Take a look at
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AsyncTaskManager.java
268 poolsAsyncTaskMap.put(storagePoolID, new HashMap<Guid,
AsyncTaskStatus>(
269 (Map<Guid, AsyncTaskStatus>)
Backend.getInstance().getResourceManager().RunVdsCommand(
270 VDSCommandType.SPMGetAllTasksStatuses,
271 new
IrsBaseVDSCommandParameters(storagePoolID)).getReturnValue()));
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
3:50 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: (2 inline comments)
Doron, could you give us a hand? I would like to verify that we do not break an API that
an old-but-installed rhevm^WEngine depends upon.
....................................................
File vdsm_cli/vdsClient.py
Line 1107: tag = ''
for the record, I don't understand Ayal either.
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
I'd try recruiting Doron's memory
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
2:36 p.m.
Adam Litke has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
I checked the current code and it does not. How far back do I need to check?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
4:36 p.m.
Ayal Baron has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
5:19 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm_cli/vdsClient.py
Line 1132: def getAllTasksStatuses(self, args):
touching it is not fun, but this script has doc. please update it for these two verbs.
....................................................
File vdsm/storage/hsm.py
Line 1691: def getAllTasksStatuses(self, tag=None, options = None):
have you checked that ovirt-engine never passes the spUUID param?
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
10:35 a.m.
Adam Litke has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: (2 inline comments)
....................................................
File vdsm/BindingXMLRPC.py
Line 659: tag = None
This could do with a comment. vdsClient would like to have getAllTasksInfo return _all_
tasks (not the filtered list that engine wants). In order to do that it must pass None
which is not allowed by our current xmlrpc configuration. So what this does is allow any
expression that evaluates to False: '' or False specifically to trigger the
behavior. It makes sense that the string "" should imply no filtering.
Does it make sense now?
....................................................
File vdsm_cli/vdsClient.py
Line 1107: tag = ''
I am not really sure what you mean here. Could you explain in a little more detail.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
2:07 a.m.
Ayal Baron has posted comments on this change.
Change subject: API: getAllTasks{Info|Statuses} does not return all tasks
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(3 inline comments)
....................................................
File vdsm/BindingXMLRPC.py
Line 659: tag = None
I don't understand the need for this "if". If nothing was passed, tag would
be "spm"...?
Line 666: tag = None
Same as above.
....................................................
File vdsm_cli/vdsClient.py
Line 1107: tag = ''
If we do not support passing None in xmlrpc then in the if above you can create a partial
object bound to tag args[0] and that way avoid passing anything in case there is such
param.
--
To view, visit http://gerrit.ovirt.org/3613
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43371d358dfdfcfcee14aca40ed06d63be7c0a44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
4269
days inactive
4398
days old
vdsm-patches@lists.fedorahosted.org
27 comments
7 participants
participants (7)
-
abaron＠redhat.com -
agl＠us.ibm.com -
barumuga＠redhat.com -
Dan Kenigsberg -
dfediuck＠redhat.com -
Igor Lvovsky -
Jenkins CI RO