From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id BB5401FF0AE for ; Tue, 13 Oct 2026 16:34:30 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7E090213BC; Tue, 13 Oct 2026 16:34:27 +0200 (CEST) Message-ID: Date: Tue, 13 Oct 2026 16:34:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH ha-manager v3 10/21] api: resources: migration: support additional migration options To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260918160841.128088-1-f.ebner@proxmox.com> <20260918160841.128088-11-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791902061175 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.429 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: 47IIMPDYAVW52TTADFHHMGF5HXKMAU7W X-Message-ID-Hash: 47IIMPDYAVW52TTADFHHMGF5HXKMAU7W 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 12.10.26 um 11:59 AM schrieb Daniel Kral: > On Fri Sep 18, 2026 at 6:08 PM CEST, Fiona Ebner wrote: >> @@ -34,6 +37,34 @@ my $api_copy_config = sub { >> return $scfg; >> }; >> >> +my $common_migrate_json_properties = { >> + sid => get_standard_option( >> + 'pve-ha-resource-or-vm-id', >> + { completion => \&PVE::HA::Tools::complete_sid }, >> + ), >> + node => get_standard_option( >> + 'pve-node', >> + { >> + completion => \&PVE::Cluster::complete_migration_target, >> + description => "Target node.", >> + }, >> + ), >> +}; >> + >> +my sub json_crm_command_is_supported { > > nit: This assumes that cfs_update() is called before, which is called in > PVE::RPCEnvironment::init_request() for API requests and > PVE::RESTEnvironment::setup_default_cli_env() for CLI requests, > that is used in PVE::CLI::ha_manager::setup_environment(). > > Maybe we should add a comment about that, because otherwise the > 'version-info' KV will only contain the current node's JSON string, > therefore checking only locally? Could be pedantic though. I'll add a call to cfs_update() so we have the latest state. > >> + my $version_info = PVE::Cluster::get_node_kv('version-info'); >> + for my $node (sort keys $version_info->%*) { >> + my $node_info = eval { JSON::decode_json($version_info->{$node}); }; >> + if (my $err = $@) { # warn, but continue >> + syslog('warn', "cannot parse version info for $node as JSON - $err"); > > Shouldn't we fail if it's not possible to determine a node's version? No, this is deliberate. Updated cluster is the usual situation. An error here should not affect the usual situation. I'm choosing to affect the unusual situation, where the master happens to be too old and unable to report its version properly, which should be rare. The failure is also not critical: it will be an ignored CRM command with warnings/errors logged until the version can be determined again. But I will switch the order below and keep submitting a non-JSON CRM command if the new style is not actually required. This gives an escape hatch. > > Any way, the syslog should probably have some context why parsing the > version info is needed here and should also be shown to stdout/stderr > for callers of ha-manager. Ack. > >> + } elsif (!PVE::Cluster::pvecfg_min_version($node_info->{version}, 9, 2, 21)) { >> + return 0; >> + } >> + } >> + >> + return 1; >> +} > > IMO a generic version of this helper would be nice to have in > PVE::Cluster as well, as there already are users that need that > functionality (David's vNet pool series [0] and Lukas' notification > matcher series [1]), but no blocker for this. > > [0] https://lore.proxmox.com/pve-devel/20260724142528.109453-4-d.riley@proxmox.com/ > [1] https://lore.proxmox.com/pve-devel/20261009125646.284673-39-l.wagner@proxmox.com/ > I'm not sure, because not every caller might have the same kind of failure consideration I described above. Other callers might want to be stricter if the failure scenario (i.e. in case of an old node that can't be detected) is worse or more common. There also is the consideration with offline nodes, which is another angle to the check. So I'm somewhat inclined to keep it local (for now). ---snip 8<--- >> @@ -403,12 +448,38 @@ __PACKAGE__->register_method({ >> >> my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid')); >> my $req_node = extract_param($param, 'node'); >> + my $resource_type = extract_param($param, 'resource-type'); >> + if ($resource_type ne $type) { >> + raise_param_exc({ >> + 'resource-type' => "$resource_type does not match type of $sid" }); >> + } >> + >> + # The rest of the parameters in $param are migration options passed along to the plugin. >> >> PVE::HA::Config::service_is_ha_managed($sid); >> >> check_service_state($sid); >> >> - PVE::HA::Config::queue_crm_commands("migrate $sid $req_node"); >> + my $crm_command; >> + if (json_crm_command_is_supported()) { >> + $crm_command = JSON::encode_json({ >> + kind => 'migrate', >> + sid => $sid, >> + node => $req_node, >> + options => $param, >> + }); >> + } else { >> + if (scalar(keys $param->%*) > 0) { >> + syslog( >> + 'warn', >> + "$sid migration to $req_node: ignoring additional migration options" >> + . " - not supported by CRM on all nodes", >> + ); > > nit: This should also be shown in stdout/stderr to direct callers of > ha-manager. Ack! > >> + } >> + $crm_command = "migrate $sid $req_node"; >> + } >> + >> + PVE::HA::Config::queue_crm_commands($crm_command); > > Nice! > >> $result->{sid} = $sid; >> $result->{'requested-node'} = $req_node; >> >> diff --git a/src/PVE/HA/Resources/PVECT.pm b/src/PVE/HA/Resources/PVECT.pm >> index 3d8a5b9..17dd697 100644 >> --- a/src/PVE/HA/Resources/PVECT.pm >> +++ b/src/PVE/HA/Resources/PVECT.pm >> @@ -174,4 +174,14 @@ sub get_static_stats_from_config { >> }; >> } >> >> +# FIXME: the extra properties are missing from the docs, since the PVE::API::LXC module can't be >> +# included in that environment, because of cyclic dependencies.. >> +sub migrate_json_properties { >> + my ($props) = @_; >> + my $extra_migrate_props = eval { PVE::API2::LXC::ha_migrate_json_properties() } // {}; >> + die $@ if $@ && !$ENV{PVE_GENERATING_DOCS}; >> + $props->{$_} = $extra_migrate_props->{$_} for keys $extra_migrate_props->%*; > > This will write the specific migrate props in the external $props, i.e., > $common_migrate_json_properties. For some reason, the one for VMs is run > first for me, which results in both VMs and CTs having > --with-conntrack-state in the end: > > > # ha-manager help migrate > ... > > > Therefore, this should only write in the new copy made by > ha_migrate_json_properties(). > Good catch!