Re: [Kimchi-devel] [PATCH] Handle transient VM requests differently

On 01/19/2015 10:21 AM, Christy Perez wrote: > > On 01/19/2015 10:18 AM, Aline Manera wrote: >> On 19/01/2015 14:14, Christy Perez wrote: >>> On 01/19/2015 07:41 AM, Aline Manera wrote: >>>> On 14/01/2015 21:24, Christy Perez wrote: >>>>> If a transient (or non-persistent) VM is created outside of >>>>> Kimchi, it will (expectedly) be deleted when shutdown. However, >>>>> Kimchi calls a GET after shutdown and other POST requests. When >>>>> the GET fails because the VM has been deleted, a user will get >>>>> an error. However, this is not an error condition. >>>>> >>>>> This patch checks to see if a VM is transient, and if so, does not >>>>> call the subsequent GET on the VM. >>>> The same scenario exists for networks and storage pools. >>>> Could you please also update them? >>>> >>>> Also as it is common to multiple resources we could move the >>>> verification to framework >>>> >>>> At control/base.py in in generate_action_handler_base(): >>>> >>>> is_persistent = None >>>> if hasattr(model, model_fn(self, 'is_persistent')): >>>> is_persistent = getattr(self.model, model_fn(self, >>>> 'is_persistent')) >>>> >>>> (...) >>>> >>>> except NotFoundError, e: >>>> # only raise the exception if the resource is persistent >>>> if is_persistent is not None and is_persistent(): >>>> raise cherrypy.HTTPError(404, e.message) >>> Sounds good. I'll get a v2 out ASAP with your suggestions. >>> >>> Thanks! >> I also thought about other solution. >> >> _generate_action_handler_base() already calls lookup() so we can check >> the return data >> >> data = self.lookup() >> >> if 'persistent' in data.keys() and data['persistent']: >> raise NotFoundError() >> > Great! Thanks. > Actually -- I don't think this is a good way to go after all. This would ignore the NotFoundError for all POST methods of transient objects. The way it is now only ignores it for (in this example) on poweroff, shutdown, and reset. It wouldn't make sense to ignore it for start, for example (though, start in this context will never be used, since transient things are auto-started...).

>>>>> --- >>>>> src/kimchi/control/base.py | 13 +++++++++---- >>>>> src/kimchi/control/vms.py | 17 +++++++++++++---- >>>>> src/kimchi/i18n.py | 1 + >>>>> src/kimchi/model/vms.py | 9 ++++++++- >>>>> 4 files changed, 31 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py >>>>> index 97e789f..8048370 100644 >>>>> --- a/src/kimchi/control/base.py >>>>> +++ b/src/kimchi/control/base.py >>>>> @@ -63,7 +63,8 @@ def _redirect(self, ident, code=303): >>>>> uri_params += [urllib2.quote(ident.encode('utf-8'), >>>>> safe="")] >>>>> raise cherrypy.HTTPRedirect(self.uri_fmt % >>>>> tuple(uri_params), code) >>>>> >>>>> - def generate_action_handler(self, action_name, action_args=None): >>>>> + def generate_action_handler(self, action_name, action_args=None, >>>>> + ignore_not_found=False): >>>>> def _render_element(self, ident): >>>>> self._redirect(ident) >>>>> uri_params = [] >>>>> @@ -75,7 +76,7 @@ def _render_element(self, ident): >>>>> raise internal_redirect(self.uri_fmt % >>>>> tuple(uri_params)) >>>>> >>>>> return self._generate_action_handler_base(action_name, >>>>> _render_element, >>>>> - action_args) >>>>> + ignore_not_found, >>>>> action_args) >>>>> >>>>> def generate_action_handler_task(self, action_name, >>>>> action_args=None): >>>>> def _render_task(self, task): >>>>> @@ -86,7 +87,7 @@ def _render_task(self, task): >>>>> action_args) >>>>> >>>>> def _generate_action_handler_base(self, action_name, render_fn, >>>>> - action_args=None): >>>>> + ignore_not_found, >>>>> action_args=None): >>>>> def wrapper(*args, **kwargs): >>>>> validate_method(('POST'), self.role_key, >>>>> self.admin_methods) >>>>> try: >>>>> @@ -111,7 +112,11 @@ def wrapper(*args, **kwargs): >>>>> except UnauthorizedError, e: >>>>> raise cherrypy.HTTPError(403, e.message) >>>>> except NotFoundError, e: >>>>> - raise cherrypy.HTTPError(404, e.message) >>>>> + if ignore_not_found is True: >>>>> + # Should something be returned instead? >>>>> + pass >>>>> + else: >>>>> + raise cherrypy.HTTPError(404, e.message) >>>>> except OperationFailed, e: >>>>> raise cherrypy.HTTPError(500, e.message) >>>>> except KimchiException, e: >>>>> diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py >>>>> index 1a84b6c..82682af 100644 >>>>> --- a/src/kimchi/control/vms.py >>>>> +++ b/src/kimchi/control/vms.py >>>>> @@ -21,7 +21,6 @@ >>>>> from kimchi.control.utils import internal_redirect, UrlSubNode >>>>> from kimchi.control.vm import sub_nodes >>>>> >>>>> - >>>>> @UrlSubNode('vms', True) >>>>> class VMs(Collection): >>>>> def __init__(self, model): >>>>> @@ -37,12 +36,22 @@ def __init__(self, model, ident): >>>>> self.role_key = 'guests' >>>>> self.screenshot = VMScreenShot(model, ident) >>>>> self.uri_fmt = '/vms/%s' >>>>> + _ignore_not_found = False >>>>> + try: >>>>> + # the test driver causes an exception in is_persistent() >>>>> + _ignore_not_found = \ >>>>> + not model.vm_is_persistent(self.ident.decode('utf-8')) >>>>> + except: >>>>> + pass >>>>> for ident, node in sub_nodes.items(): >>>>> setattr(self, ident, node(model, self.ident)) >>>>> self.start = self.generate_action_handler('start') >>>>> - self.poweroff = self.generate_action_handler('poweroff') >>>>> - self.shutdown = self.generate_action_handler('shutdown') >>>>> - self.reset = self.generate_action_handler('reset') >>>>> + self.poweroff = self.generate_action_handler('poweroff', >>>>> + ignore_not_found = _ignore_not_found) >>>>> + self.shutdown = self.generate_action_handler('shutdown', >>>>> + ignore_not_found = _ignore_not_found) >>>>> + self.reset = self.generate_action_handler('reset', >>>>> + ignore_not_found = _ignore_not_found) >>>>> self.connect = self.generate_action_handler('connect') >>>>> self.clone = self.generate_action_handler_task('clone') >>>>> >>>>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py >>>>> index 4eccc3e..4ab08d1 100644 >>>>> --- a/src/kimchi/i18n.py >>>>> +++ b/src/kimchi/i18n.py >>>>> @@ -106,6 +106,7 @@ >>>>> "KCHVM0033E": _("Virtual machine '%(name)s' must be stopped >>>>> before cloning it."), >>>>> "KCHVM0034E": _("Insufficient disk space to clone virtual >>>>> machine '%(name)s'"), >>>>> "KCHVM0035E": _("Unable to clone VM '%(name)s'. Details: >>>>> %(err)s"), >>>>> + "KCHVM0036E": _("Invalid operation for non-persistent virtual >>>>> machine %(name)s"), >>>>> >>>>> "KCHVMHDEV0001E": _("VM %(vmid)s does not contain directly >>>>> assigned host device %(dev_name)s."), >>>>> "KCHVMHDEV0002E": _("The host device %(dev_name)s is not allowed >>>>> to directly assign to VM."), >>>>> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py >>>>> index bae27c1..d3b4f1f 100644 >>>>> --- a/src/kimchi/model/vms.py >>>>> +++ b/src/kimchi/model/vms.py >>>>> @@ -760,9 +760,13 @@ def lookup(self, name): >>>>> 'users': users, >>>>> 'groups': groups, >>>>> 'access': 'full', >>>>> - 'persistent': True if dom.isPersistent() else False >>>>> + 'persistent': self.is_persistent(name) >>>>> } >>>>> >>>>> + def is_persistent(self, name): >>>>> + dom = self.get_vm(name, self.conn) >>>>> + return True if dom.isPersistent() else False >>>>> + >>>>> def _vm_get_disk_paths(self, dom): >>>>> xml = dom.XMLDesc(0) >>>>> xpath = "/domain/devices/disk[@device='disk']/source/@file" >>>>> @@ -784,6 +788,9 @@ def get_vm(name, conn): >>>>> def delete(self, name): >>>>> conn = self.conn.get() >>>>> dom = self.get_vm(name, self.conn) >>>>> + if not dom.isPersistent(): >>>>> + raise InvalidOperation("KCHVM0036E", {'name': name}) >>>>> + >>>>> self._vmscreenshot_delete(dom.UUIDString()) >>>>> paths = self._vm_get_disk_paths(dom) >>>>> info = self.lookup(name) > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel >