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 DCE1F1FF0B0 for ; Fri, 09 Oct 2026 15:38:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C1A1021711; Fri, 09 Oct 2026 15:38:16 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 09 Oct 2026 15:38:12 +0200 Message-Id: From: "Daniel Kral" To: "Fiona Ebner" , Subject: Re: [PATCH ha-manager v3 09/21] crm command: support JSON-style migrate command X-Mailer: aerc 0.22.0-10-g6373ac9d2179-dirty References: <20260918160841.128088-1-f.ebner@proxmox.com> <20260918160841.128088-10-f.ebner@proxmox.com> In-Reply-To: <20260918160841.128088-10-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791553092103 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.850 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: UICMSDGJWJVBB7VHG7H3OLLFAKQEDQD7 X-Message-ID-Hash: UICMSDGJWJVBB7VHG7H3OLLFAKQEDQD7 X-MailFrom: d.kral@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: 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; > =20 > use Digest::MD5 qw(md5_base64); > +use JSON qw(); > =20 > use PVE::Tools; > =20 > @@ -670,6 +671,36 @@ sub any_resource_motion_queued_or_running { > return 0; > } > =20 > +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? > + my ($self, $task, $sid, $node, $options) =3D @_; > + > + my $cmd =3D "$task $sid $node"; > + > + if (defined($options) && scalar(keys($options->%*))) { > + $cmd .=3D ' ' . JSON::encode_json($options); > + } > + > + my ($haenv, $ms, $ns, $sc, $ss) =3D $self->@{qw(haenv ms ns sc ss)}; > + > + if (my $sd =3D $ss->{$sid}) { > + if (!$ns->node_is_online($node)) { > + $haenv->log('err', "crm command error - node not online: $cm= d"); > + } else { > + if ($node eq $sd->{node}) { > + $haenv->log( > + 'info', "ignore crm command - service already on tar= get 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. > + > + return; > +} > + > # read new crm commands and save them into crm master status > sub update_crm_commands { > my ($self) =3D @_; > @@ -681,25 +712,33 @@ sub update_crm_commands { > foreach my $cmd (split(/\n/, $cmdlist)) { > chomp $cmd; > =20 > - if ($cmd =3D~ m/^(migrate|relocate)\s+(\S+)\s+(\S+)$/) { > - my ($task, $sid, $node) =3D ($1, $2, $3); > - if (my $sd =3D $ss->{$sid}) { > - if (!$ns->node_is_online($node)) { > - $haenv->log('err', "crm command error - node not onl= ine: $cmd"); > - } else { > - if ($node eq $sd->{node}) { > - $haenv->log( > - 'info', > - "ignore crm command - service already on tar= get node: $cmd", > - ); > - } else { > - $self->queue_resource_motion($cmd, $task, $sid, = $node, {}); > - } > - } > - } else { > - $haenv->log('err', "crm command error - no such service:= $cmd"); > + if ($cmd =3D~ m/^{/) { > + # New-style JSON-encoded command. Currently only 'migrate' i= s supported, allowing for > + # additional options. > + > + my $command_info =3D eval { JSON::decode_json($cmd) }; > + if (my $err =3D $@) { > + $haenv->log('err', "unable to decode command as JSON: '$= cmd' - $err"); nit: unable to decode JSON command: '$cmd' - $err > + next; > } > =20 > + my $kind =3D $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. > + my ($sid, $node, $options) =3D $command_info->@{qw(sid n= ode options)}; > + if (!defined($sid) || !defined($node)) { > + $haenv->log('err', "'migrate' JSON command without s= id or node"); > + next; > + } > + crm_cmd_migrate_relocate($self, $kind, $sid, $node, $opt= ions); > + } else { > + $haenv->log('err', "unable to handle unknown JSON comman= d: '$cmd'"); > + } > + > + next; > + } > + > + if ($cmd =3D~ m/^(migrate|relocate)\s+(\S+)\s+(\S+)$/) { > + crm_cmd_migrate_relocate($self, $1, $2, $3, {}); Nice! > } elsif ($cmd =3D~ m/^stop\s+(\S+)\s+(\S+)$/) { > my ($sid, $timeout) =3D ($1, $2); > if (my $sd =3D $ss->{$sid}) {