Re: [pve-devel] [PATCH ha-manager v2 03/26] introduce rules base plugin

["Michael Köppl" <m.koeppl@proxmox.com>]

Just noted a couple of typos inline

On 6/20/25 16:31, Daniel Kral wrote:
> Add a rules base plugin to allow users to specify different kinds of HA
> rules in a single configuration file, which put constraints on the HA
> Manager's behavior.
> 
> Rule checkers can be registered for every plugin with the
> register_check(...) method and are used for checking the feasibility of
> the rule set with check_feasibility(...), and creating a feasible rule
> set with canonicalize(...).
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> changes since v1:
>     - added documentation
>     - added state property to enable/disable rules (and allow a
>       tri-state 'contradictory' state for the API endpoint there)
>     - replace `{encode,decode}_value` plugin calls to
>       `{encode,decode}_plugin_value` with default implementation to make
>       these implementations optional for rule plugins
>     - introduce `set_rule_defaults` to set optional fields with defaults
>       in later patches
>     - renamed `are_satisfiable` to `check_feasibility`
>     - factored `canonicalize` and `checked_config` to only
>       `canonicalize`
>     - refactored the feasibility checks to be registered as individual
>       checkers with `register_check`
>     - refactored the canonicalization for plugins to be a single call to
>       plugin_canonicalize (if implemented)
>     - added rule auto completion
> 
>  debian/pve-ha-manager.install |   1 +
>  src/PVE/HA/Makefile           |   2 +-
>  src/PVE/HA/Rules.pm           | 445 ++++++++++++++++++++++++++++++++++
>  src/PVE/HA/Tools.pm           |  22 ++
>  4 files changed, 469 insertions(+), 1 deletion(-)
>  create mode 100644 src/PVE/HA/Rules.pm
> 
> diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
> index 0ffbd8d..9bbd375 100644
> --- a/debian/pve-ha-manager.install
> +++ b/debian/pve-ha-manager.install
> @@ -32,6 +32,7 @@
>  /usr/share/perl5/PVE/HA/Resources.pm
>  /usr/share/perl5/PVE/HA/Resources/PVECT.pm
>  /usr/share/perl5/PVE/HA/Resources/PVEVM.pm
> +/usr/share/perl5/PVE/HA/Rules.pm
>  /usr/share/perl5/PVE/HA/Tools.pm
>  /usr/share/perl5/PVE/HA/Usage.pm
>  /usr/share/perl5/PVE/HA/Usage/Basic.pm
> diff --git a/src/PVE/HA/Makefile b/src/PVE/HA/Makefile
> index 8c91b97..489cbc0 100644
> --- a/src/PVE/HA/Makefile
> +++ b/src/PVE/HA/Makefile
> @@ -1,4 +1,4 @@
> -SIM_SOURCES=CRM.pm Env.pm Groups.pm Resources.pm LRM.pm Manager.pm \
> +SIM_SOURCES=CRM.pm Env.pm Groups.pm Rules.pm Resources.pm LRM.pm Manager.pm \
>  	NodeStatus.pm Tools.pm FenceConfig.pm Fence.pm Usage.pm
>  
>  SOURCES=${SIM_SOURCES} Config.pm
> diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm
> new file mode 100644
> index 0000000..e1c84bc
> --- /dev/null
> +++ b/src/PVE/HA/Rules.pm
> @@ -0,0 +1,445 @@
> +package PVE::HA::Rules;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Tools;
> +
> +use PVE::HA::Tools;
> +
> +use base qw(PVE::SectionConfig);
> +
> +=head1 NAME
> +
> +PVE::HA::Rules - Base Plugin for HA Rules
> +
> +=head1 SYNOPSIS
> +
> +    use base qw(PVE::HA::Rules);
> +
> +=head1 DESCRIPTION
> +
> +This package provides the capability to have different types of rules in the
> +same config file, which put constraints or other rules that the HA Manager must
> +or should follow.
> +
> +Since rules can interfere with each other, i.e., rules can make other rules
> +invalid or infeasible, this package also provides the capability to check for
> +the feasibility of the rules within a rule plugin and between different rule
> +plugins and also prune the rule set in such a way, that becomes feasible again.
> +
> +This packages inherits its config-related methods from C<L<PVE::SectionConfig>>
> +and therefore rule plugins need to implement methods from there as well.
> +
> +=head1 USAGE
> +
> +Each I<rule plugin> is required to implement the methods C<L<type()>>,
> +C<L<properties()>>, and C<L<options>> from the C<L<PVE::SectionConfig>> to
> +extend the properties of this I<base plugin> with plugin-specific properties.
> +
> +=head2 REGISTERING CHECKS
> +
> +In order to C<L<< register|/$class->register_check(...) >>> checks for a rule
> +plugin, the plugin can override the
> +C<L<< get_plugin_check_arguments()|/$class->get_plugin_check_arguments(...) >>>
> +method, which allows the plugin's checkers to pass plugin-specific subsets of
> +specific rules, which are relevant to the checks.
> +
> +The following example shows a plugin's implementation of its
> +C<L<< get_plugin_check_arguments()|/$class->get_plugin_check_arguments(...) >>>
> +and a trivial check, which will render all rules defining a comment erroneous,
> +and blames these errors on the I<comment> property:
> +
> +    sub get_plugin_check_arguments {
> +	my ($class, $rules) = @_;
> +
> +	my @ruleids = sort {
> +	    $rules->{order}->{$a} <=> $rules->{order}->{$b}
> +	} keys %{$rules->{ids}};
> +
> +	my $result = {
> +	    custom_rules => {},
> +	};
> +
> +	for my $ruleid (@ruleids) {
> +	    my $rule = $rules->{ids}->{$ruleid};
> +
> +	    $result->{custom_rules}->{$ruleid} = $rule if defined($rule->{comment});
> +	}
> +
> +	return $result;
> +    }
> +
> +    __PACKAGE__->register_check(
> +	sub {
> +	    my ($args) = @_;
> +
> +	    return [ sort keys $args->{custom_rules}->%* ];
> +	},
> +	sub {
> +	    my ($ruleids, $errors) = @_;
> +
> +	    for my $ruleid (@$ruleids) {
> +		push @{$errors->{$ruleid}->{comment}},
> +		    "rule is ineffective, because I said so.";
> +	    }
> +	}
> +    );
> +
> +=head1 METHODS
> +
> +=cut
> +
> +my $defaultData = {
> +    propertyList => {
> +        type => {
> +            description => "HA rule type.",
> +        },
> +        rule => get_standard_option(
> +            'pve-ha-rule-id',
> +            {
> +                completion => \&PVE::HA::Tools::complete_rule,
> +                optional => 0,
> +            },
> +        ),
> +        state => {
> +            description => "State of the HA rule.",
> +            type => 'string',
> +            enum => [
> +                'enabled', 'disabled',
> +            ],
> +            default => 'enabled',
> +            optional => 1,
> +        },
> +        comment => {
> +            description => "HA rule description.",
> +            type => 'string',
> +            maxLength => 4096,
> +            optional => 1,
> +        },
> +    },
> +};
> +
> +sub private {
> +    return $defaultData;
> +}
> +
> +=head3 $class->decode_plugin_value(...)
> +
> +=head3 $class->decode_plugin_value($type, $key, $value)
> +
> +B<OPTIONAL:> Can be implemented in a I<rule plugin>.
> +
> +Called during base plugin's C<decode_value()> in order to extend the
> +deserialization for plugin-specific values which need it (e.g. lists).
> +
> +If it is not overrridden by the I<rule plugin>, then it does nothing to
> +C<$value> by default.
> +
> +=cut
> +
> +sub decode_plugin_value {
> +    my ($class, $type, $key, $value) = @_;
> +
> +    return $value;
> +}
> +
> +sub decode_value {
> +    my ($class, $type, $key, $value) = @_;
> +
> +    if ($key eq 'comment') {
> +        return PVE::Tools::decode_text($value);
> +    }
> +
> +    my $plugin = $class->lookup($type);
> +    return $plugin->decode_plugin_value($type, $key, $value);
> +}
> +
> +=head3 $class->encode_plugin_value(...)
> +
> +=head3 $class->encode_plugin_value($type, $key, $value)
> +
> +B<OPTIONAL:> Can be implemented in a I<rule plugin>.
> +
> +Called during base plugin's C<encode_value()> in order to extend the
> +serialization for plugin-specific values which need it (e.g. lists).
> +
> +If it is not overrridden by the I<rule plugin>, then it does nothing to

nit: there's an extra r

> +C<$value> by default.
> +
> +=cut
> +
> +sub encode_plugin_value {
> +    my ($class, $type, $key, $value) = @_;
> +
> +    return $value;
> +}
> +
> +sub encode_value {
> +    my ($class, $type, $key, $value) = @_;
> +
> +    if ($key eq 'comment') {
> +        return PVE::Tools::encode_text($value);
> +    }
> +
> +    my $plugin = $class->lookup($type);
> +    return $plugin->encode_plugin_value($type, $key, $value);
> +}
> +
> +sub parse_section_header {
> +    my ($class, $line) = @_;
> +
> +    if ($line =~ m/^(\S+):\s*(\S+)\s*$/) {
> +        my ($type, $ruleid) = (lc($1), $2);
> +        my $errmsg = undef; # set if you want to skip whole section
> +        eval { PVE::JSONSchema::pve_verify_configid($ruleid); };
> +        $errmsg = $@ if $@;
> +        my $config = {}; # to return additional attributes
> +        return ($type, $ruleid, $errmsg, $config);
> +    }
> +    return undef;
> +}
> +
> +# General rule helpers
> +
> +=head3 $class->set_rule_defaults($rule)
> +
> +Sets the optional properties in the C<$rule>, which have default values, but
> +haven't been explicitly set yet.
> +
> +=cut
> +
> +sub set_rule_defaults : prototype($$) {
> +    my ($class, $rule) = @_;
> +
> +    $rule->{state} = 'enabled' if !defined($rule->{state});
> +
> +    if (my $plugin = $class->lookup($rule->{type})) {
> +        my $properties = $plugin->properties();
> +
> +        for my $prop (keys %$properties) {
> +            next if defined($rule->{$prop});
> +            next if !$properties->{$prop}->{default};
> +            next if !$properties->{$prop}->{optional};
> +
> +            $rule->{$prop} = $properties->{$prop}->{default};
> +        }
> +    }
> +}
> +
> +# Rule checks definition and methods
> +
> +my $types = [];
> +my $checkdef;
> +
> +sub register {
> +    my ($class) = @_;
> +
> +    $class->SUPER::register($class);
> +
> +    # store order in which plugin types are registered
> +    push @$types, $class->type();
> +}
> +
> +=head3 $class->register_check(...)
> +
> +=head3 $class->register_check($check_func, $collect_errors_func)
> +
> +Used to register rule checks for a rule plugin.
> +
> +=cut
> +
> +sub register_check : prototype($$$) {
> +    my ($class, $check_func, $collect_errors_func) = @_;
> +
> +    my $type = eval { $class->type() };
> +    $type = 'global' if $@; # check registered here in the base plugin
> +
> +    push @{ $checkdef->{$type} }, [
> +        $check_func, $collect_errors_func,
> +    ];
> +}
> +
> +=head3 $class->get_plugin_check_arguments(...)
> +
> +=head3 $class->get_plugin_check_arguments($rules)
> +
> +B<OPTIONAL:> Can be implemented in the I<rule plugin>.
> +
> +Returns a hash of subsets of rules, which are passed to the plugin's
> +C<L<< registered checks|/$class->register_check(...) >>> so that the
> +creation of these can be shared inbetween rule check implementations.
> +
> +=cut
> +
> +sub get_plugin_check_arguments : prototype($$) {
> +    my ($class, $rules) = @_;
> +
> +    return {};
> +}
> +
> +=head3 $class->get_check_arguments(...)
> +
> +=head3 $class->get_check_arguments($rules)
> +
> +Returns the union of the plugin's subsets of rules, which are passed to the
> +plugin's C<L<< registered checks|/$class->register_check(...) >>> so that the
> +creation of these can be shared inbetween rule check implementations.
> +
> +=cut
> +
> +sub get_check_arguments : prototype($$) {
> +    my ($class, $rules) = @_;
> +
> +    my $global_args = {};
> +
> +    for my $type (@$types) {
> +        my $plugin = $class->lookup($type);
> +        my $plugin_args = eval { $plugin->get_plugin_check_arguments($rules) };
> +        next if $@; # plugin doesn't implement get_plugin_check_arguments(...)
> +
> +        $global_args = { $global_args->%*, $plugin_args->%* };
> +    }
> +
> +    return $global_args;
> +}
> +
> +=head3 $class->check_feasibility($rules)
> +
> +Checks whether the given C<$rules> are feasible by running all checks, which
> +were registered with C<L<< register_check()|/$class->register_check(...) >>>,
> +and returns a hash map of errorneous rules.

nit: s/errorneous/erroneous

> +
> +The checks are run in the order in which the rule plugins were registered,
> +while global checks, i.e. checks between different rule types, are run at the
> +very last.
> +
> +=cut
> +
> +sub check_feasibility : prototype($$) {
> +    my ($class, $rules) = @_;
> +
> +    my $global_errors = {};
> +    my $removable_ruleids = [];
> +
> +    my $global_args = $class->get_check_arguments($rules);
> +
> +    for my $type (@$types, 'global') {
> +        for my $entry (@{ $checkdef->{$type} }) {
> +            my ($check, $collect_errors) = @$entry;
> +
> +            my $errors = $check->($global_args);
> +            $collect_errors->($errors, $global_errors);
> +        }
> +    }
> +
> +    return $global_errors;
> +}
> +
> +=head3 $class->plugin_canonicalize($rules)
> +
> +B<OPTIONAL:> Can be implemented in the I<rule plugin>.
> +
> +Modifies the C<$rules> to a plugin-specific canonical form.
> +
> +=cut
> +
> +sub plugin_canonicalize : prototype($$) {
> +    my ($class, $rules) = @_;
> +}
> +
> +=head3 $class->canonicalize($rules)
> +
> +Modifies C<$rules> to contain only feasible rules.
> +
> +This is done by running all checks, which were registered with
> +C<L<< register_check()|/$class->register_check(...) >>> and removing any
> +rule, which makes the rule set infeasible.
> +
> +Returns a list of messages with the reasons why rules were removed.
> +
> +=cut
> +
> +sub canonicalize : prototype($$) {
> +    my ($class, $rules) = @_;
> +
> +    my $messages = [];
> +    my $global_errors = $class->check_feasibility($rules);
> +
> +    for my $ruleid (keys %$global_errors) {
> +        delete $rules->{ids}->{$ruleid};
> +        delete $rules->{order}->{$ruleid};
> +    }
> +
> +    for my $ruleid (sort keys %$global_errors) {
> +        for my $opt (sort keys %{ $global_errors->{$ruleid} }) {
> +            for my $message (@{ $global_errors->{$ruleid}->{$opt} }) {
> +                push @$messages, "Drop rule '$ruleid', because $message.\n";
> +            }
> +        }
> +    }
> +
> +    for my $type (@$types) {
> +        my $plugin = $class->lookup($type);
> +        eval { $plugin->plugin_canonicalize($rules) };
> +        next if $@; # plugin doesn't implement plugin_canonicalize(...)
> +    }
> +
> +    return $messages;
> +}
> +
> +=head1 FUNCTIONS
> +
> +=cut
> +
> +=head3 foreach_rule(...)
> +
> +=head3 foreach_rule($rules, $func [, $opts])
> +
> +Filters the given C<$rules> according to the C<$opts> and loops over the
> +resulting rules in the order as defined in the section config and executes
> +C<$func> with the parameters C<L<< ($rule, $ruleid) >>>.
> +
> +The filter properties for C<$opts> are:
> +
> +=over
> +
> +=item C<$type>
> +
> +Limits C<$rules> to those which are of rule type C<$type>.
> +
> +=item C<$state>
> +
> +Limits C<$rules> to those which are in the rule state C<$state>.
> +
> +=back
> +
> +=cut
> +
> +sub foreach_rule : prototype($$;$) {
> +    my ($rules, $func, $opts) = @_;
> +
> +    my $type = $opts->{type};
> +    my $state = $opts->{state};
> +
> +    my @ruleids = sort {
> +        $rules->{order}->{$a} <=> $rules->{order}->{$b}
> +    } keys %{ $rules->{ids} };
> +
> +    for my $ruleid (@ruleids) {
> +        my $rule = $rules->{ids}->{$ruleid};
> +
> +        next if !$rule; # skip invalid rules
> +        next if defined($type) && $rule->{type} ne $type;
> +
> +        # rules are enabled by default
> +        my $rule_state = defined($rule->{state}) ? $rule->{state} : 'enabled';
> +
> +        next if defined($state) && $rule_state ne $state;
> +
> +        $func->($rule, $ruleid);
> +    }
> +}
> +
> +1;
> diff --git a/src/PVE/HA/Tools.pm b/src/PVE/HA/Tools.pm
> index a01ac38..767659f 100644
> --- a/src/PVE/HA/Tools.pm
> +++ b/src/PVE/HA/Tools.pm
> @@ -112,6 +112,15 @@ PVE::JSONSchema::register_standard_option(
>      },
>  );
>  
> +PVE::JSONSchema::register_standard_option(
> +    'pve-ha-rule-id',
> +    {
> +        description => "HA rule identifier.",
> +        type => 'string',
> +        format => 'pve-configid',
> +    },
> +);
> +
>  sub read_json_from_file {
>      my ($filename, $default) = @_;
>  
> @@ -292,4 +301,17 @@ sub complete_group {
>      return $res;
>  }
>  
> +sub complete_rule {
> +    my ($cmd, $pname, $cur) = @_;
> +
> +    my $cfg = PVE::HA::Config::read_rules_config();
> +
> +    my $res = [];
> +    foreach my $rule (keys %{ $cfg->{ids} }) {
> +        push @$res, $rule;
> +    }
> +
> +    return $res;
> +}
> +
>  1;



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