[Kimchi-devel] [PATCH] Handle transient VM requests differently
Christy Perez
christy at linux.vnet.ibm.com
Mon Jan 19 16:21:29 UTC 2015
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.
>>
>>>> ---
>>>> 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)
>
More information about the Kimchi-devel
mailing list