On 4/25/25 16:05, Fiona Ebner wrote:
Not much to add to Fabian's review :)
Am 25.03.25 um 16:12 schrieb Daniel Kral:
diff --git a/src/PVE/HA/Rules/Colocation.pm b/src/PVE/HA/Rules/Colocation.pm
new file mode 100644
index 0000000..808d48e
--- /dev/null
+++ b/src/PVE/HA/Rules/Colocation.pm
@@ -0,0 +1,391 @@
+package PVE::HA::Rules::Colocation;
+
+use strict;
+use warnings;
+
+use Data::Dumper;
+
+use PVE::JSONSchema qw(get_standard_option);
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::HA::Rules);
+
+sub type {
+ return 'colocation';
+}
+
+sub properties {
+ return {
+ services => get_standard_option('pve-ha-resource-id-list'),
+ affinity => {
+ description => "Describes whether the services are supposed to be kept
on separate"
+ . " nodes, or are supposed to be kept together on the same
node.",
+ type => 'string',
+ enum => ['separate', 'together'],
+ optional => 0,
+ },
+ strict => {
+ description => "Describes whether the colocation rule is mandatory or
optional.",
+ type => 'boolean',
+ optional => 0,
+ },
+ }
Style nit: missing semicolon
Since we should move the property definitions to the base module once a
second plugin re-uses them later: should we already declare 'services'
and 'strict' in the base module to start out? Then we could implement
the encode/decode part for 'services' there already. Less moving around
or duplication later on.
Yes, especially as Fabian also agreed that it would make sense that
users are allowed to make location rules for multiple services in a
single rule.
I'll start to use the isolated_properties option that @Dominik
implemented so that other options can be separated and have
plugin-specific descriptions, etc. but services can definitely live with
a more general description.
+}
+
+sub options {
+ return {
+ services => { optional => 0 },
+ strict => { optional => 0 },
+ affinity => { optional => 0 },
+ comment => { optional => 1 },
+ };
+};
+
+sub decode_value {
+ my ($class, $type, $key, $value) = @_;
+
+ if ($key eq 'services') {
+ my $res = {};
+
+ for my $service (PVE::Tools::split_list($value)) {
+ if (PVE::HA::Tools::pve_verify_ha_resource_id($service)) {
+ $res->{$service} = 1;
+ }
+ }
+
+ return $res;
+ }
+
+ return $value;
+}
+
+sub encode_value {
+ my ($class, $type, $key, $value) = @_;
+
+ if ($key eq 'services') {
+ PVE::HA::Tools::pve_verify_ha_resource_id($_) for (keys %$value);
Style nit:
[I] febner@dev8 /usr/share/perl5/PVE> ag "for keys" | wc -l
28
[I] febner@dev8 /usr/share/perl5/PVE> ag "for \(keys" | wc -l
0
ACK, will change that :)
+
+ return join(',', keys %$value);
+ }
+
+ return $value;
+}
+
---snip 8<---
+=head3 check_service_count($rules)
+
+Returns a list of conflicts caused by colocation rules, which do not have
+enough services in them, defined in C<$rules>.
+
+If there are no conflicts, the returned list is empty.
+
+=cut
+
+sub check_services_count {
+ my ($rules) = @_;
+
+ my $conflicts = [];
+
+ foreach_colocation_rule($rules, sub {
+ my ($rule, $ruleid) = @_;
+
+ push @$conflicts, $ruleid if (scalar(keys %{$rule->{services}}) < 2);
Style nit: parentheses for post-if
ACK, removed the outer parentheses
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
