Re: [Kimchi-devel] [PATCH] [Kimchi] Bug fix: Remove storage volume file while removing the storage volume

On 03/04/2016 01:28 PM, Paulo Ricardo Paz Vital wrote: > > On 03/04/2016 11:45 AM, Rodrigo Trujillo wrote: >> >> On 03/04/2016 10:27 AM, Aline Manera wrote: >>> >>> On 03/04/2016 10:23 AM, Aline Manera wrote: >>>> >>>> On 03/04/2016 08:06 AM, Paulo Ricardo Paz Vital wrote: >>>>> I think user should choose if he/she really wants remove the file >>>>> from >>>>> the system. It's a common approach ask user before execute the >>>>> deletion >>>>> if he/she wants. >>>>> >>>>> My suggestion is add an additional parameter as a flag to execute >>>>> your >>>>> try block if True. >>>> I have thought about that. But it will be a new feature instead of a >>>> bug fix and my intention here was to fix the test case is failing. >>>> >>> Sorry, I was thinking about the UI instead the backend when I replied >>> to you. >>> >>> The delete operation will have a confirmation on UI as we did for VM, >>> for example, but no additional parameter is needed on backend to >>> confirm this action. >>> >>> So when we will have the UI to manage storage volume, a confirmation >>> box will be shown to user but once confirmed a DELETE request is sent >>> without additional parameter. >>> >>> Does that make sense? >>> >>> >> I think that Paulo is saying to create a way to let user choose if he >> wants to delete or not the image file from the system. >> Like: >> >> def delete(self, pool, name, wipe=True): >> >> If wipe is False, then you should keep the file. Thinking on backend >> perspective (API) it is good option. >> >> Rodrigo >> > Yeah, Rodrigo got the main idea, but my idea is to the opposite - if > True then remove the file. > > However, if the idea is really remove you can use what Rodrigo said. Hmm... I understand it is a nice to have feature, but Kimchi must have a default behavior. And the default behavior IMO should be to delete the file (the same approach we have when deleting a virtual machine and, in which, this delete parameter also makes sense if you think about it). Thinking about the Kimchi user (who does not have virtualization background), when deleting a storage volume it is supposed that the storage volume will go away. Why should he/she answer a question "Do you want to remove the file associated to this storage volume?" ? It may confuse the user IMO. So, I keep thinking when deleting the storage volume, the default behavior should be to remove the file. (and that would be the behavior used on UI). But I agree in having this delete parameter as a nice to have feature for the next release (as we are on code freeze for 2.1 release)

>>>>> Paulo Vital. >>>>> >>>>> On 03/03/2016 05:42 PM, Aline Manera wrote: >>>>>> When removing a storage volume, it was only removed from a libvirt >>>>>> perspective, ie, the storage volume file kept in the system. >>>>>> It may confuse user as he/she will not be able to create a new >>>>>> storage >>>>>> volume with the same name from the storage volume removed before as >>>>>> the file >>>>>> exists in the system. >>>>>> To avoid it, remove the file from the system after removing it >>>>>> from a >>>>>> libvirt perspective. >>>>>> >>>>>> This issue was identified due a failure in the test suite. >>>>>> 2 files related to storage volume tests were kept on the system >>>>>> causing >>>>>> errors when running the test suite multiple times. >>>>>> >>>>>> To verify this patch, please, make sure to do not have any >>>>>> leftovers in the >>>>>> 'default' storage pool directory (/var/lib/libvirt/images). >>>>>> After that, run the test suite mutliple times and you will see no >>>>>> leftovers will be in the system when the test completes. >>>>>> >>>>>> Signed-off-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; >>>>>> --- >>>>>> model/storagevolumes.py | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/model/storagevolumes.py b/model/storagevolumes.py >>>>>> index d010bcd..f87738f 100644 >>>>>> --- a/model/storagevolumes.py >>>>>> +++ b/model/storagevolumes.py >>>>>> @@ -367,12 +367,19 @@ class StorageVolumeModel(object): >>>>>> raise InvalidParameter("KCHVOL0012E", {'type': >>>>>> pool_info['type']}) >>>>>> >>>>>> volume = StorageVolumeModel.get_storagevolume(pool, name, >>>>>> self.conn) >>>>>> + vol_path = volume.path() >>>>>> try: >>>>>> volume.delete(0) >>>>>> except libvirt.libvirtError as e: >>>>>> raise OperationFailed("KCHVOL0010E", >>>>>> {'name': name, 'err': >>>>>> e.get_error_message()}) >>>>>> >>>>>> + try: >>>>>> + os.remove(vol_path) >>>>>> + except OSError, e: >>>>>> + wok_log.error("Unable to delete storage volume file: >>>>>> %s." >>>>>> + "Details: %s" % (pool_info['path'], >>>>>> e.message)) >>>>>> + >>>>>> def resize(self, pool, name, size): >>>>>> volume = StorageVolumeModel.get_storagevolume(pool, name, >>>>>> self.conn) >>>>>> >>>>> _______________________________________________ >>>>> Kimchi-devel mailing list >>>>> Kimchi-devel(a)ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>>> >>>> _______________________________________________ >>>> Kimchi-devel mailing list >>>> Kimchi-devel(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>> >>> _______________________________________________ >>> Kimchi-devel mailing list >>> Kimchi-devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>> >> _______________________________________________ >> Kimchi-devel mailing list >> Kimchi-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >> > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel >