[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 21:23:15 UTC 2016 


On 03/07/2016 02:06 PM, Paulo Ricardo Paz Vital wrote:
>
> 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.
>

I will open 2 issues (one for storage volume and other to guest) to 
cover it.

>>>>>>> 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