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
