[pve-devel] [PATCH #1941 qemu-server] Clean up empty image subdirectories on dir based storage on VM destruction

Thomas Lamprecht t.lamprecht at proxmox.com
Fri Jan 18 09:14:00 CET 2019


On 1/17/19 5:10 PM, Christian Ebner wrote:
> No, I had no special reason to put it there, I was just looking for the seemingly easiest place to clean up the directory.
> 
> I will implement it for the Dir plugin if this is more suitable!

pve-storage, and its plugin system, is here to abstract the specifics and
detailed semantics away, as much as possible and as much it makes sense.
So, if a specific storage type (in this case directory based storage) needs
some special handling it should be handled by the respective plugin itself,
so all plugin users can profit from this and less tight-coupling is produced.
This is not always possible, there are cases where the thing one wants to do
is inherently coupled and splitting it apart may make things even more
complicated or add much overhead, but that is not, or at least, should not be
often the case, just to get you some rationale why this seemed a bit off to me.

> 
> Thx
>> On January 17, 2019 at 4:56 PM Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:
>>
>>
>> On 1/17/19 12:13 PM, Christian Ebner wrote:
>>> Fix #1941
>>
>> It's a bit of an untold convention in PVE to put above in the beginning of
>> the commit messages subject line, makes it easier for some of us to copy the
>> line for the package changelog.
>>
>>>
>>> Removes the empty vmid subdirectories when a VM gets destroyed and all the
>>> contained image files are deleted.
>>
>> Hmm, any reason that you do not do this in pve-storage?
>> Without thinking to much it seems to me that this is not the job of qemu-server,
>> it shouldn't care about such details, it only wants to free an image?
>> Or did you have specific reasons to put it here?
>>
>> Else, maybe you could integrate something like this in the Dir plugin, which
>> in this case means the base Plugin class DirPlug bases on, as it inherits the
>> free_image method.
>>
>>>
>>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>>> ---
>>>  PVE/QemuServer.pm | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index 1ccdccf..da4be85 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -2502,6 +2502,8 @@ sub destroy_vm {
>>>   	return if drive_is_cdrom($drive, 1);
>>>  
>>>  	my $volid = $drive->{file};
>>> +	my $storeid = PVE::Storage::parse_volume_id($volid);
>>> +	my $storetype = $storecfg->{ids}->{$storeid}->{type};
>>>  
>>>  	return if !$volid || $volid =~ m|^/|;
>>>  
>>> @@ -2513,6 +2515,12 @@ sub destroy_vm {
>>>  	};
>>>  	warn "Could not remove disk '$volid', check manually: $@" if $@;
>>>  
>>> +	# cleanup empty directroies on dir based storage
>>> +	if ($storetype eq 'dir') {
>>> +	    my $dir = dirname($path);
>>> +	    rmdir($dir);
>>> +	}
>>> +
>>>      });
>>>  
>>>      if ($keep_empty_config) {
>>>
>>




More information about the pve-devel mailing list