Re: [pve-devel] [PATCH ha-manager 05/15] rules: add colocation rule plugin

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

Agree with all here, but I propose to remove the merging for any 
colocation rules in the last comment, if there's nothing speaking 
against it.

On 4/25/25 16:06, Fiona Ebner wrote:
> Am 11.04.25 um 13:04 schrieb Daniel Kral:
>> On 4/3/25 14:16, Fabian Grünbichler wrote:
>>> On March 25, 2025 4:12 pm, Daniel Kral wrote:
>>>> +sub check_services_count {
>>>> +    my ($rules) = @_;
>>>> +
>>>> +    my $conflicts = [];
>>>> +
>>>> +    foreach_colocation_rule($rules, sub {
>>>> +    my ($rule, $ruleid) = @_;
>>>> +
>>>> +    push @$conflicts, $ruleid if (scalar(keys %{$rule->{services}})
>>>> < 2);
>>>> +    });
>>>> +
>>>> +    return $conflicts;
>>>> +}
>>>
>>> is this really an issue? a colocation rule with a single service is just
>>> a nop? there's currently no cleanup AFAICT if a resource is removed, but
>>
>> You're right, AFAICS those are a noop when selecting the service node. I
>> guess I was a little pedantic / overprotective here about which rules
>> make sense in general instead of what the algorithm does in the end.
>>
>> And good point about handling when resources are removed, adding that to
>> delete_service_from_config comes right on my TODO list for the v1!
>>
>>> if we add that part (we maybe should?) then one can easily end up in a
>>> situation where a rule temporarily contains a single or no service?
>>
>> Hm, yes, especially if we add pools/tags at a later point to select
>> services for the rule, then this could happen very easily. But as you
>> already mentioned, those two cases would be noops too.
>>
>> Nevertheless, should we drop this? I think it could benefit users in
>> identifying that some rules might not do something they wanted and give
>> them a reason why, i.e. there's only one service in there, but at the
>> same time it could be a little noisy if there are a lot of affected rules.
> 
> I'd still keep rules that end up with only one service around, but maybe
> (temporarily) disable them. And/or we could also add a special
> "no-effect" marker like the "conflict" one proposed in my other answer?
> Then a user could enable/make the rule effective again by adding a new
> service to it.

Yes, I think the 'conflict'/'ineffective' states are much more 
user-friendly opposed to dropping them and only making these decisions 
explicit in the log. I'll add that to the v1!

> 
>>>> +
>>>> +=head3 check_positive_intransitivity($rules)
>>>> +
>>>> +Returns a list of conflicts caused by transitive positive colocation
>>>> rules
>>>> +defined in C<$rules>.
>>>> +
>>>> +Transitive positive colocation rules exist, if there are at least
>>>> two positive
>>>> +colocation rules with the same strictness, which put at least the
>>>> same two
>>>> +services in relation. This means, that these rules can be merged
>>>> together.
>>>> +
>>>> +If there are no conflicts, the returned list is empty.
>>>
>>> The terminology here is quit confusing - conflict meaning that two rules
>>> are "transitive" and thus mergeable (which is good, cause it makes
>>> things easier to handle?) is quite weird, as "conflict" is a rather
>>> negative term..
>>>
>>> there's only a single call site in the same module, maybe we could just
>>> rename this into "find_mergeable_positive_ruleids", similar to the
>>> variable where the result is stored?
>>
>> Yeah, I was probably to keen on the `$conflict = check_something(...)`
>> pattern here, but it would be much more readable with a simpler name,
>> I'll change that for the v1!
>>
>> -----
>>
>> Ad why: I'll also add some documentation about the rationale why this is
>> needed in the first place.
>>
>> The main reason was because the latter rule check
>> 'check_inner_consistency' depends on the fact that the positive
>> colocation rules have been merged already, as it assumes that each
>> positive colocation rule has all of the services in there, which are
>> positively colocated. If it weren't so, it wouldn't detect that the
>> following three rules are inconsistent with each other:
>>
>> colocation: stick-together1
>>      services vm:101,vm:104
>>      affinity together
>>      strict 1
>>
>> colocation: stick-together2
>>      services vm:104,vm:102
>>      affinity together
>>      strict 1
>>
>> colocation: keep-apart
>>      services vm:101,vm:102,vm:103
>>      affinity separate
>>      strict 1
>>
>> This reduces the complexity of the logic a little in
>> 'check_inner_consistency' as there it doesn't have to handle this
>> special case as 'stick-together1' and 'stick-together2' are already
>> merged in to one and it is easily apparent that vm 101 and vm 102 cannot
>> be colocated and non-colocated at the same time.
>>
>> -----
>>
>> Also, I was curious about how that would work out for the case where a
>> negative colocation rule was defined for three nodes with those rules
>> split into three rules (essentially a cycle dependence). This should in
>> theory have the same semantics as the above rule set:
>>
>> colocation: stick-together1
>>      services vm:101,vm:104
>>      affinity together
>>      strict 1
>>
>> colocation: stick-together2
>>      services vm:104,vm:102
>>      affinity together
>>      strict 1
>>
>> colocation: very-lonely-services1
>>      services vm:101,vm:102
>>      affinity separate
>>      strict 1
>>
>> colocation: very-lonely-services2
>>      services vm:102,vm:103
>>      affinity separate
>>      strict 1
>>
>> colocation: very-lonely-services3
>>      services vm:101,vm:103
>>      affinity separate
>>      strict 1
>>
>> Without the merge of positive rules, 'check_inner_consistency' would
>> again not detect the inconsistency here. But with the merge correctly
>> applied before checking the consistency, this would be resolved and the
>> effective rule set would be:
> 
> I suppose the effective rule set would still also contain the two
> 'together' rules, or?

No, here it would not. I found it would be most fair or reasonable that 
if a positive and a negative colocation rule contradict each other to 
drop both of them. Here the conflicts are

stick-together1 -- very-lonely-services1
stick-together2 -- very-lonely-services1

so all three of them will be dropped from the rule set.

Seeing this again here, such cases definitely benefit from the immediate 
response with the 'conflict'/'ineffective' state to show users that 
those won't be applied instead of only logging it.

> 
>>
>> colocation: very-lonely-services2
>>      services vm:102,vm:103
>>      affinity separate
>>      strict 1
>>
>> colocation: very-lonely-services3
>>      services vm:101,vm:103
>>      affinity separate
>>      strict 1
>>
>> It could be argued, that the negative colocation rules should be merged
>> in a similar manner here, as there's now a "effective" difference in the
>> semantics of the above rule sets, as the negative colocation rule
>> between vm 101 and vm 103 and vm 102 and vm 103 remains.
>>
>> What do you think?
> 
> I don't think there's a particular need to also merge negative rules
> between services (when they form a complete graph). It won't make a
> difference if there are no conflicts with positive rules and in edge
> cases when there are conflicts (which usually gets caught while editing
> the rules), it's better to drop fewer rules, so not merging is an
> advantage. Or do you have a particular advantage in favor of merging in
> mind?

Yes, I think so too.

There's quite the semantic difference between positive and negative 
colocation rules here. "Connected" positive colocation relationships 
(strict ones in particular) must be co-located in the end anyway, so it 
makes sense to merge them. Negative colocation relationships must be 
defined in a "circular" way and might just happen by coincidence for 
small scenarios.

But one thing that just struck me is that what if the user intentionally 
wrote them as separate rules? Then it might be confusing that all rules 
are dropped and not just the minimal amount that contradict other 
rules... Then check_inner_consistency() would just drop the minimal 
amount of rules that need to be dropped as in the above example.

It would be a softer interpretation of the rules indeed, but it might 
benefit the user in the end and make things easier to follow from the 
user perspective. If there's no opposition to that, I'd tend to drop the 
merging for any rules after all.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel