[PATCH v2 0/4] Transient Object Handling
In order to prevent benign error messages from the user, some POST requests for transient objects should be handled as special cases. This patchset introduces a way to catch an error on the subsequent GET after a POST, and also implments it for the three types of transient objects that Kimchi deals with: VMs, Networks, and Storage Pools. Christy Perez (4): Handle requests differently for transient objects Transient VM POST request handling Transient Network POST request handling Transient StoragePool POST request handling src/kimchi/control/base.py | 13 +++++++++---- src/kimchi/control/networks.py | 11 +++++++++-- src/kimchi/control/storagepools.py | 11 +++++++++-- src/kimchi/control/vms.py | 18 ++++++++++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/networks.py | 6 +++++- src/kimchi/model/storagepools.py | 8 ++++++-- src/kimchi/model/vms.py | 9 ++++++++- 8 files changed, 61 insertions(+), 16 deletions(-) -- 1.9.3
If a transient (or non-persistent) object (e.g. VM, network or storage pool) is created outside of Kimchi, it will (expectedly) be deleted when shutdown. Since Kimchi calls a GET after shutdown and other POST requests, the GET fails, and a user will recieve an error. However, this is not an error condition. This patch avoids reporting the exception on the GET if the object should to ignore it. --- src/kimchi/control/base.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 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: -- 1.9.3
On 19/01/2015 19:49, Christy Perez wrote:
If a transient (or non-persistent) object (e.g. VM, network or storage pool) is created outside of Kimchi, it will (expectedly) be deleted when shutdown. Since Kimchi calls a GET after shutdown and other POST requests, the GET fails, and a user will recieve an error. However, this is not an error condition.
This patch avoids reporting the exception on the GET if the object should to ignore it. --- src/kimchi/control/base.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 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?
I don't think so. You can change the if statement to only raise the exception when ignore_not_found is False. if not ignore_not_found: raise NotFoundError()
+ pass + else: + raise cherrypy.HTTPError(404, e.message)
except OperationFailed, e: raise cherrypy.HTTPError(500, e.message) except KimchiException, e:
Ignore the exception on GET for PowerOff, Shutdown, and Reset actioins for VMs. --- src/kimchi/control/vms.py | 18 ++++++++++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 9 ++++++++- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index 1a84b6c..68e6e93 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,23 @@ 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 Exception: + 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) -- 1.9.3
On 19/01/2015 19:49, Christy Perez wrote:
Ignore the exception on GET for PowerOff, Shutdown, and Reset actioins for VMs. --- src/kimchi/control/vms.py | 18 ++++++++++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 9 ++++++++- 3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index 1a84b6c..68e6e93 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,23 @@ 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 Exception: + pass +
You should be handled in the model function (vm_is_persistent)
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}) +
Why is it needed? libvirt retuns any error when trying to delete a non-persistent VM? Should we also update the UI to avoid this kind of operation?
self._vmscreenshot_delete(dom.UUIDString()) paths = self._vm_get_disk_paths(dom) info = self.lookup(name)
On 21/01/2015 12:15, Aline Manera wrote:
On 19/01/2015 19:49, Christy Perez wrote:
Ignore the exception on GET for PowerOff, Shutdown, and Reset actioins for VMs. --- src/kimchi/control/vms.py | 18 ++++++++++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 9 ++++++++- 3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index 1a84b6c..68e6e93 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,23 @@ 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 Exception: + pass +
You should be handled in the model function (vm_is_persistent)
The same comment applies to the network and storage pool patches. Also instead of expected an generic Exception, use the exact expected exception there.
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}) +
Why is it needed? libvirt retuns any error when trying to delete a non-persistent VM?
Should we also update the UI to avoid this kind of operation?
self._vmscreenshot_delete(dom.UUIDString()) paths = self._vm_get_disk_paths(dom) info = self.lookup(name)
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Ignore the exception on GET for Stop (deactivate) of transient Networks. --- src/kimchi/control/networks.py | 11 +++++++++-- src/kimchi/model/networks.py | 6 +++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index 7cb1572..14d6324 100644 --- a/src/kimchi/control/networks.py +++ b/src/kimchi/control/networks.py @@ -20,7 +20,6 @@ from kimchi.control.base import Collection, Resource from kimchi.control.utils import UrlSubNode - @UrlSubNode('networks', True) class Networks(Collection): def __init__(self, model): @@ -33,11 +32,19 @@ def __init__(self, model): class Network(Resource): def __init__(self, model, ident): super(Network, self).__init__(model, ident) + _ignore_not_found = False + try: + # the test driver causes an exception in is_persistent() + _ignore_not_found = \ + not model.network_is_persistent(self.ident.decode('utf-8')) + except Exception: + pass self.role_key = 'network' self.admin_methods = ['PUT', 'POST', 'DELETE'] self.uri_fmt = "/networks/%s" self.activate = self.generate_action_handler('activate') - self.deactivate = self.generate_action_handler('deactivate') + self.deactivate = self.generate_action_handler('deactivate', \ + ignore_not_found=_ignore_not_found) @property def data(self): diff --git a/src/kimchi/model/networks.py b/src/kimchi/model/networks.py index 7265c64..969d922 100644 --- a/src/kimchi/model/networks.py +++ b/src/kimchi/model/networks.py @@ -261,7 +261,11 @@ def lookup(self, name): 'in_use': self._is_network_in_use(name), 'autostart': network.autostart() == 1, 'state': network.isActive() and "active" or "inactive", - 'persistent': True if network.isPersistent() else False} + 'persistent': self.is_persistent(name)} + + def is_persistent(self, name): + network = self.get_network(self.conn.get(), name) + return True if network.isPersistent() else False def _is_network_in_use(self, name): # The network "default" is used for Kimchi proposal and should not be -- 1.9.3
Ignore the exception on GET for Stop (deactivate) of transient Storage Pools. --- src/kimchi/control/storagepools.py | 11 +++++++++-- src/kimchi/model/storagepools.py | 8 ++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/kimchi/control/storagepools.py b/src/kimchi/control/storagepools.py index 872ad04..e55a85f 100644 --- a/src/kimchi/control/storagepools.py +++ b/src/kimchi/control/storagepools.py @@ -19,7 +19,6 @@ import cherrypy - from kimchi.control.base import Collection, Resource from kimchi.control.storagevolumes import IsoVolumes, StorageVolumes from kimchi.control.utils import get_class_name, model_fn @@ -75,11 +74,19 @@ def _get_resources(self, filter_params): class StoragePool(Resource): def __init__(self, model, ident): super(StoragePool, self).__init__(model, ident) + _ignore_not_found = False + try: + # the test driver causes an exception in is_persistent() + _ignore_not_found = \ + not model.storagepool_is_persistent(self.ident.decode('utf-8')) + except Exception: + pass self.role_key = 'storage' self.admin_methods = ['PUT', 'POST', 'DELETE'] self.uri_fmt = "/storagepools/%s" self.activate = self.generate_action_handler('activate') - self.deactivate = self.generate_action_handler('deactivate') + self.deactivate = self.generate_action_handler('deactivate', \ + ignore_not_found = _ignore_not_found) self.storagevolumes = StorageVolumes(self.model, ident) @property diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index e03c6bb..f7448d1 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -252,7 +252,7 @@ def lookup(self, name): pool = self.get_storagepool(name, self.conn) info = pool.info() autostart = True if pool.autostart() else False - persistent = True if pool.isPersistent() else False + persistent = self.is_persistent(name) xml = pool.XMLDesc(0) path = xpath_get_text(xml, "/pool/target/path")[0] pool_type = xpath_get_text(xml, "/pool/@type")[0] @@ -281,7 +281,7 @@ def lookup(self, name): 'nr_volumes': nr_volumes, 'persistent': persistent} - if not pool.isPersistent(): + if not persistent: # Deal with deep scan generated pool try: with self.objstore as session: @@ -293,6 +293,10 @@ def lookup(self, name): pass return res + def is_persistent(self, name): + pool = self.get_storagepool(name, self.conn) + return True if pool.isPersistent() else False + def _update_lvm_disks(self, pool_name, disks): # check if all the disks/partitions exists in the host for disk in disks: -- 1.9.3
participants (2)
-
Aline Manera -
Christy Perez