Re: [PATCH ha-manager v3 10/21] api: resources: migration: support additional migration options

[Fiona Ebner <f.ebner@proxmox.com>]

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!