[Kimchi-devel] [PATCH] Handle transient VM requests differently
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Jan 19 17:14:04 UTC 2015
On 19/01/2015 15:01, Christy Perez wrote:
>
> 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...).
>
Good point! So keep doing how you did it. Just extend it to networks and
storage pools as well.
>>>>>> ---
>>>>>> 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 at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
More information about the Kimchi-devel
mailing list