[Kimchi-devel] [PATCH] Handle transient VM requests differently
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Jan 19 16:18:59 UTC 2015
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()
>
>>> ---
>>> 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