[Kimchi-devel] [PATCH 2/4] Transient VM POST request handling

[Aline Manera]

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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>