Re: [Kimchi-devel] [PATCH 1/7] Granular Task Queries: Backend

On 04/24/2015 04:01 PM, Aline Manera wrote: > > On 17/04/2015 19:35, Christy Perez wrote: >> On 04/17/2015 01:39 PM, Aline Manera wrote: >>> On 17/04/2015 15:08, Christy Perez wrote: >>>> On 04/17/2015 12:51 PM, Aline Manera wrote: >>>>> On 17/04/2015 12:24, Christy Perez wrote: >>>>>> You can filter tasks by their state (running) and/or a target_uri. >>>>>> If you want to see all cloning VM tasks, you can only query for >>>>>> running tasks with /vm in the target_uri. This patch adds an action >>>>>> keyword to the target_uris for existing tasks. This will make it >>>>>> possible to query other types of tasks for the same resource. >>>>>> >>>>>> Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; >>>>>> --- >>>>>> docs/API.md | 16 ++++++++++++++++ >>>>>> src/kimchi/mockmodel.py | 7 ++++--- >>>>>> src/kimchi/model/debugreports.py | 4 ++-- >>>>>> src/kimchi/model/host.py | 4 ++-- >>>>>> src/kimchi/model/storagepools.py | 2 +- >>>>>> src/kimchi/model/storagevolumes.py | 12 +++++++----- >>>>>> src/kimchi/model/vms.py | 5 ++--- >>>>>> src/kimchi/model/vmsnapshots.py | 5 +++-- >>>>>> 8 files changed, 37 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/docs/API.md b/docs/API.md >>>>>> index 3f7925f..1f8e769 100644 >>>>>> --- a/docs/API.md >>>>>> +++ b/docs/API.md >>>>>> @@ -651,6 +651,22 @@ server. >>>>>> * failed: The task failed >>>>>> * message: Human-readable details about the Task status >>>>>> * target_uri: Resource URI related to the Task >>>>>> + The target uri may end with an action descriptor: >>>>>> + * VM task descriptions: >>>>>> + * clone >>>>>> + * create >>>>>> + * VM Snapshot description >>>>>> + * create >>>>>> + * Storage descriptions: >>>>>> + * create >>>>>> + * clone >>>>>> + * Pool descriptions: >>>>>> + * create >>>>>> + * scan >>>>> Is there a scan action? Something like /storagepools/<name>/scan ? >>>> In StoragePoolsModel, def _do_deep_scan(self, params): >>>> task_id = add_task('/storagepools/%s/scan' % ISO_POOL_NAME, >>>> self.scanner.start_scan, self.objstore, >>>> scan_params) >>>>>> + * Debugreport descriptions: >>>>>> + * create >>>>>> + * Host SW Update descriptions: >>>>>> + * update >>>>> I don't think we need to change the main POST action. Just those >>>>> actions >>>>> to a specific resource. >>>>> Example, >>>>> >>>>> # to create the VM >>>>> POST /vms >>>>> Target URI: /vms/<name> >>>>> >>>>> # to clone the VM >>>>> POST /vms/<name>/clone >>>>> Target URI: /vms/<name>/clone >>>>> >>>>> # to create a snapshot >>>>> POST /vms/<name>/snapshot >>>>> Target URI: /vms/<name>/snapshot/<snap> >>>>> >>>>> Does that make sense? >>>> That makes sense. I can see how it can work without changing this, >>>> but I >>>> like adding it for consistency. If you then query all tasks, some will >>>> not have the extra /<action> at the end. That will be understood to be >>>> the POST (create), but, I like the consistency of having the action for >>>> everything better. >>>> >>>> Also, the way it is now, you'll be able to query all 'create' tasks for >>>> all objects across the system, which I think would be nice -- >>>> although I >>>> don't have a direct use case for that in mind. >>> My suggestion is because we want to auto generate the target_uri based >>> on request (in future) >>> And in that case, we will not have the /create in the end. >>> >>> See: https://github.com/kimchi-project/kimchi/issues/559 >> I see! So if we did keep it as it is, we'd have to add a little extra >> logic in for that feature, like: >> >> def generate_target_uri(request): >> target_uri = some_cool_one_liner >> if request_type is 'POST': >> target_uri = "%s/create" , %target_uri >> return target_uri >> >> or something, which isn't as elegant. >> >> Another thing I just thought of is DELETE tasks. Right now those are all >> done synchronously, but things should probably also be deleted >> asynchronously in the future. Would that target uri also just be >> /vms/<vm-name>? > When generating the target URI automatically we can use the METHOD value > to distinguish the request to a resource. > > But for now, I'd suggest to only change the target URI to add the action > in the end. Just to make things simpler. It sounds like you're saying this patchset is fine as-is, then?

> >>>>> >>>>> >>>>> >>>>>> * **POST**: *See Task Actions* >>>>>> >>>>>> **Actions (POST):** >>>>>> diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py >>>>>> index 413ac5d..2b1b214 100644 >>>>>> --- a/src/kimchi/mockmodel.py >>>>>> +++ b/src/kimchi/mockmodel.py >>>>>> @@ -244,7 +244,7 @@ def _get_volume_path(self, pool, vol): >>>>>> return MockModel._libvirt_get_vol_path(pool, vol) >>>>>> >>>>>> def _gen_debugreport_file(self, name): >>>>>> - return add_task('/debugreports/%s' % name, self._create_log, >>>>>> + return add_task('/debugreports/%s/create' % name, >>>>>> self._create_log, >>>>>> self.objstore, name) >>>>>> >>>>>> def _create_log(self, cb, name): >>>>>> @@ -333,7 +333,8 @@ def _mock_packageupdate_lookup(self, pkg_name): >>>>>> return self._mock_swupdate.pkgs[pkg_name] >>>>>> >>>>>> def _mock_host_swupdate(self, args=None): >>>>>> - task_id = add_task('/host/swupdate', >>>>>> self._mock_swupdate.doUpdate, >>>>>> + task_id = add_task('/host/swupdate/update', >>>>>> + self._mock_swupdate.doUpdate, >>>>>> self.objstore) >>>>>> return self.task_lookup(task_id) >>>>>> >>>>>> @@ -385,7 +386,7 @@ def _mock_vm_clone(self, name): >>>>>> def _mock_vmsnapshots_create(self, vm_name, params): >>>>>> name = params.get('name', unicode(int(time.time()))) >>>>>> params = {'vm_name': vm_name, 'name': name} >>>>>> - taskid = add_task(u'/vms/%s/snapshots/%s' % (vm_name, name), >>>>>> + taskid = add_task(u'/vms/%s/snapshots/%s/create' % (vm_name, >>>>>> name), >>>>>> self._vmsnapshots_create_task, >>>>>> self.objstore, params) >>>>>> return self.task_lookup(taskid) >>>>>> >>>>>> diff --git a/src/kimchi/model/debugreports.py >>>>>> b/src/kimchi/model/debugreports.py >>>>>> index 5f74da8..19b4de5 100644 >>>>>> --- a/src/kimchi/model/debugreports.py >>>>>> +++ b/src/kimchi/model/debugreports.py >>>>>> @@ -62,8 +62,8 @@ def _gen_debugreport_file(self, name): >>>>>> gen_cmd = self.get_system_report_tool() >>>>>> >>>>>> if gen_cmd is not None: >>>>>> - return add_task('/debugreports/%s' % name, gen_cmd, >>>>>> self.objstore, >>>>>> - name) >>>>>> + return add_task('/debugreports/%s/create' % name, >>>>>> gen_cmd, >>>>>> + self.objstore, name) >>>>>> >>>>>> raise OperationFailed("KCHDR0002E") >>>>>> >>>>>> diff --git a/src/kimchi/model/host.py b/src/kimchi/model/host.py >>>>>> index 4419bb3..f429415 100644 >>>>>> --- a/src/kimchi/model/host.py >>>>>> +++ b/src/kimchi/model/host.py >>>>>> @@ -115,8 +115,8 @@ def swupdate(self, *name): >>>>>> raise OperationFailed('KCHPKGUPD0001E') >>>>>> >>>>>> kimchi_log.debug('Host is going to be updated.') >>>>>> - taskid = add_task('/host/swupdate', swupdate.doUpdate, >>>>>> self.objstore, >>>>>> - None) >>>>>> + taskid = add_task('/host/swupdate/update', >>>>>> swupdate.doUpdate, >>>>>> + self.objstore, None) >>>>>> return self.task.lookup(taskid) >>>>>> >>>>>> def shutdown(self, args=None): >>>>>> diff --git a/src/kimchi/model/storagepools.py >>>>>> b/src/kimchi/model/storagepools.py >>>>>> index b85f3b4..094c841 100644 >>>>>> --- a/src/kimchi/model/storagepools.py >>>>>> +++ b/src/kimchi/model/storagepools.py >>>>>> @@ -167,7 +167,7 @@ def _do_deep_scan(self, params): >>>>>> >>>>>> params['path'] = >>>>>> self.scanner.scan_dir_prepare(params['name']) >>>>>> scan_params['pool_path'] = params['path'] >>>>>> - task_id = add_task('/storagepools/%s' % ISO_POOL_NAME, >>>>>> + task_id = add_task('/storagepools/%s/scan' % ISO_POOL_NAME, >>>>>> self.scanner.start_scan, >>>>>> self.objstore, >>>>>> scan_params) >>>>>> # Record scanning-task/storagepool mapping for future >>>>>> querying >>>>>> try: >>>>>> diff --git a/src/kimchi/model/storagevolumes.py >>>>>> b/src/kimchi/model/storagevolumes.py >>>>>> index 0480496..9f93f10 100644 >>>>>> --- a/src/kimchi/model/storagevolumes.py >>>>>> +++ b/src/kimchi/model/storagevolumes.py >>>>>> @@ -116,8 +116,10 @@ def create(self, pool_name, params): >>>>>> raise InvalidParameter('KCHVOL0001E', {'name': name}) >>>>>> >>>>>> params['pool'] = pool_name >>>>>> - targeturi = '/storagepools/%s/storagevolumes/%s' % >>>>>> (pool_name, name) >>>>>> - taskid = add_task(targeturi, create_func, self.objstore, >>>>>> params) >>>>>> + targeturi = '/storagepools/%s/storagevolumes/%s/create' \ >>>>>> + % (pool_name, name) >>>>>> + taskid = add_task(targeturi, create_func, self.objstore, >>>>>> + params) >>>>>> return self.task.lookup(taskid) >>>>>> >>>>>> def _create_volume_with_file(self, cb, params): >>>>>> @@ -413,9 +415,9 @@ def clone(self, pool, name, new_pool=None, >>>>>> new_name=None): >>>>>> 'name': name, >>>>>> 'new_pool': new_pool, >>>>>> 'new_name': new_name} >>>>>> - taskid = add_task(u'/storagepools/%s/storagevolumes/%s' % >>>>>> - (pool, new_name), self._clone_task, >>>>>> self.objstore, >>>>>> - params) >>>>>> + taskid = >>>>>> add_task(u'/storagepools/%s/storagevolumes/%s/clone' % >>>>>> + (pool, new_name), self._clone_task, >>>>>> + self.objstore, params) >>>>>> return self.task.lookup(taskid) >>>>>> >>>>>> def _clone_task(self, cb, params): >>>>>> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py >>>>>> index 4c5f443..1ae841d 100644 >>>>>> --- a/src/kimchi/model/vms.py >>>>>> +++ b/src/kimchi/model/vms.py >>>>>> @@ -332,9 +332,8 @@ def clone(self, name): >>>>>> new_name = get_next_clone_name(current_vm_names, name) >>>>>> >>>>>> # create a task with the actual clone function >>>>>> - taskid = add_task(u'/vms/%s' % new_name, self._clone_task, >>>>>> - self.objstore, >>>>>> - {'name': name, 'new_name': new_name}) >>>>>> + taskid = add_task(u'/vms/%s/clone' % new_name, >>>>>> self._clone_task, >>>>>> + self.objstore, {'name': name, 'new_name': >>>>>> new_name}) >>>>>> >>>>>> return self.task.lookup(taskid) >>>>>> >>>>>> diff --git a/src/kimchi/model/vmsnapshots.py >>>>>> b/src/kimchi/model/vmsnapshots.py >>>>>> index 3a92cdc..c4cc663 100644 >>>>>> --- a/src/kimchi/model/vmsnapshots.py >>>>>> +++ b/src/kimchi/model/vmsnapshots.py >>>>>> @@ -72,8 +72,9 @@ def create(self, vm_name, params={}): >>>>>> name = params.get('name', unicode(int(time.time()))) >>>>>> >>>>>> task_params = {'vm_name': vm_name, 'name': name} >>>>>> - taskid = add_task(u'/vms/%s/snapshots/%s' % (vm_name, name), >>>>>> - self._create_task, self.objstore, >>>>>> task_params) >>>>>> + taskid = add_task(u'/vms/%s/snapshots/%s/create' % (vm_name, >>>>>> name), >>>>>> + self._create_task, self.objstore, >>>>>> + task_params) >>>>>> return self.task.lookup(taskid) >>>>>> >>>>>> def _create_task(self, cb, params):