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 1C2901FF09C for ; Mon, 19 Oct 2026 14:06:46 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id BABE221603; Mon, 19 Oct 2026 14:06:39 +0200 (CEST) Message-ID: <6080986c-899d-406c-9de9-e41f0a22affb@proxmox.com> Date: Mon, 19 Oct 2026 14:06:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH ha-manager v4 13/27] crm command: support JSON-style migrate command To: Daniel Kral , pve-devel@lists.proxmox.com References: <20261015125112.174954-1-f.ebner@proxmox.com> <20261015125112.174954-14-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: 1792411598087 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.385 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: ZTUYLUACS5XROUTQOBYBFLXYD3TGMLBW X-Message-ID-Hash: ZTUYLUACS5XROUTQOBYBFLXYD3TGMLBW 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 16.10.26 um 11:53 AM schrieb Daniel Kral: > On Thu Oct 15, 2026 at 2:50 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 >> --- >> >> Changes in v4: >> * Encode JSON with canonical => 1. >> * Improve error messages. >> >> src/PVE/HA/Manager.pm | 57 +++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 49 insertions(+), 8 deletions(-) >> >> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm >> index c28d7fa..4e7fab5 100644 >> --- a/src/PVE/HA/Manager.pm >> +++ b/src/PVE/HA/Manager.pm >> @@ -711,6 +711,28 @@ my sub check_resource_motion_valid { >> return 1; >> } >> >> +my sub handle_crm_motion_command { >> + my ($self, $task, $sid, $node, $options) = @_; >> + >> + my $cmd = "$task $sid $node"; >> + >> + if (defined($options) && scalar(keys($options->%*))) { >> + $cmd .= ' ' . JSON::to_json($options, { canonical => 1 }); >> + } >> + >> + my ($haenv, $ms, $ns, $sc, $ss) = $self->@{qw(haenv ms ns sc ss)}; > > nit: $ms, $ns, and $sc are unused here I'll drop them. > >> + >> + if (my $sd = $ss->{$sid}) { >> + if (check_resource_motion_valid($self, $node, $sd, "ignore crm command '$cmd'")) { >> + $self->queue_resource_motion($cmd, $task, $sid, $node, $options); >> + } >> + } else { >> + $haenv->log('err', "crm command error - no such service: $cmd"); >> + } >> + >> + return; >> +} >> + >> # read new crm commands and save them into crm master status >> sub update_crm_commands { >> my ($self) = @_; >> @@ -722,16 +744,35 @@ 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 (check_resource_motion_valid($self, $node, $sd, "ignore crm command '$cmd'")) { >> - $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 JSON command: '$cmd' - $err"); >> + next; >> } >> >> + my $kind = $command_info->{kind}; >> + if (!defined($kind)) { >> + $haenv->log('err', "unable to handle JSON command without 'kind': '$cmd'"); >> + } elsif ($kind eq 'migrate') { >> + 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; >> + } >> + handle_crm_motion_command($self, $kind, $sid, $node, $options); > > nit: only noticed now, but 'kind' should probably also be 'task' here as > we call it like that everywhere else... Even though with variants > 'kind' is more common, so no hard feelings here. Sure, will change it for consistency. > >> + } else { >> + $haenv->log('err', "unable to handle unknown JSON command: '$cmd'"); >> + } >> + >> + next; >> + } >> + >> + if ($cmd =~ m/^(migrate|relocate)\s+(\S+)\s+(\S+)$/) { > > nit: should probably be an elsif here? I deliberately chose the if () { ... next; } pattern, because like that, the JSON-style command handling and the usual command handling is better separated visually. But I'm going to factor out the JSON command handling into a helper function and go with elsif instead. > >> + handle_crm_motion_command($self, $1, $2, $3, {}); >> } elsif ($cmd =~ m/^stop\s+(\S+)\s+(\S+)$/) { >> my ($sid, $timeout) = ($1, $2); >> if (my $sd = $ss->{$sid}) { > >