[pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI

[pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI

[Max Carrara]

Ceph Build Commit in UI - Version 1
===================================

This series adds Ceph's build commit to the UI and lets the user know if
a service is running an outdated build and therefore ought to be
restarted.

The build commit is now displayed next to the version for all Ceph
services like so:

  18.2.2 (abcd1234)

Should a service run an outdated build, the new build commit is also
displayed:

  18.2.2 (abcd1234 -> 5678fedc)

(Icons are omitted here).

See the individual patches for more in-depth information.

Additionally, some of the code was also cleaned up and refactored a
little along the way.

I'm not 100% sure if the design I've opted for here is the best, so it
would be great to get some opinions on this. The OSD tree/list view
especially can get a little noisy if there are a lot of outdated OSDs
running.

Summary of Changes
------------------

Max Carrara (8):
  ceph: tools: refactor installation check as guard clause
  ceph: tools: update Ceph version regex
  ceph: services: remove old cluster broadcast
  ceph: services: refactor version existence check as guard clause
  utils: align regex of parse_ceph_version with Perl equivalent
  ui: ceph: services: parse and display build commit
  api: ceph: add build commit of host to Ceph osd index endpoint data
  ui: ceph: osd: rework rendering of version field & show build commit

 PVE/API2/Ceph/OSD.pm             |  1 +
 PVE/Ceph/Services.pm             | 38 +++++++++++-----------
 PVE/Ceph/Tools.pm                | 48 +++++++++++++++++-----------
 www/manager6/Utils.js            | 17 +++++++++-
 www/manager6/ceph/OSD.js         | 55 ++++++++++++++++++++++++++------
 www/manager6/ceph/ServiceList.js | 34 +++++++++++++++-----
 www/manager6/ceph/Services.js    | 14 +++++++-
 7 files changed, 149 insertions(+), 58 deletions(-)

-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 1/8] ceph: tools: refactor installation check as guard clause

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/Ceph/Tools.pm | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 9b97171e..087c4ef3 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -57,24 +57,25 @@ my $config_files = {
 sub get_local_version {
     my ($noerr) = @_;
 
-    if (check_ceph_installed('ceph_bin', $noerr)) {
-	my $ceph_version;
-	run_command(
-	    [ $ceph_service->{ceph_bin}, '--version' ],
-	    noerr => $noerr,
-	    outfunc => sub { $ceph_version = shift if !defined $ceph_version },
-	);
-	return undef if !defined $ceph_version;
-
-	if ($ceph_version =~ /^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
-	    my ($version, $buildcommit) = ($1, $2);
-	    my $subversions = [ split(/\.|-/, $version) ];
-
-	    # return (version, buildid, major, minor, ...) : major;
-	    return wantarray
-		? ($version, $buildcommit, $subversions)
-		: $subversions->[0];
-	}
+    return undef if !check_ceph_installed('ceph_bin', $noerr);
+
+    my $ceph_version;
+    run_command(
+	[ $ceph_service->{ceph_bin}, '--version' ],
+	noerr => $noerr,
+	outfunc => sub { $ceph_version = shift if !defined $ceph_version },
+    );
+
+    return undef if !defined $ceph_version;
+
+    if ($ceph_version =~ /^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
+	my ($version, $buildcommit) = ($1, $2);
+	my $subversions = [ split(/\.|-/, $version) ];
+
+	# return (version, buildid, major, minor, ...) : major;
+	return wantarray
+	    ? ($version, $buildcommit, $subversions)
+	    : $subversions->[0];
     }
 
     return undef;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 2/8] ceph: tools: update Ceph version regex

[Max Carrara]

Make the regex more maintainable declaring it as a variable, breaking it
up and commenting it by using the x flag.

Also remove the part that parses our Debian revision (e.g. -pve1) from
the version, as we do not actually include that in our Ceph builds.

The part of the regex that parses the build commit hash is made
mandatory (remove '?' after its group).

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/Ceph/Tools.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 087c4ef3..a00d23e1 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -68,7 +68,18 @@ sub get_local_version {
 
     return undef if !defined $ceph_version;
 
-    if ($ceph_version =~ /^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
+    my $re_ceph_version = qr/
+	# Skip ahead to the version, which may optionally start with 'v'
+	^ceph.*\sv?
+
+	# Parse the version X.Y, X.Y.Z, etc.
+	( \d+ (?:\.\d+)+ ) \s+
+
+	# Parse the git commit hash between parentheses
+	(?: \( ([a-zA-Z0-9]+) \) )
+    /x;
+
+    if ($ceph_version =~ /$re_ceph_version/) {
 	my ($version, $buildcommit) = ($1, $2);
 	my $subversions = [ split(/\.|-/, $version) ];
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 3/8] ceph: services: remove old cluster broadcast

[Max Carrara]

The `ceph-version` key is not used anymore, so it can go.

Double-checked by `rg`ing through all of our repositories.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/Ceph/Services.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
index e0f31e8e..f5109655 100644
--- a/PVE/Ceph/Services.pm
+++ b/PVE/Ceph/Services.pm
@@ -60,8 +60,6 @@ sub broadcast_ceph_versions {
 		return; # up to date, nothing to do so avoid (not exactly cheap) broadcast
 	    }
 	}
-	# FIXME: remove with 8.0 (or 7.2, its not _that_ bad) - for backward compat only
-	PVE::Cluster::broadcast_node_kv("ceph-version", $version);
 
 	my $node_versions = {
 	    version => {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 4/8] ceph: services: refactor version existence check as guard clause

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/Ceph/Services.pm | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
index f5109655..0b8c207e 100644
--- a/PVE/Ceph/Services.pm
+++ b/PVE/Ceph/Services.pm
@@ -50,26 +50,26 @@ sub broadcast_ceph_services {
 sub broadcast_ceph_versions {
     my ($version, $buildcommit, $vers_parts) = PVE::Ceph::Tools::get_local_version(1);
 
-    if ($version) {
-	my $nodename = PVE::INotify::nodename();
-	my $old = PVE::Cluster::get_node_kv("ceph-versions", $nodename);
-	if (defined($old->{$nodename})) {
-	    $old = eval { decode_json($old->{$nodename}) };
-	    warn $@ if $@; # should not happen
-	    if (defined($old) && $old->{buildcommit} eq $buildcommit && $old->{version}->{str} eq $version) {
-		return; # up to date, nothing to do so avoid (not exactly cheap) broadcast
-	    }
+    return undef if !$version;
+
+    my $nodename = PVE::INotify::nodename();
+    my $old = PVE::Cluster::get_node_kv("ceph-versions", $nodename);
+    if (defined($old->{$nodename})) {
+	$old = eval { decode_json($old->{$nodename}) };
+	warn $@ if $@; # should not happen
+	if (defined($old) && $old->{buildcommit} eq $buildcommit && $old->{version}->{str} eq $version) {
+	    return; # up to date, nothing to do so avoid (not exactly cheap) broadcast
 	}
-
-	my $node_versions = {
-	    version => {
-		str => $version,
-		parts => $vers_parts,
-	    },
-	    buildcommit => $buildcommit,
-	};
-	PVE::Cluster::broadcast_node_kv("ceph-versions", encode_json($node_versions));
     }
+
+    my $node_versions = {
+	version => {
+	    str => $version,
+	    parts => $vers_parts,
+	},
+	buildcommit => $buildcommit,
+    };
+    PVE::Cluster::broadcast_node_kv("ceph-versions", encode_json($node_versions));
 }
 
 sub get_ceph_versions {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 5/8] utils: align regex of parse_ceph_version with Perl equivalent

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
"Unfortunately" JS doesn't support comments in its regexes akin to
Perl's 'x' flag, but oh well ...

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

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index f5608944..74e46694 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -118,7 +118,8 @@ Ext.define('PVE.Utils', {
 	}
 
 	if (service.ceph_version) {
-	    var match = service.ceph_version.match(/version (\d+(\.\d+)*)/);
+	    // See PVE/Ceph/Tools.pm - get_local_version
+	    const match = service.ceph_version.match(/^ceph.*\sv?(\d+(?:\.\d+)+)/);
 	    if (match) {
 		return match[1];
 	    }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 6/8] ui: ceph: services: parse and display build commit

[Max Carrara]

This commit adds `PVE.Utils.parse_ceph_buildcommit`, which can be used
to get the full hash "eccf199d..." in parentheses from a string like
the following:

  ceph version 17.2.7 (eccf199d63457659c09677399928203b7903c888) quincy (stable)

This hash is displayed and taken into account when comparing monitor
and manager versions in the client. Specifically, the shortened build
commit is now displayed in parentheses next to the version for both
monitors and managers like so:

  18.2.2 (abcd1234)

Should the build commit of the running service differ from the one
that's installed on the host, the newer build commit will also be
shown in parentheses:

  18.2.2 (abcd1234 -> 5678fedc)

The icon displayed for running a service with an outdated build is the
same as for running an outdated version. The conditional display of
cluster health-related icons remains the same otherwise.

The Ceph summary view also displays the hash and will show a warning
if a service is running with a build commit that doesn't match the one
that's advertised by the host.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 www/manager6/Utils.js            | 14 ++++++++++++++
 www/manager6/ceph/ServiceList.js | 28 ++++++++++++++++++++++------
 www/manager6/ceph/Services.js    | 14 +++++++++++++-
 3 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 74e46694..435c79d7 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
 	return undefined;
     },
 
+    parse_ceph_buildcommit: function(service) {
+	if (service.ceph_version) {
+	    // See PVE/Ceph/Tools.pm - get_local_version
+	    const match = service.ceph_version.match(
+		/^ceph.*\sv?(?:\d+(?:\.\d+)+)\s+(?:\(([a-zA-Z0-9]+)\))/,
+	    );
+	    if (match) {
+		return match[1];
+	    }
+	}
+
+	return undefined;
+    },
+
     compare_ceph_versions: function(a, b) {
 	let avers = [];
 	let bvers = [];
diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
index 76710146..30f455ed 100644
--- a/www/manager6/ceph/ServiceList.js
+++ b/www/manager6/ceph/ServiceList.js
@@ -102,21 +102,37 @@ Ext.define('PVE.node.CephServiceController', {
 	if (value === undefined) {
 	    return '';
 	}
+
+	const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
+
 	let view = this.getView();
-	let host = rec.data.host, nodev = [0];
+	const host = rec.data.host;
+
+	let versionNode = [0];
+	let bcNode = '';
 	if (view.nodeversions[host] !== undefined) {
-	    nodev = view.nodeversions[host].version.parts;
+	    versionNode = view.nodeversions[host].version.parts;
+	    bcNode = view.nodeversions[host].buildcommit;
 	}
 
+	let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
 	let icon = '';
-	if (PVE.Utils.compare_ceph_versions(view.maxversion, nodev) > 0) {
+	if (PVE.Utils.compare_ceph_versions(view.maxversion, versionNode) > 0) {
 	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
-	} else if (PVE.Utils.compare_ceph_versions(nodev, value) > 0) {
+	} else if (PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
 	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
-	} else if (view.mixedversions) {
+	} else if (view.mixedversions && !bcChanged) {
 	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
 	}
-	return icon + value;
+
+	let bcPart = bc.substring(0, 9);
+	if (bcChanged) {
+	    const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
+	    icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+	    bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
+	}
+
+	return `${icon}${value} (${bcPart})`;
     },
 
     getMaxVersions: function(store, records, success) {
diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
index b9fc52c8..410b28bf 100644
--- a/www/manager6/ceph/Services.js
+++ b/www/manager6/ceph/Services.js
@@ -155,6 +155,7 @@ Ext.define('PVE.ceph.Services', {
 		    title: metadata[type][id].name || name,
 		    host: host,
 		    version: PVE.Utils.parse_ceph_version(metadata[type][id]),
+		    buildcommit: PVE.Utils.parse_ceph_buildcommit(metadata[type][id]),
 		    service: metadata[type][id].service,
 		    addr: metadata[type][id].addr || metadata[type][id].addrs || Proxmox.Utils.unknownText,
 		};
@@ -181,7 +182,10 @@ Ext.define('PVE.ceph.Services', {
 		}
 
 		if (result.version) {
-		    result.statuses.push(gettext('Version') + ": " + result.version);
+		    const host_buildcommit = metadata.node[host]?.buildcommit || "";
+
+		    const buildcommit_short = result.buildcommit.substring(0, 9);
+		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildcommit_short})`);
 
 		    if (PVE.Utils.compare_ceph_versions(result.version, maxversion) !== 0) {
 			let host_version = metadata.node[host]?.version?.parts || metadata.version?.[host] || "";
@@ -202,6 +206,14 @@ Ext.define('PVE.ceph.Services', {
 				gettext('Other cluster members use a newer version of this service, please upgrade and restart'),
 			    );
 			}
+		    } else if (host_buildcommit !== "" && result.buildcommit !== host_buildcommit) {
+			if (result.health > healthstates.HEALTH_OLD) {
+			    result.health = healthstates.HEALTH_OLD;
+			}
+			result.messages.push(
+			    PVE.Utils.get_ceph_icon_html('HEALTH_OLD', true) +
+			    gettext('A newer version was installed but old version still running, please restart'),
+			);
 		    }
 		}
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 7/8] api: ceph: add build commit of host to Ceph osd index endpoint data

[Max Carrara]

This is required in order to avoid making multiple API calls in the
following commit.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 2893456a..de4cc72b 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -206,6 +206,7 @@ __PACKAGE__->register_method ({
 
 	    if ($name && $e->{type} eq 'host') {
 		$new->{version} = $hostversions->{$name}->{version}->{str};
+		$new->{buildcommit} = $hostversions->{$name}->{buildcommit};
 	    }
 	}
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-manager 8/8] ui: ceph: osd: rework rendering of version field & show build commit

[Max Carrara]

The logic of the `render_version` function is split up in order to
handle how the version is displayed depending on the type of the row.

If the parsed version is `undefined` or the row marks the beginning of
the tree, an empty string is now returned. This behaviour is
equivalent to before, it just makes the overall logic easier.

If the row belongs to a node, the build commit is now additionally
displayed in parentheses next to the installed Ceph version:

  18.2.2 (abcd1234)

If the row belongs to an osd, the build commit is also additionally
displayed in parentheses next to the installed Ceph version.
Furthermore, should the build commit of the running osd differ from
the one that's installed on the host, the new hash will also be shown
in parentheses:

  18.2.2 (abcd1234 -> 5678fedc)

The icon displayed for running an osd with an outdated build is the
same as for running an outdated version. The conditional display of
cluster health-related icons remains the same otherwise.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 www/manager6/ceph/OSD.js | 55 ++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index d2caafa4..988962b1 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -642,23 +642,58 @@ Ext.define('PVE.node.CephOsdTree', {
 	},
 
 	render_version: function(value, metadata, rec) {
+	    if (value === undefined || rec.data.type === 'root') {
+		return '';
+	    }
+
 	    let vm = this.getViewModel();
-	    let versions = vm.get('versions');
 	    let icon = "";
-	    let version = value || "";
-	    let maxversion = vm.get('maxversion');
-	    if (value && PVE.Utils.compare_ceph_versions(value, maxversion) !== 0) {
-		let host_version = rec.parentNode?.data?.version || versions[rec.data.host] || "";
-		if (rec.data.type === 'host' || PVE.Utils.compare_ceph_versions(host_version, maxversion) !== 0) {
+	    const maxversion = vm.get('maxversion');
+	    const mixedversions = vm.get('mixedversions');
+
+	    if (rec.data.type === 'host') {
+		const bc = rec.data.buildcommit ?? '';
+
+		if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
 		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
-		} else {
-		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+		} else if (mixedversions) {
+		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
 		}
-	    } else if (value && vm.get('mixedversions')) {
+
+		if (bc === '') {
+		    return `${icon}${value}`;
+		}
+
+		return `${icon}${value} (${bc.substring(0, 9)})`;
+	    }
+
+	    const versionNode = rec.parentNode?.data?.version ?? '';
+
+	    const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
+	    const bcNode = rec.parentNode?.data?.buildcommit ?? '';
+
+	    let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
+
+	    if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
+		icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
+	    } else if (versionNode && PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
+		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+	    } else if (mixedversions && !bcChanged) {
 		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
 	    }
 
-	    return icon + version;
+	    let bcPart = bc.substring(0, 9);
+	    if (bcChanged) {
+		const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
+		icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+		bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
+	    }
+
+	    if (bcPart === '') {
+		return `${icon}${value}`;
+	    }
+
+	    return `${icon}${value} (${bcPart})`;
 	},
 
 	render_osd_val: function(value, metaData, rec) {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI

[Max Carrara]

On Tue Apr 30, 2024 at 5:28 PM CEST, Max Carrara wrote:
> Ceph Build Commit in UI - Version 1
> ===================================
>
> This series adds Ceph's build commit to the UI and lets the user know if
> a service is running an outdated build and therefore ought to be
> restarted.

Bump ;)

Also, note: This fixes #5366; I forgot to add the respective git
trailers for that. Mea culpa.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI

[Lukas Wagner]

On  2024-04-30 17:28, Max Carrara wrote:
> Ceph Build Commit in UI - Version 1
> ===================================
> 
> This series adds Ceph's build commit to the UI and lets the user know if
> a service is running an outdated build and therefore ought to be
> restarted.
> 
> The build commit is now displayed next to the version for all Ceph
> services like so:
> 
>   18.2.2 (abcd1234)
> 
> Should a service run an outdated build, the new build commit is also
> displayed:
> 
>   18.2.2 (abcd1234 -> 5678fedc)
> 
> (Icons are omitted here).
> 
> See the individual patches for more in-depth information.
> 
> Additionally, some of the code was also cleaned up and refactored a
> little along the way.
> 
> I'm not 100% sure if the design I've opted for here is the best, so it
> would be great to get some opinions on this. The OSD tree/list view
> especially can get a little noisy if there are a lot of outdated OSDs
> running.
> 

Neat, gave this a quick test and this seems to do what you describe in the cover letter.

Some comments:
  - The 'outdated OSD' view in the top-level status widget uses a different icon to
    signal an outdated OSD than the version column in the OSD tree view - maybe it would make sense
    to use the same icon here?
  - The 'Detail' view for OSDs could also show the commit hash, right now it only shows e.g. 18.2.2 (reef)


With the nits from the review fixed:

Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>



-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 6/8] ui: ceph: services: parse and display build commit

[Lukas Wagner]

On  2024-04-30 17:28, Max Carrara wrote:
> This commit adds `PVE.Utils.parse_ceph_buildcommit`, which can be used
> to get the full hash "eccf199d..." in parentheses from a string like
> the following:
> 
>   ceph version 17.2.7 (eccf199d63457659c09677399928203b7903c888) quincy (stable)
> 
> This hash is displayed and taken into account when comparing monitor
> and manager versions in the client. Specifically, the shortened build
> commit is now displayed in parentheses next to the version for both
> monitors and managers like so:
> 
>   18.2.2 (abcd1234)
> 
> Should the build commit of the running service differ from the one
> that's installed on the host, the newer build commit will also be
> shown in parentheses:
> 
>   18.2.2 (abcd1234 -> 5678fedc)
> 
> The icon displayed for running a service with an outdated build is the
> same as for running an outdated version. The conditional display of
> cluster health-related icons remains the same otherwise.
> 
> The Ceph summary view also displays the hash and will show a warning
> if a service is running with a build commit that doesn't match the one
> that's advertised by the host.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/manager6/Utils.js            | 14 ++++++++++++++
>  www/manager6/ceph/ServiceList.js | 28 ++++++++++++++++++++++------
>  www/manager6/ceph/Services.js    | 14 +++++++++++++-
>  3 files changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 74e46694..435c79d7 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
>  	return undefined;
>      },
>  
> +    parse_ceph_buildcommit: function(service) {
> +	if (service.ceph_version) {
> +	    // See PVE/Ceph/Tools.pm - get_local_version
> +	    const match = service.ceph_version.match(
> +		/^ceph.*\sv?(?:\d+(?:\.\d+)+)\s+(?:\(([a-zA-Z0-9]+)\))/,
> +	    );
> +	    if (match) {
> +		return match[1];
> +	    }
> +	}
> +
> +	return undefined;
> +    },
> +
>      compare_ceph_versions: function(a, b) {

style nit: new functions/vars should use camelCase, see our JS styleguide [1]

>  	let avers = [];
>  	let bvers = [];
> diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
> index 76710146..30f455ed 100644
> --- a/www/manager6/ceph/ServiceList.js
> +++ b/www/manager6/ceph/ServiceList.js
> @@ -102,21 +102,37 @@ Ext.define('PVE.node.CephServiceController', {
>  	if (value === undefined) {
>  	    return '';
>  	}
> +
> +	const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';

I'd prefer a longer variable name here, e.g. buildCommit - bc is a bit too terse for my
taste :)

> +
>  	let view = this.getView();
> -	let host = rec.data.host, nodev = [0];
> +	const host = rec.data.host;

style nit: we only really use `const` for proper constants, see our JS guidelines

> +
> +	let versionNode = [0];
> +	let bcNode = '';
>  	if (view.nodeversions[host] !== undefined) {
> -	    nodev = view.nodeversions[host].version.parts;
> +	    versionNode = view.nodeversions[host].version.parts;
> +	    bcNode = view.nodeversions[host].buildcommit;
>  	}
>  
> +	let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
>  	let icon = '';
> -	if (PVE.Utils.compare_ceph_versions(view.maxversion, nodev) > 0) {
> +	if (PVE.Utils.compare_ceph_versions(view.maxversion, versionNode) > 0) {
>  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> -	} else if (PVE.Utils.compare_ceph_versions(nodev, value) > 0) {
> +	} else if (PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
>  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> -	} else if (view.mixedversions) {
> +	} else if (view.mixedversions && !bcChanged) {
>  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
>  	}
> -	return icon + value;
> +
> +	let bcPart = bc.substring(0, 9);
> +	if (bcChanged) {
> +	    const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
> +	    icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> +	    bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
> +	}
> +
> +	return `${icon}${value} (${bcPart})`;
>      },
>  
>      getMaxVersions: function(store, records, success) {
> diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
> index b9fc52c8..410b28bf 100644
> --- a/www/manager6/ceph/Services.js
> +++ b/www/manager6/ceph/Services.js
> @@ -155,6 +155,7 @@ Ext.define('PVE.ceph.Services', {
>  		    title: metadata[type][id].name || name,
>  		    host: host,
>  		    version: PVE.Utils.parse_ceph_version(metadata[type][id]),
> +		    buildcommit: PVE.Utils.parse_ceph_buildcommit(metadata[type][id]),
>  		    service: metadata[type][id].service,
>  		    addr: metadata[type][id].addr || metadata[type][id].addrs || Proxmox.Utils.unknownText,
>  		};
> @@ -181,7 +182,10 @@ Ext.define('PVE.ceph.Services', {
>  		}
>  
>  		if (result.version) {
> -		    result.statuses.push(gettext('Version') + ": " + result.version);
> +		    const host_buildcommit = metadata.node[host]?.buildcommit || "";
> +
> +		    const buildcommit_short = result.buildcommit.substring(0, 9);
> +		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildcommit_short})`);
>  
>  		    if (PVE.Utils.compare_ceph_versions(result.version, maxversion) !== 0) {
>  			let host_version = metadata.node[host]?.version?.parts || metadata.version?.[host] || "";
> @@ -202,6 +206,14 @@ Ext.define('PVE.ceph.Services', {
>  				gettext('Other cluster members use a newer version of this service, please upgrade and restart'),
>  			    );
>  			}
> +		    } else if (host_buildcommit !== "" && result.buildcommit !== host_buildcommit) {
> +			if (result.health > healthstates.HEALTH_OLD) {
> +			    result.health = healthstates.HEALTH_OLD;
> +			}
> +			result.messages.push(
> +			    PVE.Utils.get_ceph_icon_html('HEALTH_OLD', true) +
> +			    gettext('A newer version was installed but old version still running, please restart'),
> +			);
>  		    }
>  		}
>  

[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 8/8] ui: ceph: osd: rework rendering of version field & show build commit

[Lukas Wagner]

On  2024-04-30 17:28, Max Carrara wrote:
> The logic of the `render_version` function is split up in order to
> handle how the version is displayed depending on the type of the row.
> 
> If the parsed version is `undefined` or the row marks the beginning of
> the tree, an empty string is now returned. This behaviour is
> equivalent to before, it just makes the overall logic easier.
> 
> If the row belongs to a node, the build commit is now additionally
> displayed in parentheses next to the installed Ceph version:
> 
>   18.2.2 (abcd1234)
> 
> If the row belongs to an osd, the build commit is also additionally
> displayed in parentheses next to the installed Ceph version.
> Furthermore, should the build commit of the running osd differ from
> the one that's installed on the host, the new hash will also be shown
> in parentheses:
> 
>   18.2.2 (abcd1234 -> 5678fedc)
> 
> The icon displayed for running an osd with an outdated build is the
> same as for running an outdated version. The conditional display of
> cluster health-related icons remains the same otherwise.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/manager6/ceph/OSD.js | 55 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
> index d2caafa4..988962b1 100644
> --- a/www/manager6/ceph/OSD.js
> +++ b/www/manager6/ceph/OSD.js
> @@ -642,23 +642,58 @@ Ext.define('PVE.node.CephOsdTree', {
>  	},
>  
>  	render_version: function(value, metadata, rec) {
> +	    if (value === undefined || rec.data.type === 'root') {
> +		return '';
> +	    }
> +
>  	    let vm = this.getViewModel();
> -	    let versions = vm.get('versions');
>  	    let icon = "";
> -	    let version = value || "";
> -	    let maxversion = vm.get('maxversion');
> -	    if (value && PVE.Utils.compare_ceph_versions(value, maxversion) !== 0) {
> -		let host_version = rec.parentNode?.data?.version || versions[rec.data.host] || "";
> -		if (rec.data.type === 'host' || PVE.Utils.compare_ceph_versions(host_version, maxversion) !== 0) {
> +	    const maxversion = vm.get('maxversion');
> +	    const mixedversions = vm.get('mixedversions');
> +
> +	    if (rec.data.type === 'host') {
> +		const bc = rec.data.buildcommit ?? '';
> +
> +		if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
>  		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> -		} else {
> -		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> +		} else if (mixedversions) {
> +		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
>  		}
> -	    } else if (value && vm.get('mixedversions')) {
> +
> +		if (bc === '') {
> +		    return `${icon}${value}`;
> +		}
> +
> +		return `${icon}${value} (${bc.substring(0, 9)})`;
> +	    }
> +
> +	    const versionNode = rec.parentNode?.data?.version ?? '';
> +
> +	    const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
> +	    const bcNode = rec.parentNode?.data?.buildcommit ?? '';

same as before, rather use `let` than `const` and maybe use a longer, more
clear name.

> +
> +	    let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
> +
> +	    if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
> +		icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> +	    } else if (versionNode && PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
> +		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> +	    } else if (mixedversions && !bcChanged) {
>  		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
>  	    }
>  
> -	    return icon + version;
> +	    let bcPart = bc.substring(0, 9);
> +	    if (bcChanged) {
> +		const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
> +		icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> +		bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
> +	    }
> +
> +	    if (bcPart === '') {
> +		return `${icon}${value}`;
> +	    }
> +
> +	    return `${icon}${value} (${bcPart})`;
>  	},
>  
>  	render_osd_val: function(value, metaData, rec) {

-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 2/8] ceph: tools: update Ceph version regex

[Lukas Wagner]

On  2024-04-30 17:28, Max Carrara wrote:
> Make the regex more maintainable declaring it as a variable, breaking it
> up and commenting it by using the x flag.
> 
> Also remove the part that parses our Debian revision (e.g. -pve1) from
> the version, as we do not actually include that in our Ceph builds.
> 
> The part of the regex that parses the build commit hash is made
> mandatory (remove '?' after its group).
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/Ceph/Tools.pm | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 087c4ef3..a00d23e1 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -68,7 +68,18 @@ sub get_local_version {
>  
>      return undef if !defined $ceph_version;
>  
> -    if ($ceph_version =~ /^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
> +    my $re_ceph_version = qr/
> +	# Skip ahead to the version, which may optionally start with 'v'
> +	^ceph.*\sv?
> +
> +	# Parse the version X.Y, X.Y.Z, etc.
> +	( \d+ (?:\.\d+)+ ) \s+
> +
> +	# Parse the git commit hash between parentheses
> +	(?: \( ([a-zA-Z0-9]+) \) )
> +    /x;

TIL about the x modifier! Neat!

-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 6/8] ui: ceph: services: parse and display build commit

[Max Carrara]

On Wed Jun 5, 2024 at 4:48 PM CEST, Lukas Wagner wrote:
>
>
> On  2024-04-30 17:28, Max Carrara wrote:
> > This commit adds `PVE.Utils.parse_ceph_buildcommit`, which can be used
> > to get the full hash "eccf199d..." in parentheses from a string like
> > the following:
> > 
> >   ceph version 17.2.7 (eccf199d63457659c09677399928203b7903c888) quincy (stable)
> > 
> > This hash is displayed and taken into account when comparing monitor
> > and manager versions in the client. Specifically, the shortened build
> > commit is now displayed in parentheses next to the version for both
> > monitors and managers like so:
> > 
> >   18.2.2 (abcd1234)
> > 
> > Should the build commit of the running service differ from the one
> > that's installed on the host, the newer build commit will also be
> > shown in parentheses:
> > 
> >   18.2.2 (abcd1234 -> 5678fedc)
> > 
> > The icon displayed for running a service with an outdated build is the
> > same as for running an outdated version. The conditional display of
> > cluster health-related icons remains the same otherwise.
> > 
> > The Ceph summary view also displays the hash and will show a warning
> > if a service is running with a build commit that doesn't match the one
> > that's advertised by the host.
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  www/manager6/Utils.js            | 14 ++++++++++++++
> >  www/manager6/ceph/ServiceList.js | 28 ++++++++++++++++++++++------
> >  www/manager6/ceph/Services.js    | 14 +++++++++++++-
> >  3 files changed, 49 insertions(+), 7 deletions(-)
> > 
> > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> > index 74e46694..435c79d7 100644
> > --- a/www/manager6/Utils.js
> > +++ b/www/manager6/Utils.js
> > @@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
> >  	return undefined;
> >      },
> >  
> > +    parse_ceph_buildcommit: function(service) {
> > +	if (service.ceph_version) {
> > +	    // See PVE/Ceph/Tools.pm - get_local_version
> > +	    const match = service.ceph_version.match(
> > +		/^ceph.*\sv?(?:\d+(?:\.\d+)+)\s+(?:\(([a-zA-Z0-9]+)\))/,
> > +	    );
> > +	    if (match) {
> > +		return match[1];
> > +	    }
> > +	}
> > +
> > +	return undefined;
> > +    },
> > +
> >      compare_ceph_versions: function(a, b) {
>
> style nit: new functions/vars should use camelCase, see our JS styleguide [1]

Oh woops, thanks for pointing this out!

>
> >  	let avers = [];
> >  	let bvers = [];
> > diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
> > index 76710146..30f455ed 100644
> > --- a/www/manager6/ceph/ServiceList.js
> > +++ b/www/manager6/ceph/ServiceList.js
> > @@ -102,21 +102,37 @@ Ext.define('PVE.node.CephServiceController', {
> >  	if (value === undefined) {
> >  	    return '';
> >  	}
> > +
> > +	const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
>
> I'd prefer a longer variable name here, e.g. buildCommit - bc is a bit too terse for my
> taste :)

Fair ;)

>
> > +
> >  	let view = this.getView();
> > -	let host = rec.data.host, nodev = [0];
> > +	const host = rec.data.host;
>
> style nit: we only really use `const` for proper constants, see our JS guidelines

Again thanks for pointing this out!

>
> > +
> > +	let versionNode = [0];
> > +	let bcNode = '';
> >  	if (view.nodeversions[host] !== undefined) {
> > -	    nodev = view.nodeversions[host].version.parts;
> > +	    versionNode = view.nodeversions[host].version.parts;
> > +	    bcNode = view.nodeversions[host].buildcommit;
> >  	}
> >  
> > +	let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
> >  	let icon = '';
> > -	if (PVE.Utils.compare_ceph_versions(view.maxversion, nodev) > 0) {
> > +	if (PVE.Utils.compare_ceph_versions(view.maxversion, versionNode) > 0) {
> >  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> > -	} else if (PVE.Utils.compare_ceph_versions(nodev, value) > 0) {
> > +	} else if (PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
> >  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> > -	} else if (view.mixedversions) {
> > +	} else if (view.mixedversions && !bcChanged) {
> >  	    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
> >  	}
> > -	return icon + value;
> > +
> > +	let bcPart = bc.substring(0, 9);
> > +	if (bcChanged) {
> > +	    const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
> > +	    icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> > +	    bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
> > +	}
> > +
> > +	return `${icon}${value} (${bcPart})`;
> >      },
> >  
> >      getMaxVersions: function(store, records, success) {
> > diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
> > index b9fc52c8..410b28bf 100644
> > --- a/www/manager6/ceph/Services.js
> > +++ b/www/manager6/ceph/Services.js
> > @@ -155,6 +155,7 @@ Ext.define('PVE.ceph.Services', {
> >  		    title: metadata[type][id].name || name,
> >  		    host: host,
> >  		    version: PVE.Utils.parse_ceph_version(metadata[type][id]),
> > +		    buildcommit: PVE.Utils.parse_ceph_buildcommit(metadata[type][id]),
> >  		    service: metadata[type][id].service,
> >  		    addr: metadata[type][id].addr || metadata[type][id].addrs || Proxmox.Utils.unknownText,
> >  		};
> > @@ -181,7 +182,10 @@ Ext.define('PVE.ceph.Services', {
> >  		}
> >  
> >  		if (result.version) {
> > -		    result.statuses.push(gettext('Version') + ": " + result.version);
> > +		    const host_buildcommit = metadata.node[host]?.buildcommit || "";
> > +
> > +		    const buildcommit_short = result.buildcommit.substring(0, 9);
> > +		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildcommit_short})`);
> >  
> >  		    if (PVE.Utils.compare_ceph_versions(result.version, maxversion) !== 0) {
> >  			let host_version = metadata.node[host]?.version?.parts || metadata.version?.[host] || "";
> > @@ -202,6 +206,14 @@ Ext.define('PVE.ceph.Services', {
> >  				gettext('Other cluster members use a newer version of this service, please upgrade and restart'),
> >  			    );
> >  			}
> > +		    } else if (host_buildcommit !== "" && result.buildcommit !== host_buildcommit) {
> > +			if (result.health > healthstates.HEALTH_OLD) {
> > +			    result.health = healthstates.HEALTH_OLD;
> > +			}
> > +			result.messages.push(
> > +			    PVE.Utils.get_ceph_icon_html('HEALTH_OLD', true) +
> > +			    gettext('A newer version was installed but old version still running, please restart'),
> > +			);
> >  		    }
> >  		}
> >  
>
> [1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 8/8] ui: ceph: osd: rework rendering of version field & show build commit

[Max Carrara]

On Wed Jun 5, 2024 at 4:48 PM CEST, Lukas Wagner wrote:
>
>
> On  2024-04-30 17:28, Max Carrara wrote:
> > The logic of the `render_version` function is split up in order to
> > handle how the version is displayed depending on the type of the row.
> > 
> > If the parsed version is `undefined` or the row marks the beginning of
> > the tree, an empty string is now returned. This behaviour is
> > equivalent to before, it just makes the overall logic easier.
> > 
> > If the row belongs to a node, the build commit is now additionally
> > displayed in parentheses next to the installed Ceph version:
> > 
> >   18.2.2 (abcd1234)
> > 
> > If the row belongs to an osd, the build commit is also additionally
> > displayed in parentheses next to the installed Ceph version.
> > Furthermore, should the build commit of the running osd differ from
> > the one that's installed on the host, the new hash will also be shown
> > in parentheses:
> > 
> >   18.2.2 (abcd1234 -> 5678fedc)
> > 
> > The icon displayed for running an osd with an outdated build is the
> > same as for running an outdated version. The conditional display of
> > cluster health-related icons remains the same otherwise.
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  www/manager6/ceph/OSD.js | 55 ++++++++++++++++++++++++++++++++--------
> >  1 file changed, 45 insertions(+), 10 deletions(-)
> > 
> > diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
> > index d2caafa4..988962b1 100644
> > --- a/www/manager6/ceph/OSD.js
> > +++ b/www/manager6/ceph/OSD.js
> > @@ -642,23 +642,58 @@ Ext.define('PVE.node.CephOsdTree', {
> >  	},
> >  
> >  	render_version: function(value, metadata, rec) {
> > +	    if (value === undefined || rec.data.type === 'root') {
> > +		return '';
> > +	    }
> > +
> >  	    let vm = this.getViewModel();
> > -	    let versions = vm.get('versions');
> >  	    let icon = "";
> > -	    let version = value || "";
> > -	    let maxversion = vm.get('maxversion');
> > -	    if (value && PVE.Utils.compare_ceph_versions(value, maxversion) !== 0) {
> > -		let host_version = rec.parentNode?.data?.version || versions[rec.data.host] || "";
> > -		if (rec.data.type === 'host' || PVE.Utils.compare_ceph_versions(host_version, maxversion) !== 0) {
> > +	    const maxversion = vm.get('maxversion');
> > +	    const mixedversions = vm.get('mixedversions');
> > +
> > +	    if (rec.data.type === 'host') {
> > +		const bc = rec.data.buildcommit ?? '';
> > +
> > +		if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
> >  		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> > -		} else {
> > -		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> > +		} else if (mixedversions) {
> > +		    icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
> >  		}
> > -	    } else if (value && vm.get('mixedversions')) {
> > +
> > +		if (bc === '') {
> > +		    return `${icon}${value}`;
> > +		}
> > +
> > +		return `${icon}${value} (${bc.substring(0, 9)})`;
> > +	    }
> > +
> > +	    const versionNode = rec.parentNode?.data?.version ?? '';
> > +
> > +	    const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
> > +	    const bcNode = rec.parentNode?.data?.buildcommit ?? '';
>
> same as before, rather use `let` than `const` and maybe use a longer, more
> clear name.

ACK - thanks!

>
> > +
> > +	    let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
> > +
> > +	    if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
> > +		icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
> > +	    } else if (versionNode && PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
> > +		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> > +	    } else if (mixedversions && !bcChanged) {
> >  		icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
> >  	    }
> >  
> > -	    return icon + version;
> > +	    let bcPart = bc.substring(0, 9);
> > +	    if (bcChanged) {
> > +		const arrow = '<i class="fa fa-fw fa-long-arrow-right"></i>';
> > +		icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
> > +		bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
> > +	    }
> > +
> > +	    if (bcPart === '') {
> > +		return `${icon}${value}`;
> > +	    }
> > +
> > +	    return `${icon}${value} (${bcPart})`;
> >  	},
> >  
> >  	render_osd_val: function(value, metaData, rec) {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI

[Max Carrara]

On Wed Jun 5, 2024 at 4:48 PM CEST, Lukas Wagner wrote:
>
>
> On  2024-04-30 17:28, Max Carrara wrote:
> > Ceph Build Commit in UI - Version 1
> > ===================================
> > 
> > This series adds Ceph's build commit to the UI and lets the user know if
> > a service is running an outdated build and therefore ought to be
> > restarted.
> > 
> > The build commit is now displayed next to the version for all Ceph
> > services like so:
> > 
> >   18.2.2 (abcd1234)
> > 
> > Should a service run an outdated build, the new build commit is also
> > displayed:
> > 
> >   18.2.2 (abcd1234 -> 5678fedc)
> > 
> > (Icons are omitted here).
> > 
> > See the individual patches for more in-depth information.
> > 
> > Additionally, some of the code was also cleaned up and refactored a
> > little along the way.
> > 
> > I'm not 100% sure if the design I've opted for here is the best, so it
> > would be great to get some opinions on this. The OSD tree/list view
> > especially can get a little noisy if there are a lot of outdated OSDs
> > running.
> > 
>
> Neat, gave this a quick test and this seems to do what you describe in the cover letter.
>
> Some comments:
>   - The 'outdated OSD' view in the top-level status widget uses a different icon to
>     signal an outdated OSD than the version column in the OSD tree view - maybe it would make sense
>     to use the same icon here?
>   - The 'Detail' view for OSDs could also show the commit hash, right now it only shows e.g. 18.2.2 (reef)
>
>
> With the nits from the review fixed:
>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

Thanks a lot for the review and giving this a spin! I'll see if I can
send in a v2 soon. If noone else has any objections, I'll attach your
T-b and R-b tags in the new series.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel