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

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

On 4/24/25 15:03, Fiona Ebner wrote:
> Am 25.03.25 um 16:12 schrieb Daniel Kral:
>> Add a rules section config base plugin to allow users to specify
>> different kinds of rules in a single configuration file.
>>
>> The interface is designed to allow sub plugins to implement their own
>> {decode,encode}_value() methods and also offer a canonicalized version
> 
> It's not "allow" them to implement, but actually requires them to
> implement it. Otherwise, it would be infinite recursion.

ACK will change the wording here.

> 
>> of their rules with canonicalize(), i.e. with any inconsistencies
>> removed and ambiguities resolved. There is also a are_satisfiable()
>> method for anticipation of the verification of additions or changes to
>> the rules config via the API.
> 
> ---snip 8<---
> 
>> diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm
>> new file mode 100644
>> index 0000000..bff3375
>> --- /dev/null
>> +++ b/src/PVE/HA/Rules.pm
>> @@ -0,0 +1,118 @@
>> +package PVE::HA::Rules;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::SectionConfig;
> 
> Missing include of PVE::Tools.
> 
> Nit: I'd put a blank here to separate modules from different packages
> and modules from the same package.

ACK both.

> 
>> +use PVE::HA::Tools;
> 
>> +
>> +use base qw(PVE::SectionConfig);
>> +
>> +# TODO Add descriptions, completions, etc.
>> +my $defaultData = {
>> +    propertyList => {
>> +	type => { description => "Rule type." },
>> +	ruleid => get_standard_option('pve-ha-rule-id'),
>> +	comment => {
>> +	    type => 'string',
>> +	    maxLength => 4096,
>> +	    description => "Rule description.",
>> +	},
> 
> Oh good, so there already is a comment property :)
> 
> ---snip 8<---
> 
>> +sub foreach_service_rule {
>> +    my ($rules, $func, $opts) = @_;
>> +
>> +    my $sid = $opts->{sid};
>> +    my $type = $opts->{type};
>> +
>> +    my @ruleids = sort {
>> +	$rules->{order}->{$a} <=> $rules->{order}->{$b}
>> +    } keys %{$rules->{ids}};
>> +
>> +    for my $ruleid (@ruleids) {
>> +	my $rule = $rules->{ids}->{$ruleid};
>> +
>> +	next if !$rule; # invalid rules are kept undef in section config, delete them
> 
> s/delete/skip/ ?

ACK

> 
>> +	next if $type && $rule->{type} ne $type;
>> +	next if $sid && !defined($rule->{services}->{$sid});
> 
> Style nit: I'd prefer defined($type) and defined($sid) in the above
> expressions

ACK

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

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.

Another (simpler) option would be to just put all checker subroutines in 
the base plugin, but that could get unmaintainable quite fast.

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


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