[Kimchi-devel] [PATCH] [Kimchi] Bug fix: Remove storage volume file while removing the storage volume
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Mar 7 15:05:23 UTC 2016
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 at linux.vnet.ibm.com>
>>>>>> ---
>>>>>> 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 at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>>>
>>>> _______________________________________________
>>>> Kimchi-devel mailing list
>>>> Kimchi-devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>>
>>> _______________________________________________
>>> Kimchi-devel mailing list
>>> Kimchi-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
> _______________________________________________
> 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