From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id C5B711FF16F
for <inbox@lore.proxmox.com>; Tue, 29 Apr 2025 11:50:35 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id C752084D7;
Tue, 29 Apr 2025 11:50:44 +0200 (CEST)
Message-ID: <01b09558-ec87-4327-a112-3e01999e779a@proxmox.com>
Date: Tue, 29 Apr 2025 11:50:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250325151254.193177-1-d.kral@proxmox.com>
<20250325151254.193177-11-d.kral@proxmox.com>
<67d201c9-64ca-4392-b166-aa14cb2bae48@proxmox.com>
Content-Language: en-US
From: Daniel Kral <d.kral@proxmox.com>
In-Reply-To: <67d201c9-64ca-4392-b166-aa14cb2bae48@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.011 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [manager.pm, colocation.pm]
Subject: Re: [pve-devel] [PATCH ha-manager 09/15] manager: apply colocation
rules when selecting service nodes
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 4/28/25 14:26, Fiona Ebner wrote:
> Am 25.03.25 um 16:12 schrieb Daniel Kral:
>> Add a mechanism to the node selection subroutine, which enforces the
>> colocation rules defined in the rules config.
>>
>> The algorithm manipulates the set of nodes directly, which the service
>> is allowed to run on, depending on the type and strictness of the
>> colocation rules, if there are any.
>>
>> This makes it depend on the prior removal of any nodes, which are
>> unavailable (i.e. offline, unreachable, or weren't able to start the
>> service in previous tries) or are not allowed to be run on otherwise
>> (i.e. HA group node restrictions) to function correctly.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> src/PVE/HA/Manager.pm | 203 ++++++++++++++++++++++++++++++++++++-
>> src/test/test_failover1.pl | 4 +-
>> 2 files changed, 205 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
>> index 8f2ab3d..79b6555 100644
>> --- a/src/PVE/HA/Manager.pm
>> +++ b/src/PVE/HA/Manager.pm
>> @@ -157,8 +157,201 @@ sub get_node_priority_groups {
>> return ($pri_groups, $group_members);
>> }
>>
>
> I feel like these helper functions should rather go into the colocation
> plugin or some other module to not bloat up Manager.pm more.
I agree, I'll move them either to Colocation.pm or some
>
>> +=head3 get_colocated_services($rules, $sid, $online_node_usage)
>> +
>> +Returns a hash map of all services, which are specified as being in a positive
>> +or negative colocation in C<$rules> with the given service with id C<$sid>.
>> +
>> +Each service entry consists of the type of colocation, strictness of colocation
>> +and the node the service is currently assigned to, if any, according to
>> +C<$online_node_usage>.
>> +
>> +For example, a service C<'vm:101'> being strictly colocated together (positive)
>> +with two other services C<'vm:102'> and C<'vm:103'> and loosely colocated
>> +separate with another service C<'vm:104'> results in the hash map:
>> +
>> + {
>> + 'vm:102' => {
>> + affinity => 'together',
>> + strict => 1,
>> + node => 'node2'
>> + },
>> + 'vm:103' => {
>> + affinity => 'together',
>> + strict => 1,
>> + node => 'node2'
>> + },
>> + 'vm:104' => {
>> + affinity => 'separate',
>> + strict => 0,
>> + node => undef
>
> Why is the node undef here?
It's set undefined as the vm:104 is not placed on a node yet. This could
happen if two nodes fail at exactly the same time and e.g. this helper
is called for selecting a service node for 'vm:101'.
I'll change the example to a more intuitive case, e.g. when a single
node failed and one of the positively colocated services has already
been assigned a node, but the current and another one hasn't (i.e. the
node is undef) and document here why.
But I'll go with the comment below and integrate this in the helper
below as it is the only user of this subroutine.
>
>> + }
>> + }
>> +
>> +=cut
>> +
>> +sub get_colocated_services {
>> + my ($rules, $sid, $online_node_usage) = @_;
>> +
>> + my $services = {};
>> +
>> + PVE::HA::Rules::Colocation::foreach_colocation_rule($rules, sub {
>> + my ($rule) = @_;
>> +
>> + for my $csid (sort keys %{$rule->{services}}) {
>> + next if $csid eq $sid;
>> +
>> + $services->{$csid} = {
>> + node => $online_node_usage->get_service_node($csid),
>> + affinity => $rule->{affinity},
>> + strict => $rule->{strict},
>> + };
>> + }
>> + }, {
>> + sid => $sid,
>> + });
>> +
>> + return $services;
>> +}
>> +
>> +=head3 get_colocation_preference($rules, $sid, $online_node_usage)
>> +
>> +Returns a list of two hashes, where each is a hash map of the colocation
>> +preference of C<$sid>, according to the colocation rules in C<$rules> and the
>> +service locations in C<$online_node_usage>.
>> +
>> +The first hash is the positive colocation preference, where each element
>> +represents properties for how much C<$sid> prefers to be on the node.
>> +Currently, this is a binary C<$strict> field, which means either it should be
>
> s/it/the service/
ACK
>
>> +there (C<0>) or must be there (C<1>).
>> +
>> +The second hash is the negative colocation preference, where each element
>> +represents properties for how much C<$sid> prefers not to be on the node.
>> +Currently, this is a binary C<$strict> field, which means either it should not
>
> s/it/the service/
ACK
>
>> +be there (C<0>) or must not be there (C<1>).
>> +
>> +=cut
>> +
>> +sub get_colocation_preference {
>> + my ($rules, $sid, $online_node_usage) = @_;
>> +
>> + my $services = get_colocated_services($rules, $sid, $online_node_usage);
>
> The name $services is a bit too generic, maybe $colocation_per_service
> or something?
>
> Maybe it would be better to just merge this one and the helper above
> into a single one? I.e. just handle the info while iterating the rules
> directly instead of creating a novel temporary per-service
> data-structure and iterate twice.
ACK, I'll do that.
I thought get_colocated_services(...) would get more users in the end,
but it seems like it wouldn't and if it does I can easily re-introduce it.
>
>> +
>> + my $together = {};
>> + my $separate = {};
>> +
>> + for my $service (values %$services) {
>> + my $node = $service->{node};
>> +
>> + next if !$node;
>> +
>> + my $node_set = $service->{affinity} eq 'together' ? $together : $separate;
>> + $node_set->{$node}->{strict} = $node_set->{$node}->{strict} || $service->{strict};
>> + }
>> +
>> + return ($together, $separate);
>> +}
>> +
>> +=head3 apply_positive_colocation_rules($together, $allowed_nodes)
>> +
>> +Applies the positive colocation preference C<$together> on the allowed node
>> +hash set C<$allowed_nodes> directly.
>> +
>> +Positive colocation means keeping services together on a single node, and
>> +therefore minimizing the separation of services.
>> +
>> +The allowed node hash set C<$allowed_nodes> is expected to contain any node,
>> +which is available to the service, i.e. each node is currently online, is
>> +available according to other location constraints, and the service has not
>> +failed running there yet.
>> +
>> +=cut
>> +
>> +sub apply_positive_colocation_rules {
>> + my ($together, $allowed_nodes) = @_;
>> +
>> + return if scalar(keys %$together) < 1;
>> +
>> + my $mandatory_nodes = {};
>> + my $possible_nodes = PVE::HA::Tools::intersect($allowed_nodes, $together);
>> +
>> + for my $node (sort keys %$together) {
>> + $mandatory_nodes->{$node} = 1 if $together->{$node}->{strict};
>> + }
>> +
>> + if (scalar keys %$mandatory_nodes) {
>> + # limit to only the nodes the service must be on.
>> + for my $node (keys %$allowed_nodes) {
>> + next if exists($mandatory_nodes->{$node});
>
> Style nit: I'd avoid using exists() if you explicitly expect a set
> value. Otherwise, it can break because of accidental auto-vivification
> in the future.
ACK, I already ran into this when restructuring this helper ;).
>
>> +
>> + delete $allowed_nodes->{$node};
>> + }
>> + } elsif (scalar keys %$possible_nodes) {
>> + # limit to the possible nodes the service should be on, if there are any.
>> + for my $node (keys %$allowed_nodes) {
>> + next if exists($possible_nodes->{$node});
>> +
>> + delete $allowed_nodes->{$node};
>
> This seems wrong. Non-strict rules should not limit the allowed nodes.
> See below for more on this.
Already addressed with the other thread :)
>
>> + }
>> + }
>> +}
>> +
>> +=head3 apply_negative_colocation_rules($separate, $allowed_nodes)
>> +
>> +Applies the negative colocation preference C<$separate> on the allowed node
>> +hash set C<$allowed_nodes> directly.
>> +
>> +Negative colocation means keeping services separate on multiple nodes, and
>> +therefore maximizing the separation of services.
>> +
>> +The allowed node hash set C<$allowed_nodes> is expected to contain any node,
>> +which is available to the service, i.e. each node is currently online, is
>> +available according to other location constraints, and the service has not
>> +failed running there yet.
>> +
>> +=cut
>> +
>> +sub apply_negative_colocation_rules {
>> + my ($separate, $allowed_nodes) = @_;
>> +
>> + return if scalar(keys %$separate) < 1;
>> +
>> + my $mandatory_nodes = {};
>> + my $possible_nodes = PVE::HA::Tools::set_difference($allowed_nodes, $separate);
>> +
>> + for my $node (sort keys %$separate) {
>> + $mandatory_nodes->{$node} = 1 if $separate->{$node}->{strict};
>> + }
>> +
>> + if (scalar keys %$mandatory_nodes) {
>> + # limit to the nodes the service must not be on.
>> + for my $node (keys %$allowed_nodes) {
>> + next if !exists($mandatory_nodes->{$node});
>> +
>> + delete $allowed_nodes->{$node};
>> + }
>> + } elsif (scalar keys %$possible_nodes) {
>> + # limit to the nodes the service should not be on, if any.
>> + for my $node (keys %$allowed_nodes) {
>> + next if exists($possible_nodes->{$node});
>> +
>> + delete $allowed_nodes->{$node};
>> + }
>> + }
>> +}
>> +
>> +sub apply_colocation_rules {
>> + my ($rules, $sid, $allowed_nodes, $online_node_usage) = @_;
>> +
>> + my ($together, $separate) = get_colocation_preference($rules, $sid, $online_node_usage);
>> +
>> + apply_positive_colocation_rules($together, $allowed_nodes);
>> + apply_negative_colocation_rules($separate, $allowed_nodes);
>
> I think there could be a problematic scenario with
> * no strict positive rules, but loose strict positive rules
> * strict negative rules
> where apply_positive_colocation_rules() will limit $allowed_nodes in
> such a way that the strict negative rules cannot be satisfied anymore
> afterwards.
>
> I feel like what we actually want from non-strict rules is not to limit
> the allowed nodes at all, but only express preferences. After scoring,
> we could:
> 1. always take a colocation preference node if present no matter what
> the usage score is
> 2. have a threshold to not follow through, if there is a non-colocation
> preference node with a much better usage score relatively
> 3. somehow massage it into the score itself. E.g. every node that would
> be preferred by colocation gets a 0.5 multiplier score adjustment while
> other scores are unchanged - remember that lower score is better.
> 4. [insert your suggestion here]
>
> So to me it seems like there should be a helper that gives us:
> 1. list of nodes that satisfy strict rules - these we can then intersect
> with the $pri_nodes
> 2. list of nodes that are preferred by non-strict rules - these we can
> consider after scoring
As mentioned in another reply, I think this should be addressed in a
separate follow-up and hopefully gets some user feedback to see what
their requirements are.
Especially since we want users to configure the threshold or score how
much the rule is preferred. I also would like to see this be somewhat
comparable to the HA group node priorities so that users can have a
mental model what these scores mean.
>
>> +}
>> +
>> 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) = @_;
>>
>> my $group = get_service_group($groups, $online_node_usage, $service_conf);
>>
>> @@ -189,6 +382,8 @@ sub select_service_node {
>>
>> return $current_node if (!$try_next && !$best_scored) && $pri_nodes->{$current_node};
>>
>> + apply_colocation_rules($rules, $sid, $pri_nodes, $online_node_usage);
>> +
>> my $scores = $online_node_usage->score_nodes_to_start_service($sid, $current_node);
>> my @nodes = sort {
>> $scores->{$a} <=> $scores->{$b} || $a cmp $b
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel