From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id CA2671FF164
for <inbox@lore.proxmox.com>; Fri, 25 Apr 2025 10:29:29 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id F2B9B1E539;
Fri, 25 Apr 2025 10:29:35 +0200 (CEST)
Message-ID: <89f355fb-6aff-4347-aefc-91edef71d93e@proxmox.com>
Date: Fri, 25 Apr 2025 10:29:01 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250325151254.193177-1-d.kral@proxmox.com>
<20250325151254.193177-6-d.kral@proxmox.com>
<c9a5bd93-751f-4861-89ee-5e5bb1cb1c80@proxmox.com>
Content-Language: en-US
From: Daniel Kral <d.kral@proxmox.com>
In-Reply-To: <c9a5bd93-751f-4861-89ee-5e5bb1cb1c80@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.011 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [rules.pm]
Subject: Re: [pve-devel] [PATCH ha-manager 04/15] add rules section config
base plugin
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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