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

[Christian Ebner]

Thanks for the feedback!

I totally agree with your reasoning, I was not yet aware of the plugin system.
Will implement it in the pve-storage Dir plugin.

> On January 18, 2019 at 9:14 AM Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:
> 
> 
> 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) {
> >>>
> >>