[pve-devel] [PATCH container] Close #1234: pct: implement rescan

[Wolfgang Bumiller]

On Tue, Sep 18, 2018 at 11:16:17AM +0200, Alwin Antreich wrote:
> This patch implements the same feature as already exists for qm
> 'rescan'. With options for updating the disksize or the unused disks
> only.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
> Sure some methods can be migrated into the guest-common. Comments appreciated.
> 
>  src/PVE/CLI/pct.pm |  44 ++++++++++++++++
>  src/PVE/LXC.pm     | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 193 insertions(+)
> 
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index 6296d6f..36e3fcf 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -461,6 +461,49 @@ sub create_file {
>      return $fd;
>  }
>  
> +__PACKAGE__->register_method ({
> +    name => 'rescan',
> +    path => 'rescan',
> +    method => 'POST',
> +    description => "Rescan all storages and update disk sizes and unused disk images.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', {
> +		optional => 1,
> +		completion => \&PVE::LXC::complete_ctid,
> +	    }),
> +	    dryrun => {
> +		type => 'boolean',
> +		optional => 1,
> +		default => 0,
> +		description => 'Do not actually write changes out to conifg.',
> +	    },
> +	    unusedOnly => {

Please don't introduce yet another CLI parameter style...
Use: 'unused-only'

> +		type => 'boolean',
> +		optional => 1,
> +		default => 0,
> +		description => 'Add only unsued disk(s) to the config.',
> +	    },
> +	    disksizeOnly => {

Same

Also, does it make sense to have two '*-only' parameters? They make it
sound like they were supposed to be mutually exclusive.
Perhaps it would be better to have --no-unused, --no-disksize, or simply
default-true '--update-unused', '--update-disksize'?

> +		type => 'boolean',
> +		optional => 1,
> +		default => 0,
> +		description => 'Only update the disk size in the conifg.',
> +	    },
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	print "NOTE: running in dry-run mode, won't write changes out!\n" if $param->{dryrun};
> +
> +	PVE::LXC::rescan($param->{vmid}, 0, $param);
> +
> +	return undef;
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'pull',
>      path => 'pull',
> @@ -774,6 +817,7 @@ our $cmddef = {
>      pull => [ __PACKAGE__, 'pull', ['vmid', 'path', 'destination']],
>  
>      df => [ __PACKAGE__, 'df', ['vmid']],
> +    rescan  => [ __PACKAGE__, 'rescan', []],
>  
>      destroy => [ 'PVE::API2::LXC', 'destroy_vm', ['vmid'], 
>  		 { node => $nodename }, $upid_exit ],
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 0b57ae9..e78128a 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1538,6 +1538,155 @@ sub create_disks {
>      return $vollist;
>  }
>  
> +sub update_disksize {
> +    my ($vmid, $conf, $volid_hash) = @_;

Weird naming. $volid_hash maps volume-ids to ... and contains...
AFAICT this initially contains all volumes find in any storage for the
VM. So maybe, $vm_storage_info, or $all_volumes or something?
It's particularly hard to follow this down in the update_unused()
function along with $refpath and $referenced...

> +
> +    my $changes;
> +
> +    my $update_mp = sub {
> +	my ($key, $mp, @param) = @_;
> +	my $size = $volid_hash->{$mp->{volume}}->{size};
> +
> +	if ($size != $mp->{size}) {
> +	    $changes = 1;
> +	    $mp->{size} = $size;
> +	}
> +
> +	my $skip = $key eq 'rootfs' ? 1 : 0;

The flag is for whether this is a rootfs. Its implementation contains a
$skip list because rootfs shouldn't store the 'mp' property in the
config. The flag itself isn't called $skip.
Also you don't need to enforce the 1:0 values.

> +	$conf->{$key} = PVE::LXC::Config->print_ct_mountpoint($mp, $skip);
> +    };
> +
> +    PVE::LXC::Config->foreach_mountpoint($conf, $update_mp);
> +
> +    return $changes;
> +
> +}
> +
> +sub update_unused {
> +    my ($vmid, $conf, $volid_hash) = @_;
> +
> +    my $changes;
> +    my $prefix = "CT $vmid:";
> +
> +    # Note: it is allowed to define multiple storages with same path (alias), so
> +    # we need to check both 'volid' and real 'path' (two different volid can point
> +    # to the same path).
> +
> +    # used and unused disks

The naming does not really reflect that.

> +    my $refpath = {};
> +    my $referenced = {};
> +
> +    foreach my $opt (keys %$conf) {
> +	next if ($opt !~ m/^unused(\d+)$/);

You're not referencing the capture group, so you could drop the
parentheses.

> +	my $vol = $volid_hash->{$conf->{$opt}};
> +	$refpath->{$vol->{path}} = $vol->{volid};
> +    }
> +
> +    foreach my $key (keys %$volid_hash) {
> +	my $vol = $volid_hash->{$key};
> +	my $in_use = PVE::LXC::Config->is_volume_in_use($conf, $vol->{volid});
> +

Here would be a good place for a
	my $path = $vol->{path};
;-)

> +	if ($in_use) {
> +	    $refpath->{$vol->{path}} = $key;
> +	    delete $referenced->{$vol->{path}} if $referenced->{$vol->{path}};

Do we really need the postfix `if`?

> +	} else {
> +	    if ((!$referenced->{$vol->{path}}) && (!$refpath->{$vol->{path}})) {
> +	        $referenced->{$vol->{path}} = $key;
> +	    }
> +	}
> +
> +    }
> +
> +    for my $key (keys %$referenced) {
> +	my $disk = $referenced->{$key};
> +	my $unused = PVE::LXC::Config->add_unused_volume($conf, $disk);
> +
> +	if ($unused) {
> +	    $changes = 1;
> +	    print "$prefix add unreferenced volume '$disk' as '$unused' to config.\n";
> +	}
> +    }
> +
> +    return $changes;
> +
> +}
> +
> +sub scan_volids {
> +    my ($cfg, $vmid) = @_;
> +
> +    my $info = PVE::Storage::vdisk_list($cfg, undef, $vmid);
> +
> +    my $volid_hash = {};
> +    foreach my $storeid (keys %$info) {
> +	foreach my $item (@{$info->{$storeid}}) {
> +	    next if !($item->{volid} && $item->{size});

Should we move this to pve-guest-common? In either case I'd like to see
a small improvement here: $item->{volid} is used 3 times, once as key
for another hash access, please move it out into a helper variable
($volid)...


> +	    $item->{path} = PVE::Storage::path($cfg, $item->{volid});
> +	    $volid_hash->{$item->{volid}} = $item;
> +	}
> +    }
> +
> +    return $volid_hash;
> +}
> +
> +sub rescan {
> +    my ($vmid, $nolock, $param) = @_;
> +
> +    my $cfg = PVE::Storage::config();
> +
> +    # FIXME: Remove once our RBD plugin can handle CT and VM on a single storage
> +    # see: https://pve.proxmox.com/pipermail/pve-devel/2018-July/032900.html
> +    foreach my $stor (keys %{$cfg->{ids}}) {
> +	delete($cfg->{ids}->{$stor}) if !$cfg->{ids}->{$stor}->{content}->{rootdir};
> +    }
> +
> +    print "rescan volumes...\n";
> +    my $volid_hash = scan_volids($cfg, $vmid);
> +
> +    my $updatefn =  sub {
> +	my ($vmid) = @_;
> +
> +	my $changes;
> +	my $unused = $param->{unusedOnly};
> +	my $disksize = $param->{disksizeOnly};
> +
> +	$unused = $disksize = 1 if ((!$unused) && (!$disksize));
> +
> +	my $conf = PVE::LXC::Config->load_config($vmid);
> +
> +	PVE::LXC::Config->check_lock($conf);
> +
> +	my $vm_volids = {};
> +	foreach my $volid (keys %$volid_hash) {
> +	    my $info = $volid_hash->{$volid};
> +	    $vm_volids->{$volid} = $info if $info->{vmid} && $info->{vmid} == $vmid;
> +	}
> +
> +	my $upu = update_unused($vmid, $conf, $vm_volids) if $unused;
> +	my $upd = update_disksize($vmid, $conf, $vm_volids) if $disksize;
> +	$changes = $upu // $upd;

Nit: A logical || would work the same. I find '//' in a purely boolean
context rather weird, it's what you use when you need non-'undef'
values.

> +
> +	PVE::LXC::Config->write_config($vmid, $conf) if $changes && !$param->{dryrun};
> +    };
> +
> +    if (defined($vmid)) {
> +	if ($nolock) {
> +	    &$updatefn($vmid);
> +	} else {
> +	    PVE::LXC::Config->lock_config($vmid, $updatefn, $vmid);
> +	}
> +    } else {
> +	my $vmlist = config_list();
> +	foreach my $vmid (keys %$vmlist) {
> +	    if ($nolock) {
> +		&$updatefn($vmid);
> +	    } else {
> +		PVE::LXC::Config->lock_config($vmid, $updatefn, $vmid);
> +	    }
> +	}
> +    }
> +}
> +
> +
>  # bash completion helper
>  
>  sub complete_os_templates {
> -- 
> 2.11.0