[pve-devel] [RFC guest-common 1/16] Implement update_volume_ids and add required helpers: foreach_volume and print_volume

[Fabian Grünbichler]

On January 29, 2020 2:29 pm, Fabian Ebner wrote:
> This function is intened to be used after doing a migration where some
> of the volume IDs changed.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
>  PVE/AbstractConfig.pm | 61 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
> index a94a379..fb833cb 100644
> --- a/PVE/AbstractConfig.pm
> +++ b/PVE/AbstractConfig.pm
> @@ -366,6 +366,67 @@ sub get_replicatable_volumes {
>      die "implement me - abstract method\n";
>  }
>  
> +sub foreach_volume {
> +    my ($class, $conf, $func, @param) = @_;
> +
> +    die "abstract method - implement me\n";
> +}
> +
> +sub print_volume {
> +    my ($class, $volume) = @_;
> +
> +    die "abstract method - implement me\n";
> +}

if we do this, we probably also want a parse_volume here? see comments 
on qemu-server #12

> +
> +# $volume_map is a hash of 'old_volid' => 'new_volid' pairs.
> +# This method replaces 'old_volid' by 'new_volid' throughout
> +# the config including snapshots, both for volumes appearing in
> +# foreach_volume as well as vmstate and unusedN values.
> +sub update_volume_ids {
> +    my ($class, $conf, $volume_map) = @_;
> +
> +    my $newconf = {};

why not modify the config in place? you replace the old one with the 
returned new one anyway in the single caller, and write it out directly 
afterwards ;) it would make the code shorter, and more inline with how 
we usually modify $conf

> +
> +    my $do_replace = sub {
> +	my ($key, $volume, $newconf, $volume_map) = @_;

no need to pass $newconf and $volume_map as parameter, the one from 
update_volume_ids is accessible here anyway.

> +
> +	my $old_volid = $volume->{file} // $volume->{volume};

it might make sense to expose this (under which key the volid is stored) 
via AbstractConfig/QemuConfig/LXC::Config, Aaron's backup status patch series also 
needs this information in pve-manager.

> +	if (my $new_volid = $volume_map->{$old_volid}) {
> +	    $volume->{file} = $new_volid if defined($volume->{file});
> +	    $volume->{volume} = $new_volid if defined($volume->{volume});

which would make this a single line

> +	    $newconf->{$key} = $class->print_volume($volume);
> +	}
> +    };
> +
> +    my $replace_volids = sub {
> +	my ($conf) = @_;
> +
> +	my $newconf = {};
> +	foreach my $key (keys %{$conf}) {
> +	    next if $key =~ m/^snapshots$/;
> +	    # these keys are not handled by foreach_volume

would it make sense to include them optionally?

we have lots of use cases where we really want to iterate over ALL the 
currently referenced volumes.. I know both of them only have a volid, 
but we could just set that into $parsed->{file} or $parsed->{volume} and 
leave the rest empty.  if you call foreach_volume with 
$opts->{include_vmstate} or $opts->{include_unused} you of course need 
to be able to handle them properly, and we'd need to look whether 
pve-container can easily support such an interface as well.

> +	    if ($key =~ m/^(vmstate)|(unused\d+)$/) {
> +		my $old_volid = $conf->{$key};
> +		$newconf->{$key} = $volume_map->{$old_volid};

in-place, this could become

$conf->{$key} = $volume_map->{$conf->{$key}};

> +	    }
> +	    $newconf->{$key} = $conf->{$key} if !defined($newconf->{$key});

not needed for in-place

> +	}
> +
> +	$class->foreach_volume($conf, $do_replace, $newconf, $volume_map);
> +	return $newconf;
> +    };
> +
> +    $newconf = $replace_volids->($conf);
> +    foreach my $snap (keys %{$conf->{snapshots}}) {
> +	my $newsnap = $replace_volids->($conf->{snapshots}->{$snap});
> +	foreach my $k (keys %{$newsnap}) {
> +	    $newconf->{snapshots}->{$snap}->{$k} = $newsnap->{$k};
> +	}

this second foreach should not be needed:

foreach my $snap (keys %${$conf->{snapshots}}) {
    $newconf->{snapshots}->{$snap} = $replace_volids->($conf->{snapshots}->{$snap});
}

(or $conf-> if we drop $newconf)

> +    }
> +
> +    return $newconf;
> +}
> +
>  # Internal snapshots
>  
>  # NOTE: Snapshot create/delete involves several non-atomic
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>