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

[Fiona Ebner <f.ebner@proxmox.com>]

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.

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


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