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 5C95C1FF0AE for ; Tue, 13 Oct 2026 16:10:19 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3013121350; Tue, 13 Oct 2026 16:10:17 +0200 (CEST) Message-ID: Date: Tue, 13 Oct 2026 16:10:11 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH ha-manager v3 09/21] crm command: support JSON-style migrate command To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260918160841.128088-1-f.ebner@proxmox.com> <20260918160841.128088-10-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: 1791900611901 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.431 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: OBFKBQCCY6MQ3HNIJF7TBATP54TW3TXQ X-Message-ID-Hash: OBFKBQCCY6MQ3HNIJF7TBATP54TW3TXQ 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 09.10.26 um 3:38 PM schrieb Daniel Kral: > On Fri Sep 18, 2026 at 6:08 PM CEST, Fiona Ebner wrote: >> To pass along opaque migration options from the resource-specific >> migration API endpoint to the resource plugin implementation, the >> options will be encoded as JSON. Instead of just adding an extra >> positional argument with those, support having the full CRM command be >> recorded as JSON in the queued commands file. >> >> Signed-off-by: Fiona Ebner >> --- >> >> New in v3. >> >> src/PVE/HA/Manager.pm | 73 +++++++++++++++++++++++++++++++++---------- >> 1 file changed, 56 insertions(+), 17 deletions(-) >> >> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm >> index 555ea0d..25026d6 100644 >> --- a/src/PVE/HA/Manager.pm >> +++ b/src/PVE/HA/Manager.pm >> @@ -4,6 +4,7 @@ use strict; >> use warnings; >> >> use Digest::MD5 qw(md5_base64); >> +use JSON qw(); >> >> use PVE::Tools; >> >> @@ -670,6 +671,36 @@ sub any_resource_motion_queued_or_running { >> return 0; >> } >> >> +my sub crm_cmd_migrate_relocate { > > Hm, queue_resource_motion() 'queues' the resource motion command to > $sd->{cmd}, which is handled in next_state_{started,stopped}(). > > Maybe we should call this handle_crm_motion_command() or some > permutation of that? Or assert_and_queue_resource_motion()? > > The term 'command' is already quite overloaded in the HA stack, but at > least in the Manager we can discern between crm_*_command (crm_commands > stack) and resource_*_command ($sd->{cmd}), what do you think? I'll go with handle_crm_motion_command(). > >> + my ($self, $task, $sid, $node, $options) = @_; >> + >> + my $cmd = "$task $sid $node"; >> + >> + if (defined($options) && scalar(keys($options->%*))) { >> + $cmd .= ' ' . JSON::encode_json($options); >> + } >> + >> + my ($haenv, $ms, $ns, $sc, $ss) = $self->@{qw(haenv ms ns sc ss)}; >> + >> + if (my $sd = $ss->{$sid}) { >> + if (!$ns->node_is_online($node)) { >> + $haenv->log('err', "crm command error - node not online: $cmd"); >> + } else { >> + if ($node eq $sd->{node}) { >> + $haenv->log( >> + 'info', "ignore crm command - service already on target node: $cmd", >> + ); >> + } else { >> + $self->queue_resource_motion($cmd, $task, $sid, $node, $options); >> + } >> + } >> + } else { >> + $haenv->log('err', "crm command error - no such service: $cmd"); >> + } > > If we factor out the logic to a assert_valid_resource_motion() helper as > suggested in patch #4, then we could use it here, but not a blocker for > this at all. I'm doing this for v4. > >> + >> + return; >> +} >> + >> # read new crm commands and save them into crm master status >> sub update_crm_commands { >> my ($self) = @_; >> @@ -681,25 +712,33 @@ sub update_crm_commands { >> foreach my $cmd (split(/\n/, $cmdlist)) { >> chomp $cmd; >> >> - if ($cmd =~ m/^(migrate|relocate)\s+(\S+)\s+(\S+)$/) { >> - my ($task, $sid, $node) = ($1, $2, $3); >> - if (my $sd = $ss->{$sid}) { >> - if (!$ns->node_is_online($node)) { >> - $haenv->log('err', "crm command error - node not online: $cmd"); >> - } else { >> - if ($node eq $sd->{node}) { >> - $haenv->log( >> - 'info', >> - "ignore crm command - service already on target node: $cmd", >> - ); >> - } else { >> - $self->queue_resource_motion($cmd, $task, $sid, $node, {}); >> - } >> - } >> - } else { >> - $haenv->log('err', "crm command error - no such service: $cmd"); >> + if ($cmd =~ m/^{/) { >> + # New-style JSON-encoded command. Currently only 'migrate' is supported, allowing for >> + # additional options. >> + >> + my $command_info = eval { JSON::decode_json($cmd) }; >> + if (my $err = $@) { >> + $haenv->log('err', "unable to decode command as JSON: '$cmd' - $err"); > > nit: unable to decode JSON command: '$cmd' - $err > >> + next; >> } >> >> + my $kind = $command_info->{kind}; >> + if (defined($kind) && $kind eq 'migrate') { > > nit: maybe move the defined($kind) check above this already, because > otherwise any other branch that checks for $kind eq 'something' would > need it as well. > Will do! >> + my ($sid, $node, $options) = $command_info->@{qw(sid node options)}; >> + if (!defined($sid) || !defined($node)) { >> + $haenv->log('err', "'migrate' JSON command without sid or node"); >> + next; >> + } >> + crm_cmd_migrate_relocate($self, $kind, $sid, $node, $options); >> + } else { >> + $haenv->log('err', "unable to handle unknown JSON command: '$cmd'"); >> + } >> + >> + next; >> + } >> + >> + if ($cmd =~ m/^(migrate|relocate)\s+(\S+)\s+(\S+)$/) { >> + crm_cmd_migrate_relocate($self, $1, $2, $3, {}); > > Nice! > >> } elsif ($cmd =~ m/^stop\s+(\S+)\s+(\S+)$/) { >> my ($sid, $timeout) = ($1, $2); >> if (my $sd = $ss->{$sid}) { > >