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

On 24/04/2015 19:49, Christy Perez wrote: > > 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? No. Do not append "create" when it is not an action - just a POST request POST /vms will turn on target_uri=/vms/<name> POST /debugreports will turn on target_uri=/debugreports/<name> POST /vms/<name>/start will turn on target_uri=/vms/<name>/start POST /vms/<name>/clone will turn on target_uri=/vms/<name>/clone Does that make sense? Using those target_uri we will only need to append the METHOD on the beginning when creating it automatically.

> >> >>>>>> >>>>>> >>>>>> >>>>>>> * **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):