[pve-devel] [PATCH ha-manager 04/15] add rules section config base plugin

[Daniel Kral]

On 4/25/25 11:12, Fiona Ebner wrote:
> Am 25.04.25 um 10:29 schrieb Daniel Kral:
>> On 4/24/25 15:03, Fiona Ebner wrote:
>>> Am 25.03.25 um 16:12 schrieb Daniel Kral:
>>>> +
>>>> +    $func->($rule, $ruleid);
>>>> +    }
>>>> +}
>>>> +
>>>> +sub canonicalize {
>>>> +    my ($class, $rules, $groups, $services) = @_;
>>>> +
>>>> +    die "implement in subclass";
>>>> +}
>>>> +
>>>> +sub are_satisfiable {
>>>> +    my ($class, $rules, $groups, $services) = @_;
>>>> +
>>>> +    die "implement in subclass";
>>>> +}
>>>
>>> This might not be possible to implement in just the subclasses. E.g.
>>> services 1 and 2 have strict colocation with each other, but 1 has
>>> restricted location on node A and 2 has restricted location on node B.
>>>
>>> I don't think it hurts to rather put the implementation here with
>>> knowledge of all rule types and what inter-dependencies they entail. And
>>> maybe have it be a function rather than a method then?
>>
>> Yes, you're right, it would make more sense to have these be functions
>> rather than methods. In the current implementation it's rather confusing
>> and in the end $rules should consist of all types of rules, so $groups
>> and $services are hopefully not needed as separate parameters anymore
>> (The only usage for these are to check for HA group members).
> 
> For canonicalize(), I don't think it's a hard requirement. Can still be
> useful for further optimization of course.
> 
>> What do you think about something like a
>>
>> sub register_rule_check {
>>      my ($class, $check_func, $canonicalize_func, $satisfiable_func) = @_;
>> }
>>
>> in the base plugin and then each plugin can register their checker
>> methods with the behavior what is done when running canonicalize(...)
>> and are_satisfiable(...)? These then have to go through every registered
>> entry in the list and call $check_func and then either
>> $canonicalize_func and $satisfiable_func.
> 
> I don't see how that would help with the scenario I described above
> where the non-satisfiability can only be seen by knowing about
> inter-dependencies between rules.
> 
>> Another (simpler) option would be to just put all checker subroutines in
>> the base plugin, but that could get unmaintainable quite fast.
> 
> I think the helpers should go into the plugins. These can be designed to
> take the constraints arising from the inter-dependency as arguments.
> E.g. a helper in the location plugin, simply checking if the location
> rules are satisfiable (no constraints) and returning the arising
> services<->nodes constraints. A helper in the colocation plugin to check
> if colocation rules are satisfiable given certain services<->nodes
> constraints. The main function in the base plugin would just need to
> call these two in order then.

As discussed off-list, I'll take a closer look how we can improve the 
interface of the helpers between the Location and Colocation plugin 
here, so that they are less coupled on one another.

Depending on how large the rule set can get, I could see some possible 
improvements to factor out some of the common checks as pointed out by 
Fabian and you on/off-list, so that they're only done once, but as 
discussed off-list, I'll wait until their configuration variables are 
settled (nofailback, enabled/disabled/conflict).

> 
>>>> +sub checked_config {
>>>> +    my ($rules, $groups, $services) = @_;
>>>> +
>>>> +    my $types = __PACKAGE__->lookup_types();
>>>> +
>>>> +    for my $type (@$types) {
>>>> +    my $plugin = __PACKAGE__->lookup($type);
>>>> +
>>>> +    $plugin->canonicalize($rules, $groups, $services);
>>>
>>> Shouldn't we rather only pass the rules that belong to the specific
>>> plugin rather than always all?
>>
>> As in the previous comment, I think it would be reasonable to pass all
>> types of rules as there are some checks that require to check between
>> colocation and location rules, for example. But it would also make sense
>> to move these more general checks in the base plugin, so that the
>> checkers in the plugins have to only care about their own feasibility.
> 
> Again, IMHO we could have the plugins implement suitable helper
> functions, but put the logic that knows about inter-dependencies into
> the base plugin itself. Otherwise, you essentially need every plugin to
> care about all others, rather than having only the common base plugin
> care about all.
> 
> So design the helpers in expectation of what inter-dependencies we need
> to consider (this will of course change with future rules, but we are
> flexible to adapt), but don't have the plugins be concerned with other
> plugins directly, i.e. they don't need to know how the constraints arise
> from other rule types.

Similar to the above, having a more decoupled interface or separating 
check helpers into "plugin-local" and "global" (e.g. checking 
inconsistent inter-dependencies between location and colocation rules) 
makes sense here.