Re: [pve-devel] [PATCH qemu-server v11 3/5] migration: add check_non_migratable_resources function

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

Am 29.05.24 um 14:23 schrieb Markus Frank:
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2a1d4d7..8f36cf8 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4504,7 +4504,7 @@ __PACKAGE__->register_method({
>  	$res->{running} = PVE::QemuServer::check_running($vmid) ? 1:0;
>  
>  	my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
> -	    PVE::QemuServer::check_local_resources($vmconf, 1);
> +	    PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
>  	delete $missing_mappings_by_node->{$localnode};
>  
>  	my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
> @@ -5192,6 +5192,9 @@ __PACKAGE__->register_method({
>  	die "unable to use snapshot name 'pending' (reserved name)\n"
>  	    if lc($snapname) eq 'pending';
>  
> +	my $vmconf = PVE::QemuConfig->load_config($vmid);
> +	PVE::QemuServer::check_non_migratable_resources($vmconf, $param->{vmstate}, 0);

This can be fine as an early check. But there should be another check
after the 'snapshot' lock was set in the config (or right before while
still holding the config file lock). Otherwise, there is no guarantee
that the config is the same at the time of the check and during the
actual snapshot operation.

My suggestion is to introduce a new abstract method (named something
like __snapshot_assert_no_blockers()) in AbstractConfig that is called
during __snapshot_prepare() which QemuConfig can override. Like that we
can do the check before writing the modified config for the snapshot.

> +
>  	my $realcmd = sub {
>  	    PVE::Cluster::log_msg('info', $authuser, "snapshot VM $vmid: $snapname");
>  	    PVE::QemuConfig->snapshot_create($vmid, $snapname, $param->{vmstate},
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 31a7ee9..9f342bf 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2563,13 +2563,32 @@ sub config_list {
>      return $res;
>  }
>  
> +sub check_non_migratable_resources {
> +    my ($conf, $state, $noerr) = @_;
> +
> +    my @non_migr_res = ();

Style nit: I'd prefer writing out the name or use something like
"blockers" (inside the function where it's clear what is meant) if it
really is too long

> +    if ($state && $conf->{amd_sev}) {
> +	push @non_migr_res, "amd_sev";
> +    }
> +
> +    if (scalar @non_migr_res && !$noerr) {

Style nit: I'd prefer parentheses for scalar(@stuff)

> +	die "Cannot live-migrate, snapshot (with RAM), or hibernate a VM with:"
> +	    ." @non_migr_res\n";
> +    }
> +
> +    return @non_migr_res;
> +}
> +


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel