[PATCH] [Kimchi] Bug fix: Remove storage volume file while removing the storage volume
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@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) -- 2.5.0
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. 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@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)
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.
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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?
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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.
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
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@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@ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Reviewed-by: Paulo Vital <pvital@linux.vnet.ibm.com> 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@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)
participants (4)
-
Aline Manera -
Paulo Ricardo Paz Vital -
Paulo Vital
-
Rodrigo Trujillo