[pve-devel] [PATCH-SERIES] improve warnings handling in UI and add to PVE

[pve-devel] [PATCH-SERIES] improve warnings handling in UI and add to PVE

[Fabian Ebner]

The first half is just a small UI improvement (but it's all over the place),
replacing 'WARNINGS' with (a language-aware) 'Warnings'.

The later half adds the feature of ending in a WARNINGS state to PVE and
provides a usage example.


proxmox-backup and pve-manager need a dependency bump for proxmox-widget toolkit
pve-container needs a dependency bump for pve-common


proxmox-widget-toolkit:

Fabian Ebner (1):
  tasks: add warningsText

 src/Utils.js      | 1 +
 src/node/Tasks.js | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)


proxmox-backup:

Fabian Ebner (1):
  ui: tasks: use warningsText

 www/panel/Tasks.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


pve-manager:

Fabian Ebner (2):
  ui: cluster task log: eslint fixes
  ui: cluster task log: handle warnings like the node task log does

 www/manager6/dc/Tasks.js | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)


pve-common:

Fabian Ebner (1):
  allow workers to count warnings and finish tasks in a WARNINGS state

 src/PVE/RESTEnvironment.pm | 20 ++++++++++++++++++--
 src/PVE/Tools.pm           |  2 ++
 2 files changed, 20 insertions(+), 2 deletions(-)


pve-container:

Fabian Ebner (1):
  restore: sanitize config: use new warn() function

 src/PVE/LXC/Create.pm | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH widget-toolkit 1/6] tasks: add warningsText

[Fabian Ebner]

to avoid having capitalized 'WARNINGS' (especially since 'Error' is not) and
support different languages.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/Utils.js      | 1 +
 src/node/Tasks.js | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/Utils.js b/src/Utils.js
index af5f1db..229e6f5 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -48,6 +48,7 @@ utilities: {
     noneText: gettext('none'),
     NoneText: gettext('None'),
     errorText: gettext('Error'),
+    warningsText: gettext('Warnings'),
     unknownText: gettext('Unknown'),
     defaultText: gettext('Default'),
     daysText: gettext('days'),
diff --git a/src/node/Tasks.js b/src/node/Tasks.js
index 1f01b07..b01f65e 100644
--- a/src/node/Tasks.js
+++ b/src/node/Tasks.js
@@ -202,8 +202,9 @@ Ext.define('Proxmox.node.Tasks', {
 			switch (parsed) {
 			    case 'unknown': return Proxmox.Utils.unknownText;
 			    case 'error': return Proxmox.Utils.errorText + ': ' + value;
+			    case 'warning': return Proxmox.Utils.warningsText +
+				value.replace('WARNINGS', '');
 			    case 'ok': // fall-through
-			    case 'warning': // fall-through
 			    default: return value;
 			}
 		    },
-- 
2.20.1

[pve-devel] [PATCH proxmox-backup 2/6] ui: tasks: use warningsText

[Fabian Ebner]

to avoid having capitalized 'WARNINGS' (especially since 'Error' is not) and
support different languages.

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

Dependency bump for widget-toolkit is needed.

 www/panel/Tasks.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/www/panel/Tasks.js b/www/panel/Tasks.js
index a194e478..adc82d05 100644
--- a/www/panel/Tasks.js
+++ b/www/panel/Tasks.js
@@ -376,8 +376,9 @@ Ext.define('PBS.node.Tasks', {
 		switch (parsed) {
 		    case 'unknown': return Proxmox.Utils.unknownText;
 		    case 'error': return Proxmox.Utils.errorText + ': ' + value;
+		    case 'warning': return Proxmox.Utils.warningsText +
+			value.replace('WARNINGS', '');
 		    case 'ok': // fall-through
-		    case 'warning': // fall-through
 		    default: return value;
 		}
 	    },
-- 
2.20.1

[pve-devel] [PATCH manager 3/6] ui: cluster task log: eslint fixes

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/dc/Tasks.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/www/manager6/dc/Tasks.js b/www/manager6/dc/Tasks.js
index c244bd2e..c5075dc3 100644
--- a/www/manager6/dc/Tasks.js
+++ b/www/manager6/dc/Tasks.js
@@ -82,9 +82,9 @@ Ext.define('PVE.dc.Tasks', {
 		    width: 150,
 		    renderer: function(value, metaData, record) {
 			if (record.data.pid) {
-			    if (record.data.type == "vncproxy" ||
-				record.data.type == "vncshell" ||
-				record.data.type == "spiceproxy") {
+			    if (record.data.type === "vncproxy" ||
+				record.data.type === "vncshell" ||
+				record.data.type === "spiceproxy") {
 				metaData.tdCls = "x-grid-row-console";
 			    } else {
 				metaData.tdCls = "x-grid-row-loading";
@@ -117,7 +117,7 @@ Ext.define('PVE.dc.Tasks', {
 		    width: 200,
 		    renderer: function(value, metaData, record) {
 			if (record.data.pid) {
-			    if (record.data.type != "vncproxy") {
+			    if (record.data.type !== "vncproxy") {
 				metaData.tdCls = "x-grid-row-loading";
 			    }
 			    return "";
-- 
2.20.1

[pve-devel] applied: [PATCH manager 3/6] ui: cluster task log: eslint fixes

[Dominik Csapak]

applied, thanks

[pve-devel] [PATCH manager 4/6] ui: cluster task log: handle warnings like the node task log does

[Fabian Ebner]

Copied the relevant code from widget-toolkit.

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

Dependency bump for widget-toolkit is needed.

 www/manager6/dc/Tasks.js | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/www/manager6/dc/Tasks.js b/www/manager6/dc/Tasks.js
index c5075dc3..ddf15642 100644
--- a/www/manager6/dc/Tasks.js
+++ b/www/manager6/dc/Tasks.js
@@ -61,9 +61,15 @@ Ext.define('PVE.dc.Tasks', {
 		getRowClass: function(record, index) {
 		    var status = record.get('status');
 
-		    if (status && status != 'OK') {
-			return "proxmox-invalid-row";
+		    if (status) {
+			let parsed = Proxmox.Utils.parse_task_status(status);
+			if (parsed === 'error') {
+			    return "proxmox-invalid-row";
+			} else if (parsed === 'warning') {
+			    return "proxmox-warning-row";
+			}
 		    }
+		    return '';
 		},
 	    },
 	    sortableColumns: false,
@@ -122,11 +128,16 @@ Ext.define('PVE.dc.Tasks', {
 			    }
 			    return "";
 			}
-			if (value == 'OK') {
-			    return 'OK';
+
+			let parsed = Proxmox.Utils.parse_task_status(value);
+			switch (parsed) {
+			    case 'unknown': return Proxmox.Utils.unknownText;
+			    case 'error': return Proxmox.Utils.errorText + ': ' + value;
+			    case 'warning': return Proxmox.Utils.warningsText +
+				value.replace('WARNINGS', '');
+			    case 'ok': // fall-through
+			    default: return value;
 			}
-			// metaData.attr = 'style="color:red;"';
-			return Proxmox.Utils.errorText + ': ' + value;
 		    },
 		},
 	    ],
-- 
2.20.1

[pve-devel] [PATCH/RFC common 5/6] allow workers to count warnings and finish tasks in a WARNINGS state

[Fabian Ebner]

as is already supported by the UI (and PBS).

A nice bonus is that warn() can be used by both workers and non-workers, while
for workers the output is redirected/duplicated as set up by {fork,tee}_worker()
and non-erroring workers that issued a warning will end in a WARNINGS state.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

@Thomas: Hope this isn't too far from what you had in mind.

I'm not too familiar with task status related code, so hopefully I haven't
missed something important.

Also not sure if the newline should be included or not within warn().

 src/PVE/RESTEnvironment.pm | 20 ++++++++++++++++++--
 src/PVE/Tools.pm           |  2 ++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
index d5b84d0..1bdcc34 100644
--- a/src/PVE/RESTEnvironment.pm
+++ b/src/PVE/RESTEnvironment.pm
@@ -115,7 +115,10 @@ sub init {
     # priv ... access from private server (pvedaemon)
     # ha   ... access from HA resource manager agent (pve-ha-manager)
 
-    my $self = { type => $type };
+    my $self = {
+	type => $type,
+	warning_count => 0,
+    };
 
     bless $self, $class;
 
@@ -448,7 +451,6 @@ my $tee_worker = sub {
 	    }
 	}
 
-	# get status (error or OK)
 	POSIX::read($ctrlfd, $readbuf, 4096);
 	if ($readbuf =~ m/^TASK OK\n?$/) {
 	    # skip printing to stdout
@@ -456,6 +458,9 @@ my $tee_worker = sub {
 	} elsif ($readbuf =~ m/^TASK ERROR: (.*)\n?$/) {
 	    print STDERR "$1\n";
 	    print $taskfh "\n$readbuf"; # ensure start on new line for webUI
+	} elsif ($readbuf =~ m/^TASK WARNINGS: (\d+)\n?$/) {
+	    print STDERR "Task finished with $1 warning(s)!\n";
+	    print $taskfh "\n$readbuf"; # ensure start on new line for webUI
 	} else {
 	    die "got unexpected control message: $readbuf\n";
 	}
@@ -617,6 +622,9 @@ sub fork_worker {
 	    syslog('err', $err);
 	    $msg = "TASK ERROR: $err\n";
 	    $exitcode = -1;
+	} elsif (my $warnings = $self->{warning_count}) {
+	    $msg = "TASK WARNINGS: $warnings\n";
+	    $exitcode = 0;
 	} else {
 	    $msg = "TASK OK\n";
 	    $exitcode = 0;
@@ -703,6 +711,14 @@ sub fork_worker {
     return wantarray ? ($upid, $res) : $upid;
 }
 
+sub warn {
+    my ($self, $message) = @_;
+
+    print STDERR "WARN: $message\n";
+
+    $self->{warning_count}++;
+}
+
 # Abstract function
 
 sub log_cluster_msg {
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index fc4a367..b087c39 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1156,6 +1156,8 @@ sub upid_read_status {
 	    return 'OK';
 	} elsif ($line =~ m/^TASK ERROR: (.+)$/) {
 	    return $1;
+	} elsif ($line =~ m/^TASK (WARNINGS: \d+)$/) {
+	    return $1;
 	} else {
 	    return "unexpected status";
 	}
-- 
2.20.1

[pve-devel] [PATCH/RFC container 6/6] restore: sanitize config: use new warn() function

[Fabian Ebner]

to make it more visible when the task finished with warnings.

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

Dependency bump for pve-common is needed.

 src/PVE/LXC/Create.pm | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index 82d7ad9..67b4c7a 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -6,6 +6,7 @@ use File::Basename;
 use File::Path;
 use Fcntl;
 
+use PVE::RPCEnvironment;
 use PVE::Storage::PBSPlugin;
 use PVE::Storage;
 use PVE::DataCenterConfig;
@@ -306,6 +307,8 @@ sub restore_configuration_from_proxmox_backup {
 sub sanitize_and_merge_config {
     my ($conf, $oldconf, $restricted, $unique) = @_;
 
+    my $rpcenv = PVE::RPCEnvironment::get();
+
     foreach my $key (keys %$oldconf) {
 	next if $key eq 'digest' || $key eq 'rootfs' || $key eq 'snapshots' || $key eq 'unprivileged' || $key eq 'parent';
 	next if $key =~ /^mp\d+$/; # don't recover mountpoints
@@ -316,12 +319,16 @@ sub sanitize_and_merge_config {
 
 	if ($key eq 'lxc' && $restricted) {
 	    my $lxc_list = $oldconf->{'lxc'};
-	    warn "skipping custom lxc options, restore manually as root:\n";
-	    warn "--------------------------------\n";
+
+	    my $msg = "skipping custom lxc options, restore manually as root:\n";
+	    $msg .= "--------------------------------\n";
 	    foreach my $lxc_opt (@$lxc_list) {
-		warn "$lxc_opt->[0]: $lxc_opt->[1]\n"
+		$msg .= "$lxc_opt->[0]: $lxc_opt->[1]\n"
 	    }
-	    warn "--------------------------------\n";
+	    $msg .= "--------------------------------";
+
+	    $rpcenv->warn($msg);
+
 	    next;
 	}
 
-- 
2.20.1

Re: [pve-devel] [pbs-devel] [PATCH-SERIES] improve warnings handling in UI and add to PVE

[Dominik Csapak]

series looks good to me, the only thing that might be not ideal
is the value.replace('WARNINGS', '') call. it seems very brittle
though in reality it will probably not be a problem

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>

Re: [pve-devel] [pbs-devel] [PATCH-SERIES] improve warnings handling in UI and add to PVE

[Fabian Ebner]

Am 16.03.21 um 15:02 schrieb Dominik Csapak:
> series looks good to me, the only thing that might be not ideal
> is the value.replace('WARNINGS', '') call. it seems very brittle
> though in reality it will probably not be a problem
> 
> Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>
> 

Thanks for the review. I'm also not 100% happy with the replace. I 
didn't see a better alternative at the time, but maybe I do now:

We need to keep backwards compatibility for the existing callers of 
parse_task_status(), so also returning the number of warnings there 
doesn't seem to work. Instead, we could add a new 
parse_task_status_full() that returns both the status *and* the text to 
be displayed.

A better version of the replace would also be possible:
     value.replace('WARNINGS', Proxmox.Utils.warningsText);

If one of those sounds sensible, I'll send a v2.