[pve-devel] [RFC manager] Storage: remove unreferend images in storage contendview

[Dominik Csapak]

a general remark:

the logic is not quite correct

i doubt we want to actually remove volumes that belong to an
existing vm but are not referenced, this is contrary to the function
of 'qm rescan'

i would propose another approach:

make 'qm rescan' (and pct rescan) a button in the gui,
then the users can delete the images at the vm if they need to

and if there exists no vm to the image, then we have to
search if it is referenced anywhere (e.g.
if someone added 'vm-100-disk-1' to vm 200)

generally it should not be necessary to delete images manually
since we clean up? or are there some conditions which produces
left over images (aside from bugs of course)

some further comments/nitpicks inline


On 10/5/18 12:48 PM, Wolfgang Link wrote:
>      If an image on storage has not referenced to any guest config,
>      we can safely delete it on the GUI.
>      Images that are marked as unused in the config are referred.
> 
>      This call can't be in the storage because of cycles dependencies.
> ---
>   PVE/API2/Nodes.pm                   | 86 +++++++++++++++++++++++++++++++++++++
>   www/manager6/storage/ContentView.js | 62 +++++++++++++++++++++-----
>   2 files changed, 137 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 49502830..80ba5e4d 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -187,6 +187,8 @@ __PACKAGE__->register_method ({
>   	    { name => 'certificates' },
>   	    { name => 'config' },
>   	    { name => 'hosts' },
> +	    { name => 'deleteimage' },
> +
>   	    ];

nitpick: the empty line is unnecessary

>   
>   	return $result;
> @@ -632,6 +634,90 @@ __PACKAGE__->register_method({
>   my $sslcert;
>   
>   __PACKAGE__->register_method ({
> +    name => 'deleteimage',
> +    path => 'deleteimage/{volume}',
> +    method => 'DELETE',
> +    protected => 1,
> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/storage/{storage}', ['Datastore.Allocate'], any => 1],
> +    },
> +    description => "Remove image which is not referred to any Config.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    volume => {
> +		description => "Volume identifier",
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => "string",
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $volume = PVE::Tools::extract_param($param, 'volume');
> +	my $node = PVE::Tools::extract_param($param, 'node');
> +
> +	my $storagecfg = PVE::Storage::config();
> +	my ($sid, $volname) = PVE::Storage::parse_volume_id($volume);
> +
> +	$rpcenv->check($authuser, "/storage/$sid", ['Datastore.Allocate']);
> +
> +	my $vms = PVE::Cluster::get_vmlist();
> +
> +	my $lookup_guest_class = sub {
> +	    my ($vmtype) = @_;
> +
> +	    if ($vmtype eq 'qemu') {
> +		return 'PVE::QemuConfig';
> +	    } elsif ($vmtype eq 'lxc') {
> +		return 'PVE::LXC::Config';
> +	    } else {
> +		die "unknown guest type '$vmtype' - internal error";
> +	    }
> +	};
> +
> +	foreach my $key_vmid (keys %{$vms->{ids}}) {
> +	    my $vm = $vms->{ids}->{$key_vmid};
> +	    my $vmtype = $vm->{type};
> +
> +	    my $guest_class = &$lookup_guest_class($vmtype);
> +	    my $guest_conf = $guest_class->load_config($key_vmid, $vm->{node});
> +
> +	    foreach my $key (keys %$guest_conf) {
> +		if ($key =~ /^(unused|ide|sata|scsi|virtio)\d+$/) {
> +		    my $volid = $guest_conf->{$key};
> +
> +		    next if $volid =~ /media=cdrom/ && $volid !~ /cloudinit/;
> +		    my ($local_sid, $local_volname) =
> +			PVE::Storage::parse_volume_id($volid);
> +
> +		    if ($node ne $vm->{node} &&
> +			!($storagecfg->{ids}->{$sid}->{shared} == 1)) {
> +			next;
> +		    }

would that not mean that you can delete referenced images on non shared 
storage (e.g. replicated disks)? and every image that is mapped as a 
cdrom and has cloudinit in the name?

> +
> +		    if ($local_sid eq $sid && $local_volname =~ /$volname/) {
> +			return  "Image: $volume is referred in config: $key_vmid\n";
> +		    }
> +		}
> +	    }
> +	}

that whole logic can only ever work for vms, never for containers since
container do not have ide/sata/etc. but rootfs/mpX

> +
> +	eval {
> +	    PVE::Storage::vdisk_free ($storagecfg, $volume);
> +	};
> +	return @_;
> +    }});
> +
> +__PACKAGE__->register_method ({
>       name => 'vncshell',
>       path => 'vncshell',
>       method => 'POST',
> diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
> index 0661065d..136fb924 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -373,6 +373,55 @@ Ext.define('PVE.storage.ContentView', {
>   
>   	Proxmox.Utils.monStoreErrors(me, store);
>   
> +	var removeButton = Ext.create('Proxmox.button.StdRemoveButton',{
> +	    selModel: sm,
> +	    enableFn: function(rec) {
> +		if (rec && rec.data.content !== 'images') {
> +		    removeButton.setVisible(true);
> +		    imageRemoveButton.setVisible(false);
> +		    return true;
> +		}
> +		return false;
> +	    },
> +	    callback: function() {
> +		reload();
> +	    },
> +	    baseurl: baseurl + '/'
> +	});
> +
> +	var imageRemoveButton = Ext.create('Proxmox.button.StdRemoveButton',{
> +	    selModel: sm,
> +
> +	    enableFn: function(rec) {
> +		if (rec && rec.data.content === 'images') {
> +		    removeButton.setVisible(false);
> +		    imageRemoveButton.setVisible(true);
> +		    return true;
> +		}
> +		return false;
> +	    },
> +
> +	    handler: function(btn, event, rec) {
> +		Proxmox.Utils.API2Request({
> +		    url: '/nodes/' + nodename + '/deleteimage/' + rec.data.volid ,
> +		    method: 'DELETE',
> +		    waitMsgTarget: me.waitMsgTarget,
> +		    callback: function(options, success, response) {
> +			Ext.callback(me.callback, me.scope, [options, success, response], 0, me);
> +			reload();
> +		    },
> +		    failure: function (response, opts) {
> +			Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> +		    },
> +		    success: function(response, opts) {
> +			if (response.result.data !== 1) {
> +			    Ext.Msg.alert(gettext('Error'), response.result.data);
> +			}
> +		    }
> +		});
> +	    },

would not make more sense to set the baseurl here to 
'/nodes/nodename/deleteimage/'  instead of writing a custom handler?

> +	});
> +
>   	var templateButton = Ext.create('Proxmox.button.Button',{
>   	    itemId: 'tmpl-btn',
>   	    text: gettext('Templates'),
> @@ -437,17 +486,8 @@ Ext.define('PVE.storage.ContentView', {
>   			win.on('destroy', reload);
>   		    }
>   		},
> -		{
> -		    xtype: 'proxmoxStdRemoveButton',
> -		    selModel: sm,
> -		    enableFn: function(rec) {
> -			return rec && rec.data.content !== 'images';
> -		    },
> -		    callback: function() {
> -			reload();
> -		    },
> -		    baseurl: baseurl + '/'
> -		},
> +		removeButton,
> +		imageRemoveButton,
>   		templateButton,
>   		uploadButton,
>   		{
>