[PATCH v3 0/4] Transient Object Handling
In order to prevent benign error messages from being displayed to the user, some POST requests for transient objects should be handled as special cases. This patchset introduces a way to mark an action as destructive so that the subsequent GET after a POST is bypassed. It also adds that logic to 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 | 14 +++++++++----- src/kimchi/control/networks.py | 4 ++-- src/kimchi/control/storagepools.py | 4 ++-- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 6 files changed, 23 insertions(+), 13 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 receive an error. However, this is not an error condition. This patch does not call the GET function for transient objects if the function will delete the object (as indicated by a new parameter passed to generate_action_handler). Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index 484e9b9..04a846c 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -71,7 +71,8 @@ def _redirect(self, action_result, code=303): 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, + destructive=False): def _render_element(self, ident): self._redirect(ident) uri_params = [] @@ -83,7 +84,8 @@ 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) + destructive=destructive, + action_args=action_args) def generate_action_handler_task(self, action_name, action_args=None): def _render_task(self, task): @@ -91,10 +93,10 @@ def _render_task(self, task): return kimchi.template.render('Task', task) return self._generate_action_handler_base(action_name, _render_task, - action_args) + action_args=action_args) def _generate_action_handler_base(self, action_name, render_fn, - action_args=None): + destructive=False, action_args=None): def wrapper(*args, **kwargs): validate_method(('POST'), self.role_key, self.admin_methods) try: @@ -109,7 +111,9 @@ def wrapper(*args, **kwargs): action_fn = getattr(self.model, model_fn(self, action_name)) action_result = action_fn(*model_args) - return render_fn(self, action_result) + if destructive is False or \ + ('persistent' in self.info.keys() and self.info['persistent'] is True): + return render_fn(self, action_result) except MissingParameter, e: raise cherrypy.HTTPError(400, e.message) except InvalidParameter, e: -- 1.9.3
Do not call GET for PowerOff, Shutdown, and Reset actions for transient VMs. Also return an error for Delete of transient VMs, since libvirt does not support this operation. Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index 1a84b6c..dda4b31 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): @@ -40,9 +39,12 @@ def __init__(self, model, ident): 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', + destructive=True) + self.shutdown = self.generate_action_handler('shutdown', + destructive=True) + self.reset = self.generate_action_handler('reset', + destructive=True) 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 9987786..86a2a1f 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -836,6 +836,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
Set the Deactivate action as a destructive one so that the GET will not be called for transient networks afterward. Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/networks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index 7cb1572..22fc3bb 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): @@ -37,7 +36,8 @@ def __init__(self, model, ident): 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', \ + destructive=True) @property def data(self): -- 1.9.3
Question: does libvirt supports delete a non-persistent network or storage pool? Is that restriction only for guests? On 27/01/2015 16:39, Christy Perez wrote:
Set the Deactivate action as a destructive one so that the GET will not be called for transient networks afterward.
Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/networks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index 7cb1572..22fc3bb 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): @@ -37,7 +36,8 @@ def __init__(self, model, ident): 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', \ + destructive=True)
@property def data(self):
On 01/30/2015 08:35 AM, Aline Manera wrote:
Question: does libvirt supports delete a non-persistent network or storage pool? Is that restriction only for guests?
For storage pools, you can't delete an active pool. Since deactivating a transient pool removes it, you'll never be given the option of deleting a transient pool. IOW, it's not something we need to take into account. For networks, there is no delete. There's only destroy. So, yes, this restriction need only be enforced for guests.
On 27/01/2015 16:39, Christy Perez wrote:
Set the Deactivate action as a destructive one so that the GET will not be called for transient networks afterward.
Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/networks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index 7cb1572..22fc3bb 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): @@ -37,7 +36,8 @@ def __init__(self, model, ident): 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', \ + destructive=True)
@property def data(self):
Set the Deactivate action as a destructive one so that the GET will not be called for transient storage pools afterward. Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/control/storagepools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kimchi/control/storagepools.py b/src/kimchi/control/storagepools.py index 872ad04..ba388e8 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 @@ -79,7 +78,8 @@ def __init__(self, model, ident): 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', + destructive=True) self.storagevolumes = StorageVolumes(self.model, ident) @property -- 1.9.3
Code is good for me, testcases lacked, mockmodel supports create transient network and try to stop it. On 01/27/2015 01:39 PM, Christy Perez wrote:
In order to prevent benign error messages from being displayed to the user, some POST requests for transient objects should be handled as special cases.
This patchset introduces a way to mark an action as destructive so that the subsequent GET after a POST is bypassed. It also adds that logic to 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 | 14 +++++++++----- src/kimchi/control/networks.py | 4 ++-- src/kimchi/control/storagepools.py | 4 ++-- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 6 files changed, 23 insertions(+), 13 deletions(-)
Please, run "make check-local" to fix the pep8 issues. On 27/01/2015 16:39, Christy Perez wrote:
In order to prevent benign error messages from being displayed to the user, some POST requests for transient objects should be handled as special cases.
This patchset introduces a way to mark an action as destructive so that the subsequent GET after a POST is bypassed. It also adds that logic to 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 | 14 +++++++++----- src/kimchi/control/networks.py | 4 ++-- src/kimchi/control/storagepools.py | 4 ++-- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 6 files changed, 23 insertions(+), 13 deletions(-)
The only pep8 issue I see is the one that's always there (that wasn't caused by this change): Checking for invalid i18n string... KCHUTILS0003E is obsolete, it is no longer in use make: *** [check-local] Error 1 Do you see a different one? On 01/30/2015 08:40 AM, Aline Manera wrote:
Please, run "make check-local" to fix the pep8 issues.
On 27/01/2015 16:39, Christy Perez wrote:
In order to prevent benign error messages from being displayed to the user, some POST requests for transient objects should be handled as special cases.
This patchset introduces a way to mark an action as destructive so that the subsequent GET after a POST is bypassed. It also adds that logic to 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 | 14 +++++++++----- src/kimchi/control/networks.py | 4 ++-- src/kimchi/control/storagepools.py | 4 ++-- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 6 files changed, 23 insertions(+), 13 deletions(-)
On 30/01/2015 14:11, Christy Perez wrote:
The only pep8 issue I see is the one that's always there (that wasn't caused by this change):
Checking for invalid i18n string... KCHUTILS0003E is obsolete, it is no longer in use make: *** [check-local] Error 1
Do you see a different one?
Yeap! Probably we are using a different pep8 version. alinefm@alinefm:~/kimchi$ sudo make check-local [sudo] password for alinefm: PYTHONPATH=src contrib/check_i18n.py plugins/*/i18n.py src/kimchi/i18n.py Checking for invalid i18n string... Checking for invalid i18n string successfully find . -path './.git' -prune -type f -o \ -name '*.py' -o -name '*.py.in' | xargs /usr/bin/pyflakes | \ grep -w -v "\./src/kimchi/websocket\.py" | \ while read LINE; do echo "$LINE"; false; done /usr/bin/pep8 --version 1.5.6 /usr/bin/pep8 --filename '*.py,*.py.in' --exclude="*src/kimchi/config.py,*src/kimchi/i18n.py,*tests/test_config.py" . ./src/kimchi/control/base.py:115:18: E127 continuation line over-indented for visual indent ./src/kimchi/control/base.py:115:80: E501 line too long (88 > 79 characters) ./src/kimchi/control/networks.py:23:1: E302 expected 2 blank lines, found 1 ./src/kimchi/control/networks.py:39:70: E502 the backslash is redundant between brackets ./src/kimchi/control/networks.py:40:13: E128 continuation line under-indented for visual indent ./src/kimchi/control/storagepools.py:82:13: E128 continuation line under-indented for visual indent ./src/kimchi/control/vms.py:24:1: E302 expected 2 blank lines, found 1 ./src/kimchi/control/vms.py:43:13: E128 continuation line under-indented for visual indent ./src/kimchi/control/vms.py:45:13: E128 continuation line under-indented for visual indent ./src/kimchi/control/vms.py:47:13: E128 continuation line under-indented for visual indent Makefile:828: recipe for target 'check-local' failed make: *** [check-local] Error 1
On 01/30/2015 08:40 AM, Aline Manera wrote:
Please, run "make check-local" to fix the pep8 issues.
On 27/01/2015 16:39, Christy Perez wrote:
In order to prevent benign error messages from being displayed to the user, some POST requests for transient objects should be handled as special cases.
This patchset introduces a way to mark an action as destructive so that the subsequent GET after a POST is bypassed. It also adds that logic to 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 | 14 +++++++++----- src/kimchi/control/networks.py | 4 ++-- src/kimchi/control/storagepools.py | 4 ++-- src/kimchi/control/vms.py | 10 ++++++---- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 3 +++ 6 files changed, 23 insertions(+), 13 deletions(-)
participants (3)
-
Aline Manera -
Christy Perez -
Royce Lv