On 5/2/25 11:33, Fiona Ebner wrote:
Am 30.04.25 um 13:09 schrieb Daniel Kral:On 3/25/25 16:12, Daniel Kral wrote:sub select_service_node { - my ($groups, $online_node_usage, $sid, $service_conf, $current_node, $try_next, $tried_nodes, $maintenance_fallback, $best_scored) = @_; + # TODO Cleanup this signature post-RFC + my ($rules, $groups, $online_node_usage, $sid, $service_conf, $current_node, $try_next, $tried_nodes, $maintenance_fallback, $best_scored) = @_;I'm currently trying to clean up the helper's signature here, but doing something like sub select_service_node { my ($service_info, $affinity_info, $try_next, $best_scored) = @_; my ($sid, $service_conf, $current_node) = $service_info->@{qw(sid config current_node)}; my ($rules, $groups, $online_node_usage, $tried_nodes, $maintenance_fallback) = $affinity_info->@{qw(rules groups online_node_usage failed_nodes maintenance_node)}; would require us to create helper structures on all four call sites (one of them is just the test case ./test_failover1.pl), or introduce another helper to just create them for passing it here and immediately de- structuring it in select_service_node(...): sub get_service_affinity_info { my ($self, $sid, $cd, $sd) = @_; my $service_info = { sid => $sid, config => $cd, current_node => $sd->{node}, }; my $affinity_info = { rules => $self->{rules}, groups => $self->{groups}, failed_nodes => $sd->{failed_nodes}, maintenance_node => $sd->{maintenance_node}, online_node_usage => $self->{online_node_usage}, }; return ($service_info, $affinity_info); }; Also the call site in next_state_recovery(...) does not pass $sd-{failed_nodes}, $sd->{maintenance_node} and $best_scored to it. AFAICS$sd->{failed_nodes} should be undef in next_state_recovery(...) anyway, but I feel like I have missed some states it could be in there. And $sd-{maintenance_node} could be set anytime.I think it makes sense to have it explicitly (rather than just implicitly) opt-out of $try_next just like the caller for rebalance_on_request_start. Without $try_next, the $tried_nodes parameter does not have any effect (the caller for rebalance_on_request_start passes it, but select_service_node() won't read it if $try_next isn't set). The caller in next_state_recovery() should also pass $best_scored IMHO, so that it is fully aligned with the caller for rebalance_on_request_start. It won't be an actual change result-wise, because for recovery, the current node is not available, so it already cannot be the result, but it makes sense semantically. We want the best scored node for recovery. And having the two callers look the same is a simplification too.
Yes, I put a patch in-between so that all the arguments are made explicit before there's any refactoring. Then the commit message can hold some rationale why for reviewers and future reference.
In the same spirit I'd also like to make it more explicit that for next_state_recovery() it can have $current_node and $maintenance_node set, but they're irrelevant as those will be invalid nodes to select anyway, i.e. $group_members->{$node} and $pri_nodes->{$node} are both false as both contain only available nodes in the first place, if I'm not mistaken.
If there's nothing speaking against that, I'd prefer to elevate select_service_node(...) to be a method as it needs quite a lot of state anyway, especially as we will need global information about other services than just the current one in the future anyway.I don't have strong feelings about this, both approaches seem fine to me.So, I'd do something like sub select_service_node { my ($self, $sid, $service_conf, $sd, $mode) = @_; my ($rules, $groups, $online_node_usage) = $self->@{qw(rules groups online_node_usage)};If we don't want to make it a method, we could still pass these ones separately. After implementing location rules, $groups would be dropped anyways.
Right, then I'll go for keeping it as a helper as this wouldn't be a method in its own right anyway.
my ($current_node, $tried_nodes, $maintenance_fallback) = $self-@{qw(node failed_nodes maintenance_node)};It's $sd->... here.
ACK.
here. It's not fancy as in there's a well-defined interface one can immediately see what this helper needs (as it has access to the whole $self) and doesn't have the guarantees of a standalone helper (won't touch $self), but I think it could be better than creating helper structures which are only pass a message, which is immediately destructured anyway. We could also just pass $self slightly differently, but I don't see much difference there. The $mode could then be a enumeration of e.g. whether $try_next (e.g. 'try_again') or $best_scored (e.g. 'rebalance') is used (and can beHaving a mode sounds good to me. I don't think it should be called 'rebalance', the best-scored semantics should apply to recovery too, see above.
Right, it wouldn't sound right there then. I think I'll just simply go with 'try-next' and 'best-score' for both ;).
The only reference to the previous $best_scored would be dropped in select_service_node() as both states are now mutually exclusive, which also shows that setting $mode = 'best-score' for next_state_recovery() shouldn't change the result.
_______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
