[pve-devel] [PATCH-SERIES] correctly handle warnings status

[pve-devel] [PATCH-SERIES] correctly handle warnings status

[Fabian Ebner]

There's a few places where we use the logic "OK" if and only if no error for a
task status, but this is no longer true now that we also support ending in a
warnings status (not yet used by anything, but the feature is there).

Everything depends on the first patch and depedency bumps for pve-common
are needed across the board.


pve-common:

Fabian Ebner (1):
  tools: add upid_status_is_error function

 src/PVE/Tools.pm | 8 ++++++++
 1 file changed, 8 insertions(+)


pve-manager:

Fabian Ebner (4):
  api/cli: tasks: don't treat warnings status as an error status
  nodes: startall: correctly handle warning status for delayed task
  cli: pveam: remove unused private sub
  cli tools: correctly handle warnings status

 PVE/API2/Nodes.pm  | 2 +-
 PVE/API2/Tasks.pm  | 5 ++++-
 PVE/CLI/pveam.pm   | 6 ------
 PVE/CLI/pveceph.pm | 2 +-
 PVE/CLI/pvenode.pm | 7 +++++--
 PVE/CLI/vzdump.pm  | 2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)


pve-storage:

Fabian Ebner (1):
  api: content: correctly handle warnings status for delayed task

 PVE/API2/Storage/Content.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


pve-container:

Fabian Ebner (1):
  pct: correctly handle warnings task status

 src/PVE/CLI/pct.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


qemu-server:

Fabian Ebner (2):
  cli tools: correctly handle warnings task status
  api: update vm: correctly handle warnings status for delayed task

 PVE/API2/Qemu.pm     | 2 +-
 PVE/CLI/qm.pm        | 2 +-
 PVE/CLI/qmrestore.pm | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH common 1/9] tools: add upid_status_is_error function

[Fabian Ebner]

There's also support for ending a task with warnings now, so the logic "status
not 'OK' means error" does not work anymore.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/Tools.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 16ae3d2..6f205df 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1165,6 +1165,14 @@ sub upid_read_status {
     return "unable to read tail (got $br bytes)";
 }
 
+# Check if the status returned by upid_read_status is an error status.
+# If the status could not be parsed it's also treated as an error.
+sub upid_status_is_error {
+    my ($status) = @_;
+
+    return !($status eq 'OK' || $status =~ m/^WARNINGS: \d+$/);
+}
+
 # useful functions to store comments in config files
 sub encode_text {
     my ($text) = @_;
-- 
2.20.1

[pve-devel] [PATCH manager 2/9] api/cli: tasks: don't treat warnings status as an error status

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-common is needed.

 PVE/API2/Tasks.pm  | 5 ++++-
 PVE/CLI/pvenode.pm | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 8f6ab603..abae8ba3 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -142,7 +142,10 @@ __PACKAGE__->register_method({
 
 	    return 1 if $typefilter && $task->{type} ne $typefilter;
 
-	    return 1 if $errors && $task->{status} && $task->{status} eq 'OK';
+	    if ($errors && $task->{status} && !PVE::Tools::upid_status_is_error($task->{status})) {
+		return 1;
+	    }
+
 	    return 1 if $param->{vmid} && (!$task->{id} || $task->{id} ne $param->{vmid});
 
 	    return 1 if $count++ < $start;
diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
index a33fcd93..b05b3e15 100644
--- a/PVE/CLI/pvenode.pm
+++ b/PVE/CLI/pvenode.pm
@@ -181,7 +181,10 @@ our $cmddef = {
 	    foreach my $task (@$data) {
 		if (!defined($task->{status})) {
 		    $task->{status} = 'UNKNOWN';
-		} elsif ($task->{status} ne 'OK' && $task->{status} ne 'RUNNING') {
+		# RUNNING is set by the API call and needs to be checked explicitly
+		} elsif (PVE::Tools::upid_status_is_error($task->{status}) &&
+		    $task->{status} ne 'RUNNING')
+		{
 		    $task->{status} = 'ERROR';
 		}
 	    }
-- 
2.20.1

[pve-devel] [PATCH manager 3/9] nodes: startall: correctly handle warning status for delayed task

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Nodes.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index e58d9c10..1efba772 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1810,7 +1810,7 @@ __PACKAGE__->register_method ({
 			}
 
 			my $status = PVE::Tools::upid_read_status($upid);
-			if ($status eq 'OK') {
+			if (!PVE::Tools::upid_status_is_error($status)) {
 			    # use default delay to reduce load
 			    my $delay = defined($d->{up}) ? int($d->{up}) : $default_delay;
 			    if ($delay > 0) {
-- 
2.20.1

[pve-devel] [PATCH manager 4/9] cli: pveam: remove unused private sub

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pveam.pm | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/PVE/CLI/pveam.pm b/PVE/CLI/pveam.pm
index a506aa9b..6c26f209 100644
--- a/PVE/CLI/pveam.pm
+++ b/PVE/CLI/pveam.pm
@@ -20,12 +20,6 @@ use base qw(PVE::CLIHandler);
 
 my $nodename = PVE::INotify::nodename();
 
-my $upid_exit = sub {
-    my $upid = shift;
-    my $status = PVE::Tools::upid_read_status($upid);
-    exit($status eq 'OK' ? 0 : -1);
-};
-
 sub setup_environment {
     PVE::RPCEnvironment->setup_default_cli_env();
 }
-- 
2.20.1

[pve-devel] [PATCH manager 5/9] cli tools: correctly handle warnings status

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pveceph.pm | 2 +-
 PVE/CLI/pvenode.pm | 2 +-
 PVE/CLI/vzdump.pm  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
index 2745cc62..c02dc3fe 100755
--- a/PVE/CLI/pveceph.pm
+++ b/PVE/CLI/pveceph.pm
@@ -35,7 +35,7 @@ my $nodename = PVE::INotify::nodename();
 my $upid_exit = sub {
     my $upid = shift;
     my $status = PVE::Tools::upid_read_status($upid);
-    exit($status eq 'OK' ? 0 : -1);
+    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 };
 
 sub setup_environment {
diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
index b05b3e15..acef6c3b 100644
--- a/PVE/CLI/pvenode.pm
+++ b/PVE/CLI/pvenode.pm
@@ -36,7 +36,7 @@ my $upid_exit = sub {
     my $upid = shift;
     my $status = PVE::Tools::upid_read_status($upid);
     print "Task $status\n";
-    exit($status eq 'OK' ? 0 : -1);
+    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 };
 
 sub param_mapping {
diff --git a/PVE/CLI/vzdump.pm b/PVE/CLI/vzdump.pm
index fe5fd049..3f625574 100755
--- a/PVE/CLI/vzdump.pm
+++ b/PVE/CLI/vzdump.pm
@@ -19,7 +19,7 @@ our $cmddef = [ 'PVE::API2::VZDump', 'vzdump', 'vmid', undef,
 		    my $upid = shift;
 		    exit(0) if $upid eq 'OK';
 		    my $status = PVE::Tools::upid_read_status($upid);
-		    exit($status eq 'OK' ? 0 : -1);
+		    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 		}];
 
 1;
-- 
2.20.1

[pve-devel] [PATCH storage 6/9] api: content: correctly handle warnings status for delayed task

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-common is needed.

 PVE/API2/Storage/Content.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 8b0e3de..a510e3b 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -454,7 +454,7 @@ __PACKAGE__->register_method ({
 
 	    if (!$currently_deleting) {
 		my $status = PVE::Tools::upid_read_status($upid);
-		return undef if $status eq 'OK';
+		return undef if !PVE::Tools::upid_status_is_error($status);
 		die $status;
 	    }
 	}
-- 
2.20.1

[pve-devel] [PATCH container 7/9] pct: correctly handle warnings task status

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-common is needed.

 src/PVE/CLI/pct.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 6b63915..b606c62 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -29,7 +29,7 @@ my $nodename = PVE::INotify::nodename();
 my $upid_exit = sub {
     my $upid = shift;
     my $status = PVE::Tools::upid_read_status($upid);
-    exit($status eq 'OK' ? 0 : -1);
+    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 };
 
 sub setup_environment {
-- 
2.20.1

[pve-devel] [PATCH qemu-server 8/9] cli tools: correctly handle warnings task status

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-common is needed.

 PVE/CLI/qm.pm        | 2 +-
 PVE/CLI/qmrestore.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index f8972bd..a8ad571 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -42,7 +42,7 @@ use base qw(PVE::CLIHandler);
 my $upid_exit = sub {
     my $upid = shift;
     my $status = PVE::Tools::upid_read_status($upid);
-    exit($status eq 'OK' ? 0 : -1);
+    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 };
 
 my $nodename = PVE::INotify::nodename();
diff --git a/PVE/CLI/qmrestore.pm b/PVE/CLI/qmrestore.pm
index 7c09814..034233a 100755
--- a/PVE/CLI/qmrestore.pm
+++ b/PVE/CLI/qmrestore.pm
@@ -81,7 +81,7 @@ our $cmddef = [ __PACKAGE__, 'qmrestore', ['archive', 'vmid'], undef,
 		sub {
 		    my $upid = shift;
 		    my $status = PVE::Tools::upid_read_status($upid);
-		    exit($status eq 'OK' ? 0 : -1);
+		    exit(PVE::Tools::upid_status_is_error($status) ? -1 : 0);
 		}];
 
 1;
-- 
2.20.1

[pve-devel] [PATCH qemu-server 9/9] api: update vm: correctly handle warnings status for delayed task

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c56b609..336aa73 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1438,7 +1438,7 @@ my $update_vm_api  = sub {
 
 		if (!$running) {
 		    my $status = PVE::Tools::upid_read_status($upid);
-		    return if $status eq 'OK';
+		    return if !PVE::Tools::upid_status_is_error($status);
 		    die $status;
 		}
 	    }
-- 
2.20.1

[pve-devel] applied: [PATCH common 1/9] tools: add upid_status_is_error function

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> There's also support for ending a task with warnings now, so the logic "status
> not 'OK' means error" does not work anymore.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/Tools.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH qemu-server 9/9] api: update vm: correctly handle warnings status for delayed task

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH qemu-server 8/9] cli tools: correctly handle warnings task status

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-common is needed.
> 
>  PVE/CLI/qm.pm        | 2 +-
>  PVE/CLI/qmrestore.pm | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH container 7/9] pct: correctly handle warnings task status

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-common is needed.
> 
>  src/PVE/CLI/pct.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied this already Monday, but forgot to reply.. any how, thanks!

[pve-devel] applied: [PATCH storage 6/9] api: content: correctly handle warnings status for delayed task

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-common is needed.
> 
>  PVE/API2/Storage/Content.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, but had to resolve a merge conflict due to changes in context, thanks!

[pve-devel] applied: [PATCH manager 5/9] cli tools: correctly handle warnings status

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pveceph.pm | 2 +-
>  PVE/CLI/pvenode.pm | 2 +-
>  PVE/CLI/vzdump.pm  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 4/9] cli: pveam: remove unused private sub

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pveam.pm | 6 ------
>  1 file changed, 6 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 3/9] nodes: startall: correctly handle warning status for delayed task

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH manager 2/9] api/cli: tasks: don't treat warnings status as an error status

[Thomas Lamprecht]

On 12.05.21 14:32, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-common is needed.
> 
>  PVE/API2/Tasks.pm  | 5 ++++-
>  PVE/CLI/pvenode.pm | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
> index 8f6ab603..abae8ba3 100644
> --- a/PVE/API2/Tasks.pm
> +++ b/PVE/API2/Tasks.pm
> @@ -142,7 +142,10 @@ __PACKAGE__->register_method({
>  
>  	    return 1 if $typefilter && $task->{type} ne $typefilter;
>  
> -	    return 1 if $errors && $task->{status} && $task->{status} eq 'OK';
> +	    if ($errors && $task->{status} && !PVE::Tools::upid_status_is_error($task->{status})) {
> +		return 1;
> +	    }
> +
>  	    return 1 if $param->{vmid} && (!$task->{id} || $task->{id} ne $param->{vmid});
>  
>  	    return 1 if $count++ < $start;

Dominik is working on porting-over/implementing the more modern task filtering + UI we did for PBS,
so above hunk will go obsolete anyway so I'm going to skip this one.
Below should still be useful afterwards.

> diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
> index a33fcd93..b05b3e15 100644
> --- a/PVE/CLI/pvenode.pm
> +++ b/PVE/CLI/pvenode.pm
> @@ -181,7 +181,10 @@ our $cmddef = {
>  	    foreach my $task (@$data) {
>  		if (!defined($task->{status})) {
>  		    $task->{status} = 'UNKNOWN';
> -		} elsif ($task->{status} ne 'OK' && $task->{status} ne 'RUNNING') {
> +		# RUNNING is set by the API call and needs to be checked explicitly
> +		} elsif (PVE::Tools::upid_status_is_error($task->{status}) &&
> +		    $task->{status} ne 'RUNNING')
> +		{
>  		    $task->{status} = 'ERROR';
>  		}
>  	    }
>