[pve-devel] [PATCH ha-manager 1/2] fix #4984: recompute online usage: add service usage to migration target only if online

[pve-devel] [PATCH ha-manager 1/2] fix #4984: recompute online usage: add service usage to migration target only if online

[Fiona Ebner]

Otherwise, when using the 'basic' plugin, this would lead to
autovivification of the $target node in the Perl hash tracking the
usage and it would wrongly be considered online when selecting the
recovery node.

The 'static' plugin was not affected, because it would check and warn
before adding usage to a node that was not registered with add_node()
first. Doing the same in the 'basic' plugin will be done by another
patch.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/HA/Manager.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 6a13360..d983672 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -275,7 +275,8 @@ sub recompute_online_node_usage {
 		# count it for both, source and target as load is put on both
 		$online_node_usage->add_service_usage_to_node($source, $sid, $source, $target)
 		    if $state ne 'request_start_balance';
-		$online_node_usage->add_service_usage_to_node($target, $sid, $source, $target);
+		$online_node_usage->add_service_usage_to_node($target, $sid, $source, $target)
+		    if $online_node_usage->contains_node($target);
 	    } elsif ($state eq 'stopped' || $state eq 'request_start') {
 		# do nothing
 	    } else {
-- 
2.39.2

[pve-devel] [PATCH ha-manager 2/2] usage: basic: avoid node autovivification when adding service usage

[Fiona Ebner]

Part of what caused bug #4984. Make the code future-proof and warn
when the node was never registered in the plugin, similar to what the
'static' usage plugin already does.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/HA/Usage/Basic.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/PVE/HA/Usage/Basic.pm b/src/PVE/HA/Usage/Basic.pm
index f066350..d6b3d6c 100644
--- a/src/PVE/HA/Usage/Basic.pm
+++ b/src/PVE/HA/Usage/Basic.pm
@@ -10,6 +10,7 @@ sub new {
 
     return bless {
 	nodes => {},
+	haenv => $haenv,
     }, $class;
 }
 
@@ -40,7 +41,14 @@ sub contains_node {
 sub add_service_usage_to_node {
     my ($self, $nodename, $sid, $service_node, $migration_target) = @_;
 
-    $self->{nodes}->{$nodename}++;
+    if ($self->contains_node($nodename)) {
+	$self->{nodes}->{$nodename}++;
+    } else {
+	$self->{haenv}->log(
+	    'warning',
+	    "unable to add service '$sid' usage to node '$nodename' - node not in usage hash",
+	);
+    }
 }
 
 sub score_nodes_to_start_service {
-- 
2.39.2

Re: [pve-devel] [PATCH ha-manager 1/2] fix #4984: recompute online usage: add service usage to migration target only if online

[Fiona Ebner]

Forgot to mention that this also applies to stable-7 (bug report was
with a Proxmox VE 7 system).

[pve-devel] applied-series: [PATCH ha-manager 1/2] fix #4984: recompute online usage: add service usage to migration target only if online

[Thomas Lamprecht]

Am 05/10/2023 um 16:05 schrieb Fiona Ebner:
> Otherwise, when using the 'basic' plugin, this would lead to
> autovivification of the $target node in the Perl hash tracking the
> usage and it would wrongly be considered online when selecting the
> recovery node.
> 
> The 'static' plugin was not affected, because it would check and warn
> before adding usage to a node that was not registered with add_node()
> first. Doing the same in the 'basic' plugin will be done by another
> patch.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/HA/Manager.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied both patches with slightly rewording/shortening the commit message
subject, thanks!

Re: [pve-devel] applied-series: [PATCH ha-manager 1/2] fix #4984: recompute online usage: add service usage to migration target only if online

[Fiona Ebner]

Am 06.10.23 um 18:10 schrieb Thomas Lamprecht:
> Am 05/10/2023 um 16:05 schrieb Fiona Ebner:
>> Otherwise, when using the 'basic' plugin, this would lead to
>> autovivification of the $target node in the Perl hash tracking the
>> usage and it would wrongly be considered online when selecting the
>> recovery node.
>>
>> The 'static' plugin was not affected, because it would check and warn
>> before adding usage to a node that was not registered with add_node()
>> first. Doing the same in the 'basic' plugin will be done by another
>> patch.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/HA/Manager.pm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>>
> 
> applied both patches with slightly rewording/shortening the commit message
> subject, thanks!

Does it make sense for stable-7 too or is this too much of an edge case?
The bug report was with Proxmox VE 7.