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

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

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.

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

> +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/ ?

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

> +
> +	$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?

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


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