From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id CFCFA1FF0E5 for ; Wed, 29 Jul 2026 11:57:02 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 987EF20AF5; Wed, 29 Jul 2026 11:57:02 +0200 (CEST) Message-ID: <26a8a6f4-0870-4bfc-807e-b75275f6d743@proxmox.com> Date: Wed, 29 Jul 2026 11:56:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC qemu-server 2/4] remote migrate: collect preconditions as structured findings To: Erik Fastermann , pve-devel@lists.proxmox.com References: <20260721115827.163442-1-e.fastermann@proxmox.com> <20260721115827.163442-3-e.fastermann@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260721115827.163442-3-e.fastermann@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785318963109 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.182 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: MVAW5ALKSGJR34GXNKSSQ7HVE3JZS3JQ X-Message-ID-Hash: MVAW5ALKSGJR34GXNKSSQ7HVE3JZS3JQ X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Am 21.07.26 um 1:58 PM schrieb Erik Fastermann: > Extract the remote-migration precondition checks into a > validate_remote_migrate_preconditions helper that records each problem > as a {severity, code, message} finding and returns the derived > migration parameters. The endpoint fails on the first error finding, > preserving the previous abort behavior. > > It also pulls the checks that previously lived only in the qm CLI into > the endpoint. Direct API callers such as the web UI now run them too, > so fewer migrations start only to fail partway. This prepares for a > precondition endpoint that returns the full findings list instead of > dying on the first error. Nit: that part could be it's own patch > User-visible effect is minimal: successful migrations are unchanged. > Some precondition error messages are reworded and the "Establishing > API connection" info line is no longer printed. Do we gain anything from not printing it? Such changes can also be nicer in separate patches to ease reviewability and discussion. > > Signed-off-by: Erik Fastermann > --- > src/PVE/API2/Qemu.pm | 304 ++++++++++++++++++++++++++++++------------- > 1 file changed, 211 insertions(+), 93 deletions(-) > > diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm > index 68f1800c..9b487457 100644 > --- a/src/PVE/API2/Qemu.pm > +++ b/src/PVE/API2/Qemu.pm > @@ -1042,6 +1042,195 @@ sub assert_scsi_feature_compatibility { > } > } > > +my sub validate_remote_migrate_preconditions { > + my ($param, $findings) = @_; > + > + my $add_error = sub { Style nit: please collect the subroutine arguments as named arguments like usual. In particular, you can use a hash like %extra_info as the last one. > + push @$findings, > + { > + severity => 'error', > + code => $_[0], Nit: for the {storage}/import-metadata endpoint we use 'type' rather than 'code'. > + message => ("$_[1]" =~ s/\s+$//r), Please use chomp() rather than such a regex (or PVE::Tools::trim, but I don't think there'll be trailing whitespace, else we should rather fix it where the messsage originates). > + @_[2 .. $#_], > + }; > + }; > + > + my $add_warning = sub { Style nit: please collect the subroutine arguments as named arguments like usual. > + push @$findings, > + { > + severity => 'warning', > + code => $_[0], > + message => ("$_[1]" =~ s/\s+$//r), Same as above > + @_[2 .. $#_], > + }; > + }; > + > + my $source_vmid = extract_param($param, 'vmid'); > + my $target_endpoint = extract_param($param, 'target-endpoint'); > + my $remote = PVE::JSONSchema::parse_property_string('proxmox-remote', $target_endpoint); > + my $target_vmid = extract_param($param, 'target-vmid') // $source_vmid; > + > + my $target_storage = extract_param($param, 'target-storage'); > + my $storagemap = eval { PVE::JSONSchema::parse_idmap($target_storage, 'pve-storage-id') }; > + raise_param_exc({ 'target-storage' => "failed to parse storage map: $@" }) > + if $@; > + > + my $target_bridge = extract_param($param, 'target-bridge'); > + my $bridgemap = eval { PVE::JSONSchema::parse_idmap($target_bridge, 'pve-bridge-id') }; > + raise_param_exc({ 'target-bridge' => "failed to parse bridge map: $@" }) > + if $@; > + > + my $delete = extract_param($param, 'delete') // 0; > + > + PVE::Cluster::check_cfs_quorum(); > + > + # test if VM exists > + my $conf = eval { PVE::QemuConfig->load_config($source_vmid) }; > + if ($@) { Style nit: please use if (my $err = $@) { to immediately assign the error to a variable. Same for the other instances. > + $add_error->('load-vm-config', $@); > + return; > + } > + > + if (PVE::QemuConfig->has_lock($conf)) { > + $add_error->('vm-locked', "VM is locked ($conf->{lock})"); > + } > + > + if (PVE::HA::Config::service_is_configured("vm:$source_vmid")) { > + $add_error->('vm-ha-configured', 'cannot remote-migrate VM that is configured for HA'); > + } > + > + my $conn_args = { > + protocol => 'https', > + host => $remote->{host}, > + port => $remote->{port} // 8006, > + apitoken => $remote->{apitoken}, > + }; > + > + if ($remote->{fingerprint}) { > + $conn_args->{cached_fingerprints} = { uc($remote->{fingerprint}) => 1 }; > + } > + > + my $api_client = PVE::APIClient::LWP->new(%$conn_args); > + > + # check connection once at the start > + eval { $api_client->get("/version") }; > + if ($@) { > + $add_error->('remote-conn', $@); > + return; > + } > + > + my $resources = $api_client->get("/cluster/resources", { type => 'vm' }); > + if (grep { defined($_->{vmid}) && $_->{vmid} eq $target_vmid } @$resources) { > + $add_error->('target-vmid-exists', "remote: guest with ID '$target_vmid' already exists"); > + } > + > + my $storages = $api_client->get("/nodes/localhost/storage", { enabled => 1 }); > + > + my $check_remote_storage = sub { > + my ($storage) = @_; > + my $found = [grep { $_->{storage} eq $storage } @$storages]; > + > + if (!@$found) { > + $add_error->( > + 'storage-missing', > + "remote: storage '$storage' does not exist (or missing permission)", > + storage => $storage, > + ); > + return; > + } > + > + $found = $found->[0]; > + my $content_types = [PVE::Tools::split_list($found->{content})]; > + $add_error->( > + 'storage-no-images', > + "remote: storage '$storage' cannot store images", > + storage => $storage, > + ) if !grep { $_ eq 'images' } @$content_types; > + }; > + > + foreach my $target_sid (values %{ $storagemap->{entries} }) { Style nit: please use for instead of foreach, please use post deref ->%* > + $check_remote_storage->($target_sid); > + } > + ---snip 8<--- > @@ -5713,104 +5902,26 @@ __PACKAGE__->register_method({ > my $rpcenv = PVE::RPCEnvironment::get(); > my $authuser = $rpcenv->get_user(); > > - my $source_vmid = extract_param($param, 'vmid'); > - my $target_endpoint = extract_param($param, 'target-endpoint'); > - my $target_vmid = extract_param($param, 'target-vmid') // $source_vmid; > - > - my $delete = extract_param($param, 'delete') // 0; > - > - PVE::Cluster::check_cfs_quorum(); > - > - # test if VM exists > - my $conf = PVE::QemuConfig->load_config($source_vmid); > - > - PVE::QemuConfig->check_lock($conf); > - > - raise_param_exc({ vmid => "cannot remote-migrate VM that is configured for HA" }) > - if PVE::HA::Config::service_is_configured("vm:$source_vmid"); > - > - my $remote = PVE::JSONSchema::parse_property_string('proxmox-remote', $target_endpoint); > - > - # TODO: move this as helper somewhere appropriate? > - my $conn_args = { > - protocol => 'https', > - host => $remote->{host}, > - port => $remote->{port} // 8006, > - apitoken => $remote->{apitoken}, > - }; > - > - if ($remote->{fingerprint}) { > - $conn_args->{cached_fingerprints} = { uc($remote->{fingerprint}) => 1 }; > - } > - > - print "Establishing API connection with remote at '$remote->{host}'\n"; > - > - my $api_client = PVE::APIClient::LWP->new(%$conn_args); > - > - my $repl_conf = PVE::ReplicationConfig->new(); > - my $is_replicated = $repl_conf->check_for_existing_jobs($source_vmid, 1); > - die "cannot remote-migrate replicated VM\n" if $is_replicated; > + my $findings = []; > + my $plan = validate_remote_migrate_preconditions($param, $findings); Style nit: why pass in $findings like this? You can just return multiple values instead. I'd prefer a name like $migration_info rather than $plan. > > - if (PVE::QemuServer::check_running($source_vmid)) { > - die "can't migrate running VM without --online\n" if !$param->{online}; > - > - } else { > - warn "VM isn't running. Doing offline migration instead.\n" if $param->{online}; > - $param->{online} = 0; > + foreach my $finding (@$findings) { Style nit: please use for instead of foreach > + warn "$finding->{message}\n" if $finding->{severity} eq 'warning'; > + die "$finding->{message}\n" if $finding->{severity} eq 'error'; We can print all warnings and errors and then die with a message saying how many warnings and errors there are. > } > > - if (defined($conf->{cpu})) { > - my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu}); > - my $cputype = $cpu->{cputype}; > - if (defined($cputype) && PVE::QemuServer::CPUConfig::is_custom_model($cputype)) { > - my $custom_cpu = PVE::QemuServer::CPUConfig::get_custom_model($cputype); > - > - my $remote_custom_cpu = eval { > - $api_client->get("/cluster/qemu/custom-cpu-models/" > - . URI::Escape::uri_escape_utf8($cputype)); > - }; > - die "could not validate custom CPU model compatibility: $@\n" if $@; > - > - my $cpu_schema = { > - type => 'object', > - properties => PVE::QemuServer::CPUConfig->options(), > - }; > - eval { PVE::JSONSchema::validate($remote_custom_cpu, $cpu_schema); }; > - die "could not validate custom CPU model compatibility: $@\n" if $@; > - > - PVE::QemuServer::CPUConfig::assert_custom_model_compatibility( > - $custom_cpu, $remote_custom_cpu, > - ); > - } > - } > - > - my $storecfg = PVE::Storage::config(); > - my $target_storage = extract_param($param, 'target-storage'); > - my $storagemap = > - eval { PVE::JSONSchema::parse_idmap($target_storage, 'pve-storage-id') }; > - raise_param_exc({ 'target-storage' => "failed to parse storage map: $@" }) > - if $@; > - > - my $target_bridge = extract_param($param, 'target-bridge'); > - my $bridgemap = eval { PVE::JSONSchema::parse_idmap($target_bridge, 'pve-bridge-id') }; > - raise_param_exc({ 'target-bridge' => "failed to parse bridge map: $@" }) > - if $@; > - > - die "remote migration requires explicit storage mapping!\n" > - if $storagemap->{identity}; > - > - $param->{storagemap} = $storagemap; > - $param->{bridgemap} = $bridgemap; > + $param->{storagemap} = $plan->{storagemap}; > + $param->{bridgemap} = $plan->{bridgemap}; > $param->{remote} = { > - conn => $conn_args, # re-use fingerprint for tunnel > - client => $api_client, > - vmid => $target_vmid, > + conn => $plan->{conn_args}, # re-use fingerprint for tunnel > + client => $plan->{api_client}, > + vmid => $plan->{target_vmid}, > }; > $param->{migration_type} = 'websocket'; > $param->{'with-local-disks'} = 1; > - $param->{delete} = $delete if $delete; > + $param->{delete} = $plan->{delete} if $plan->{delete}; > > - my $cluster_status = $api_client->get("/cluster/status"); > + my $cluster_status = $plan->{api_client}->get("/cluster/status"); > my $target_node; > foreach my $entry (@$cluster_status) { > next if $entry->{type} ne 'node'; > @@ -5824,14 +5935,21 @@ __PACKAGE__->register_method({ > if !defined($target_node); > > my $realcmd = sub { > - PVE::QemuMigrate->migrate($target_node, $remote->{host}, $source_vmid, $param); > + PVE::QemuMigrate->migrate( > + $target_node, > + $plan->{conn_args}->{host}, > + $plan->{source_vmid}, > + $param, > + ); > }; > > my $worker = sub { > - return PVE::GuestHelpers::guest_migration_lock($source_vmid, 10, $realcmd); > + return PVE::GuestHelpers::guest_migration_lock($plan->{source_vmid}, 10, $realcmd); > }; > > - return $rpcenv->fork_worker('qmigrate', $source_vmid, $authuser, $worker); > + return $rpcenv->fork_worker( > + 'qmigrate', $plan->{source_vmid}, $authuser, $worker, > + ); > }, > }); >