[Kimchi-devel] [PATCH] [Kimchi] Bug fix: Remove storage volume file while removing the storage volume
Paulo Ricardo Paz Vital
pvital at linux.vnet.ibm.com
Mon Mar 7 17:06:01 UTC 2016
On 03/07/2016 12:05 PM, Aline Manera wrote:
>
>
> 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)
>
Ok, so let's add this as future feature.
>
>>>>>> 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