[pve-devel] [PATCH v2 common 0/1] fix #1454: ARC in Node Summary

[pve-devel] [PATCH v2 common 0/1] fix #1454: ARC in Node Summary

[Matthias Heiserer]

I've dropped the patches for storing arcsize in RRD. I think it would be a neat
improvement, but showing ARC in RAM bar works without just as well.

That means the progress bar shows RAM and ARC separately, while the 
Memory Usage graph displays RAM + ARC combined.

Matthias Heiserer (1):
  meminfo: also return arcsize

 src/PVE/ProcFSTools.pm | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH v2 common 1/1] fix #1454: meminfo: also return arcsize

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
Changes from v1:
None

 src/PVE/ProcFSTools.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/ProcFSTools.pm b/src/PVE/ProcFSTools.pm
index eb4b0f5..0686c34 100644
--- a/src/PVE/ProcFSTools.pm
+++ b/src/PVE/ProcFSTools.pm
@@ -283,6 +283,7 @@ sub read_meminfo {
 	swaptotal => 0,
 	swapfree => 0,
 	swapused => 0,
+	arcsize => 0,
     };
 
     my $fh = IO::File->new ("/proc/meminfo", "r");
@@ -307,6 +308,11 @@ sub read_meminfo {
     my $spages = PVE::Tools::file_read_firstline("/sys/kernel/mm/ksm/pages_sharing") // 0 ;
     $res->{memshared} = int($spages) * 4096;
 
+    my $arcstats = PVE::Tools::file_get_contents("/proc/spl/kstat/zfs/arcstats");
+    if ($arcstats && $arcstats =~ m/size\s+\d+\s+(\d+)/m) {
+	$res->{arcsize} = int ($1);
+    }
+
     return $res;
 }
 
-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/2] fix #1454: node/status: return arc size

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v1:
No Changes

 PVE/API2/Nodes.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index bfe5c40a..0795f06d 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -389,6 +389,7 @@ __PACKAGE__->register_method({
 	    free => $meminfo->{memfree},
 	    total => $meminfo->{memtotal},
 	    used => $meminfo->{memused},
+	    arcsize => $meminfo->{arcsize},
 	};
 
 	$res->{ksm} = {
-- 
2.30.2

[pve-devel] [PATCH v2 widget-toolkit] fix #1454: InfoWidget for Memory

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

I'm a bit unsure about the color. If clashes a bit with the red when the
RAM is near full in light mode. Open for better suggestions, but
should work for now.

changes from v1:
ignore arcsize when not set
separate progress/memory widget:
override rendertpl and dont inject second bar

 src/Makefile                  |  1 +
 src/css/ext6-pmx.css          |  6 ++++
 src/panel/NodeMemoryWidget.js | 54 +++++++++++++++++++++++++++++++++++
 src/panel/StatusView.js       |  2 +-
 4 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 src/panel/NodeMemoryWidget.js

diff --git a/src/Makefile b/src/Makefile
index 30e8fd5..a10f019 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -55,6 +55,7 @@ JSSRC=					\
 	panel/EOLNotice.js		\
 	panel/InputPanel.js		\
 	panel/InfoWidget.js		\
+	panel/NodeMemoryWidget.js	\
 	panel/LogView.js		\
 	panel/NodeInfoRepoStatus.js	\
 	panel/JournalView.js		\
diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
index 2ffd2a8..8d2c3ef 100644
--- a/src/css/ext6-pmx.css
+++ b/src/css/ext6-pmx.css
@@ -348,3 +348,9 @@ div.right-aligned {
     color: #555;
 }
 /* action column fix end */
+
+.zfs-arc {
+	background-color: #c976b7;
+	color: #c976b7;
+	height: 100%;
+}
diff --git a/src/panel/NodeMemoryWidget.js b/src/panel/NodeMemoryWidget.js
new file mode 100644
index 0000000..e7619fd
--- /dev/null
+++ b/src/panel/NodeMemoryWidget.js
@@ -0,0 +1,54 @@
+Ext.define('Proxmox.panel.ArcProgress', {
+    extend: 'Ext.ProgressBar',
+    alias: 'widget.pmxArcProgress',
+
+   childEls: [
+	'arcbar',
+   ],
+
+    // modified from https://docs.sencha.com/extjs/7.0.0/classic/src/ProgressBar.js.html
+    renderTpl: [
+	`<div id="{id}-arcbar" data-ref="arcbar"
+	    style='margin-right: 100%; width: auto;'
+	    class='zfs-arc'>
+	</div>`,
+	'<div id="{id}-bar" data-ref="bar" class="{baseCls}-bar {baseCls}-bar-{ui}" role="presentation" style="width:{percentage}%">',
+	    '<tpl if="internalText">',
+		'<div class="{baseCls}-text" role="presentation">',
+		    '<div role="presentation">{text}</div>',
+		'</div>',
+	    '</tpl>',
+	'</div>',
+    ],
+
+    updateArc: function(width) {
+	this.arcbar.setStyle('margin-right', `${width}%`);
+    },
+
+    initComponent: function() {
+	this.callParent();
+    },
+});
+
+
+Ext.define('Proxmox.widget.NodeMemory', {
+    extend: 'Proxmox.widget.Info',
+    alias: 'widget.pmxNodeMemoryWidget',
+
+    updateValue: function(text, usage, mem) {
+	let me = this;
+
+	if (mem) {
+	    usage = (mem.used - (mem.arcsize || 0)) / mem.total;
+	    me.getComponent("progress").updateArc((mem.free / mem.total) * 100);
+	    me.callParent([text, usage]);
+	}
+    },
+
+    initComponent: function() {
+	let me = this;
+	me.items.filter(i => i.xtype === 'progressbar')
+	    .forEach(i => { i.xtype = 'pmxArcProgress'; });
+	me.callParent();
+    },
+});
diff --git a/src/panel/StatusView.js b/src/panel/StatusView.js
index e2e81e2..7258f36 100644
--- a/src/panel/StatusView.js
+++ b/src/panel/StatusView.js
@@ -72,7 +72,7 @@ Ext.define('Proxmox.panel.StatusView', {
 	    if (Ext.isFunction(field.calculate)) {
 		calculate = field.calculate;
 	    }
-	    field.updateValue(renderer.call(field, used, max), calculate(used, max));
+	    field.updateValue(renderer.call(field, used, max), calculate(used, max), used);
 	}
     },
 
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/2] fix #1454: ui: node summary: show arc alongside RAM

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v1:
reword title so it's clear it includes ARC

 www/manager6/node/StatusView.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/www/manager6/node/StatusView.js b/www/manager6/node/StatusView.js
index d34724f7..7c67cab2 100644
--- a/www/manager6/node/StatusView.js
+++ b/www/manager6/node/StatusView.js
@@ -49,9 +49,10 @@ Ext.define('PVE.node.StatusView', {
 	    padding: '0 0 20 0',
 	},
 	{
+	    xtype: 'pmxNodeMemoryWidget',
 	    iconCls: 'fa fa-fw pmx-itype-icon-memory pmx-icon',
 	    itemId: 'memory',
-	    title: gettext('RAM usage'),
+	    title: gettext('RAM usage') + ", " + gettext("ZFS ARC"),
 	    valueField: 'memory',
 	    maxField: 'memory',
 	    renderer: Proxmox.Utils.render_node_size_usage,
-- 
2.30.2

Re: [pve-devel] [PATCH v2 common 0/1] fix #1454: ARC in Node Summary

[Max Carrara]

Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>

Looks pretty good; I'm not 100% sure about the ARC bar's dark theme
colour, but I like the light theme's a lot. Maybe somebody else could
chime in with other suggestions.

I also checked again whether the ARC bar remains visible if the RAM
approaches critical levels (and consequently turns yellow/red), as we
already had done off-list, just in case.

Re: [pve-devel] [PATCH v2 common 1/1] fix #1454: meminfo: also return arcsize

[Thomas Lamprecht]

Am 28/03/2023 um 14:49 schrieb Matthias Heiserer:
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> Changes from v1:
> None
> 
>  src/PVE/ProcFSTools.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/ProcFSTools.pm b/src/PVE/ProcFSTools.pm
> index eb4b0f5..0686c34 100644
> --- a/src/PVE/ProcFSTools.pm
> +++ b/src/PVE/ProcFSTools.pm
> @@ -283,6 +283,7 @@ sub read_meminfo {
>  	swaptotal => 0,
>  	swapfree => 0,
>  	swapused => 0,
> +	arcsize => 0,
>      };
>  
>      my $fh = IO::File->new ("/proc/meminfo", "r");
> @@ -307,6 +308,11 @@ sub read_meminfo {
>      my $spages = PVE::Tools::file_read_firstline("/sys/kernel/mm/ksm/pages_sharing") // 0 ;
>      $res->{memshared} = int($spages) * 4096;
>  
> +    my $arcstats = PVE::Tools::file_get_contents("/proc/spl/kstat/zfs/arcstats");

file_get_contents dies if the file isn't there, which might happen e.g., on PMG or if
one disables ZFS – why ever that would be.

so rather do:

my $arcstats = eval { PVE::Tools::file_get_contents("/proc/spl/kstat/zfs/arcstats") };

(just ignore the error, alternatively we could set a flag on ENOENT to avoid querying that
the next time, but that would be just an optimization).

> +    if ($arcstats && $arcstats =~ m/size\s+\d+\s+(\d+)/m) {

this match is rather brittle as there are multiple lines with size, e.g:

grep -P 'size\s+\d+\s+(\d+)' /proc/spl/kstat/zfs/arcstats
size                            4    5356816
compressed_size                 4    221696
uncompressed_size               4    5079552
overhead_size                   4    4947968
hdr_size                        4    29520
data_size                       4    0
metadata_size                   4    5169664
dbuf_size                       4    38016
dnode_size                      4    93248
bonus_size                      4    14080
anon_size                       4    0
mru_size                        4    2696704
mru_ghost_size                  4    0
mfu_size                        4    2472960
mfu_ghost_size                  4    0
l2_prefetch_asize               4    0
l2_mru_asize                    4    0
l2_mfu_asize                    4    0
l2_bufc_data_asize              4    0
l2_bufc_metadata_asize          4    0
l2_size                         4    0
l2_asize                        4    0
l2_hdr_size                     4    0
l2_log_blk_avg_asize            4    0
l2_log_blk_asize                4    0
l2_rebuild_size                 4    0
l2_rebuild_asize                4    0
arc_raw_size                    4    0
abd_chunk_waste_size            4    12288

so it hard depends on ZFS never changing the existing order or adding another field that
matches before the actual size one. So please add start/end anchors, e.g.:

if ($arcstats && $arcstats =~ m/^size\s+\d+\s+(\d+)$/m) {


> +	$res->{arcsize} = int ($1);

nit: extra whitespace after `int` and before the parenthesis.

> +    }
> +
>      return $res;
>  }
>  

Re: [pve-devel] [PATCH v2 widget-toolkit] fix #1454: InfoWidget for Memory

[Stefan Sterz]

On 28.03.23 14:49, Matthias Heiserer wrote:
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> 
> I'm a bit unsure about the color. If clashes a bit with the red when the
> RAM is near full in light mode. Open for better suggestions, but
> should work for now.
> 
> changes from v1:
> ignore arcsize when not set
> separate progress/memory widget:
> override rendertpl and dont inject second bar
> 
>  src/Makefile                  |  1 +
>  src/css/ext6-pmx.css          |  6 ++++
>  src/panel/NodeMemoryWidget.js | 54 +++++++++++++++++++++++++++++++++++
>  src/panel/StatusView.js       |  2 +-
>  4 files changed, 62 insertions(+), 1 deletion(-)
>  create mode 100644 src/panel/NodeMemoryWidget.js
> 
> diff --git a/src/Makefile b/src/Makefile
> index 30e8fd5..a10f019 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -55,6 +55,7 @@ JSSRC=					\
>  	panel/EOLNotice.js		\
>  	panel/InputPanel.js		\
>  	panel/InfoWidget.js		\
> +	panel/NodeMemoryWidget.js	\
>  	panel/LogView.js		\
>  	panel/NodeInfoRepoStatus.js	\
>  	panel/JournalView.js		\
> diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
> index 2ffd2a8..8d2c3ef 100644
> --- a/src/css/ext6-pmx.css
> +++ b/src/css/ext6-pmx.css
> @@ -348,3 +348,9 @@ div.right-aligned {
>      color: #555;
>  }
>  /* action column fix end */
> +
> +.zfs-arc {
> +	background-color: #c976b7;
> +	color: #c976b7;

thanks for this, it will surely help user's better understand the ram
usage chart. i'd have two suggestions:

1) can we start using hsl for color going forward? the dark theme
already does that, and it helps us stay more consistent (e.g., if you
want a dark/lighter version of a color, that is much easier in hsl, in
rgb it's very easy to accidentally get a different hue etc.)

2) i think it would also be good to start using css variables in
general. if we ever do need to change these colors in a theme, it's much
easier to change a variable, then to have to override each css style.

in regard to the general color choice: yeah this purple is not ideal.
maybe you could use something that is closer to the blue in hue.
currently, the two colors used have these two closest hsl equivalents,
blue: hsl(206deg, 65%, 85%) red: hsl(360deg, 100%, 77%). so, potentially
hsl(280deg, 82.5%, 60%) would work. but im not to sure either.

purple in other contexts is used as a more intense version of red (e.g.,
heat maps) so perhaps we should think of a different hue altogether.

however, putting this all together would yield:

:root {
	--pwt-zfs-arc: hsl(280deg, 82.5%, 60%);
}

.zfs-arc {
	background-color: var(--pwt-zfs-arc);
	color: var(--pwt-zfs-arc);
}

then in the dark theme we could simply do (this color is imo more
consistent with the dark theme chart/gauge colors):

:root {
	 --pwt-zfs-arc: hsl(280deg, 100%, 40.5%);
}



> +	height: 100%;
> +}
> diff --git a/src/panel/NodeMemoryWidget.js b/src/panel/NodeMemoryWidget.js
> new file mode 100644
> index 0000000..e7619fd
> --- /dev/null
> +++ b/src/panel/NodeMemoryWidget.js
> @@ -0,0 +1,54 @@
> +Ext.define('Proxmox.panel.ArcProgress', {
> +    extend: 'Ext.ProgressBar',
> +    alias: 'widget.pmxArcProgress',
> +
> +   childEls: [
> +	'arcbar',
> +   ],
> +
> +    // modified from https://docs.sencha.com/extjs/7.0.0/classic/src/ProgressBar.js.html
> +    renderTpl: [
> +	`<div id="{id}-arcbar" data-ref="arcbar"
> +	    style='margin-right: 100%; width: auto;'
> +	    class='zfs-arc'>
> +	</div>`,
> +	'<div id="{id}-bar" data-ref="bar" class="{baseCls}-bar {baseCls}-bar-{ui}" role="presentation" style="width:{percentage}%">',
> +	    '<tpl if="internalText">',
> +		'<div class="{baseCls}-text" role="presentation">',
> +		    '<div role="presentation">{text}</div>',
> +		'</div>',
> +	    '</tpl>',
> +	'</div>',
> +    ],
> +
> +    updateArc: function(width) {
> +	this.arcbar.setStyle('margin-right', `${width}%`);
> +    },
> +
> +    initComponent: function() {
> +	this.callParent();
> +    },
> +});
> +
> +
> +Ext.define('Proxmox.widget.NodeMemory', {
> +    extend: 'Proxmox.widget.Info',
> +    alias: 'widget.pmxNodeMemoryWidget',
> +
> +    updateValue: function(text, usage, mem) {
> +	let me = this;
> +
> +	if (mem) {
> +	    usage = (mem.used - (mem.arcsize || 0)) / mem.total;
> +	    me.getComponent("progress").updateArc((mem.free / mem.total) * 100);
> +	    me.callParent([text, usage]);
> +	}
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +	me.items.filter(i => i.xtype === 'progressbar')
> +	    .forEach(i => { i.xtype = 'pmxArcProgress'; });
> +	me.callParent();
> +    },
> +});
> diff --git a/src/panel/StatusView.js b/src/panel/StatusView.js
> index e2e81e2..7258f36 100644
> --- a/src/panel/StatusView.js
> +++ b/src/panel/StatusView.js
> @@ -72,7 +72,7 @@ Ext.define('Proxmox.panel.StatusView', {
>  	    if (Ext.isFunction(field.calculate)) {
>  		calculate = field.calculate;
>  	    }
> -	    field.updateValue(renderer.call(field, used, max), calculate(used, max));
> +	    field.updateValue(renderer.call(field, used, max), calculate(used, max), used);
>  	}
>      },
>