Re: [pve-devel] [PATCH v5 container 1/3] fix #1532: add target-storage support to migration

[Fabian Ebner <f.ebner@proxmox.com>]

Am 09.02.22 um 14:07 schrieb Fabian Grünbichler:
> re-using helpers that already exist for qemu-server. this is a
> pre-requisite for extending remote migration support to containers.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     new in v5, no GUI yet until possible wrinkles are ironed out
>     
>     requires pve-common/pve-guest-common with changes from this series
> 
>  src/PVE/API2/LXC.pm    | 32 ++++++++++++++++++++++
>  src/PVE/LXC/Migrate.pm | 62 ++++++++++++++++++++++++++++++++----------
>  2 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 7573814..61eaaf7 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -36,6 +36,18 @@ BEGIN {
>      }
>  }
>  
> +my $check_storage_access_migrate = sub {
> +    my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
> +
> +    PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
> +
> +    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
> +
> +    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
> +    die "storage '$storage' does not support CT rootdirs\n"
> +	if !$scfg->{content}->{rootdir};
> +};
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::LXC::Config",
>      path => '{vmid}/config',
> @@ -1091,6 +1103,7 @@ __PACKAGE__->register_method({
>  		description => "Target node.",
>  		completion => \&PVE::Cluster::complete_migration_target,
>  	    }),
> +	    'target-storage' => get_standard_option('pve-targetstorage'),

This option is currently registered in PVE/QemuServer.pm and I don't
think we want to depend on that ;)

Also, the parameter is named 'targetstorage' for VMs. I do agree this
one is nicer, but it might be confusing.

>  	    online => {
>  		type => 'boolean',
>  		description => "Use online/live migration.",
> @@ -1149,6 +1162,25 @@ __PACKAGE__->register_method({
>  		if !$param->{online} && !$param->{restart};
>  	}
>  
> +	if (my $targetstorage = delete $param->{'target-storage'}) {
> +	    my $storecfg = PVE::Storage::config();
> +	    my $storagemap = eval { PVE::JSONSchema::parse_idmap($targetstorage, 'pve-storage-id') };
> +	    raise_param_exc({ targetstorage => "failed to parse storage map: $@" })

Should be 'target-storage' if we go with that.

> +		if $@;
> +
> +	    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk'])
> +		if !defined($storagemap->{identity});
> +
> +	    foreach my $target_sid (values %{$storagemap->{entries}}) {
> +		$check_storage_access_migrate->($rpcenv, $authuser, $storecfg, $target_sid, $target);
> +	    }
> +
> +	    $check_storage_access_migrate->($rpcenv, $authuser, $storecfg, $storagemap->{default}, $target)
> +		if $storagemap->{default};
> +
> +	    $param->{storagemap} = $storagemap;
> +	}
> +
>  	if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
>  
>  	    my $hacmd = sub {
> diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
> index 95562e4..c85a09c 100644
> --- a/src/PVE/LXC/Migrate.pm
> +++ b/src/PVE/LXC/Migrate.pm

----8<----

> @@ -194,7 +210,8 @@ sub phase1 {
>  	next if @{$dl->{$storeid}} == 0;
>  
>  	# check if storage is available on target node
> -	PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, $self->{node});
> +	my $targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $storeid);
> +	PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
>  
>  	die "content type 'rootdir' is not available on storage '$storeid'\n"
>  	    if !$scfg->{content}->{rootdir};

Should use target's scfg.

> @@ -275,25 +292,38 @@ sub phase1 {
>  	next if $rep_volumes->{$volid};
>  	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>  	push @{$self->{volumes}}, $volid;
> -	my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$sid], $opts->{bwlimit});
> +
>  	# JSONSchema and get_bandwidth_limit use kbps - storage_migrate bps
> +	my $bwlimit = $volhash->{$volid}->{bwlimit};
>  	$bwlimit = $bwlimit * 1024 if defined($bwlimit);
>  
> -	my $storage_migrate_opts = {
> -	    'ratelimit_bps' => $bwlimit,
> -	    'insecure' => $opts->{migration_type} eq 'insecure',
> -	    'with_snapshots' => $volhash->{$volid}->{snapshots},
> +	my $targetsid = $volhash->{$volid}->{targetsid};
> +
> +	my $new_volid = eval {
> +	    my $storage_migrate_opts = {
> +		'ratelimit_bps' => $bwlimit,
> +		'insecure' => $opts->{migration_type} eq 'insecure',
> +		'with_snapshots' => $volhash->{$volid}->{snapshots},

Since we update the config below, I think we can now also enable
allow_rename here. Otherwise, it's rather easy to run into conflicts
when mapping two storages on the source side to the same one on the target.

> +	    };
> +
> +	    my $logfunc = sub { $self->log('info', $_[0]); };
> +	    return PVE::Storage::storage_migrate(
> +		$self->{storecfg},
> +		$volid,
> +		$self->{ssh_info},
> +		$targetsid,
> +		$storage_migrate_opts,
> +		$logfunc,
> +	    );
>  	};
>  
> -	my $logfunc = sub { $self->log('info', $_[0]); };
> -	eval {
> -	    PVE::Storage::storage_migrate($self->{storecfg}, $volid, $self->{ssh_info},
> -					  $sid, $storage_migrate_opts, $logfunc);
> -	};
>  	if (my $err = $@) {
> -	    die "storage migration for '$volid' to storage '$sid' failed - $err\n";
> +	    die "storage migration for '$volid' to storage '$targetsid' failed - $err\n";
>  	}
>  
> +	$self->{volume_map}->{$volid} = $new_volid;
> +	$self->log('info', "volume '$volid' is '$new_volid' on the target\n");
> +
>  	eval { PVE::Storage::deactivate_volumes($self->{storecfg}, [$volid]); };
>  	if (my $err = $@) {
>  	    $self->log('warn', $err);
> @@ -316,6 +346,8 @@ sub phase1 {
>  
>      # transfer replication state before moving config
>      $self->transfer_replication_state() if $rep_volumes;
> +    PVE::LXC::Config->update_volume_ids($conf, $self->{volume_map});
> +    PVE::LXC::Config->write_config($vmid, $conf);
>      PVE::LXC::Config->move_config_to_node($vmid, $self->{node});
>      $self->{conf_migrated} = 1;
>      $self->switch_replication_job_target() if $rep_volumes;