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

["Daniel Kral" <d.kral@proxmox.com>]

On Tue Oct 13, 2026 at 4:34 PM CEST, Fiona Ebner wrote:
> Am 12.10.26 um 11:59 AM schrieb Daniel Kral:
>> On Fri Sep 18, 2026 at 6:08 PM CEST, Fiona Ebner wrote:
>>> +    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.

Yeah, since 'version-info' is broadcasted in the same format since
Proxmox VE 7.1 so that shouldn't be a problem.

>
> 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.

This depends if we want to switch to only the JSON CRM command format in
the future, then it would be preferable to always use the new format so
we can switch to it in some future major version, at least for the
migrate command?

>
>> 
>> 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).

Right, good point, haven't thought about this! Then let's keep it as it
is for now.