[pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

[pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

[Max Carrara]

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

Notable Changes since v1
------------------------

  * Use camelCase instead of snake_case for new functions / variables
    as per our style guide [0] (thanks Lukas!)
  * Refrain from using `const` for things that aren't actual constants
    as per our style guide [1] (thanks Lukas!)
  * NEW: Patch 09: Increase the default width of the version field in
    the OSD tree so that longer strings are immediately readable without
    needing to adjust the column widths manually
    --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space
    in the column
  * NEW: Patch 10: Include Ceph build commit in the version string
    which is part of the object of the `ceph/osd/{osdid}/metadata` call

For a detailed list of changes, please see the comments in the
individual patches.

NOTE: I added Lukas's T-b and R-b tags to all patches except the new
ones, as mentioned in a reply to v1 [2].

Older Versions
--------------

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html

References
----------

[0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
[1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
[2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html

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

Max Carrara (10):
  ceph: tools: refactor installation check as guard clause
  ceph: tools: parse Ceph version in separate sub and update 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
  ui: ceph: osd: increase width of version column
  api: ceph: change version format in OSD metadata endpoint

 PVE/API2/Ceph/OSD.pm             |  9 ++++-
 PVE/Ceph/Services.pm             | 38 ++++++++++----------
 PVE/Ceph/Tools.pm                | 59 ++++++++++++++++++++++----------
 www/manager6/Utils.js            | 17 ++++++++-
 www/manager6/ceph/OSD.js         | 57 +++++++++++++++++++++++++-----
 www/manager6/ceph/ServiceList.js | 32 +++++++++++++----
 www/manager6/ceph/Services.js    | 14 +++++++-
 7 files changed, 170 insertions(+), 56 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 v2 pve-manager 01/10] ceph: tools: refactor installation check as guard clause

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * none

 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 v2 pve-manager 02/10] ceph: tools: parse Ceph version in separate sub and update regex

[Max Carrara]

The part of the `get_local_version` sub that's concerned with actually
parsing the Ceph version is factored into a separate sub
`parse_ceph_version`. That way the parsing logic can easily be reused.

Make the version 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>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * parse Ceph version in separate subroutine `parse_ceph_version`
  * update comments

NOTE: Not sure if we should just `return
parse_ceph_version($ceph_version)` below - the context is "passed on"
but I don't want to introduce a pattern where others have to decipher
what a subroutine returns through another subroutine's return value.
(Unless it's really obvious.)

 PVE/Ceph/Tools.pm | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 087c4ef3..90313353 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -68,11 +68,33 @@ 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 ($version, $buildcommit, $subversions) = parse_ceph_version($ceph_version);
+
+    return undef if !defined($version);
+
+    # return (version, buildid, [major, minor, ...]) : major;
+    return wantarray ? ($version, $buildcommit, $subversions) : $subversions->[0];
+}
+
+sub parse_ceph_version : prototype($) {
+    my ($ceph_version) = @_;
+
+    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) ];
 
-	# return (version, buildid, major, minor, ...) : major;
+	# return (version, buildid, [major, minor, ...]) : major;
 	return wantarray
 	    ? ($version, $buildcommit, $subversions)
 	    : $subversions->[0];
-- 
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 v2 pve-manager 03/10] 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>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * none

 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 v2 pve-manager 04/10] ceph: services: refactor version existence check as guard clause

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * none

 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 v2 pve-manager 05/10] utils: align regex of parse_ceph_version with Perl equivalent

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * none

 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 v2 pve-manager 06/10] ui: ceph: services: parse and display build commit

[Max Carrara]

This commit adds `PVE.Utils.parseCephBuildCommit`, 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>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * use camelCase instead of snake_case (thanks Lukas!)
  * use more descriptive variable names (thanks Lukas!)
  * use `let` instead of `const` for variables where applicable (thanks Lukas!)

 www/manager6/Utils.js            | 14 ++++++++++++++
 www/manager6/ceph/ServiceList.js | 32 ++++++++++++++++++++++++++------
 www/manager6/ceph/Services.js    | 14 +++++++++++++-
 3 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 74e46694..f2fd0f7e 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
 	return undefined;
     },
 
+    parseCephBuildCommit: 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..d994aa4e 100644
--- a/www/manager6/ceph/ServiceList.js
+++ b/www/manager6/ceph/ServiceList.js
@@ -102,21 +102,41 @@ Ext.define('PVE.node.CephServiceController', {
 	if (value === undefined) {
 	    return '';
 	}
+
+	let buildCommit = PVE.Utils.parseCephBuildCommit(rec.data) ?? '';
+
 	let view = this.getView();
-	let host = rec.data.host, nodev = [0];
+	let host = rec.data.host;
+
+	let versionNode = [0];
+	let buildCommitNode = '';
 	if (view.nodeversions[host] !== undefined) {
-	    nodev = view.nodeversions[host].version.parts;
+	    versionNode = view.nodeversions[host].version.parts;
+	    buildCommitNode = view.nodeversions[host].buildcommit;
 	}
 
+	let bcChanged =
+	    buildCommit !== '' &&
+	    buildCommitNode !== '' &&
+	    buildCommit !== buildCommitNode;
+
 	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 buildCommitPart = buildCommit.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');
+	    buildCommitPart = `${buildCommit.substring(0, 9)}${arrow}${buildCommitNode.substring(0, 9)}`;
+	}
+
+	return `${icon}${value} (${buildCommitPart})`;
     },
 
     getMaxVersions: function(store, records, success) {
diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
index b9fc52c8..7ce289dd 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.parseCephBuildCommit(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 buildCommitHost = metadata.node[host]?.buildcommit || "";
+
+		    const buildCommitShort = result.buildcommit.substring(0, 9);
+		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildCommitShort})`);
 
 		    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 (buildCommitHost !== "" && result.buildcommit !== buildCommitHost) {
+			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 v2 pve-manager 07/10] 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>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * none

 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 v2 pve-manager 08/10] 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>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 --> v2:
  * use camelCase instead of snake_case (thanks Lukas!)
  * use more descriptive variable names (thanks Lukas!)
  * use `let` instead of `const` for variables where applicable (thanks Lukas!)

 www/manager6/ceph/OSD.js | 56 +++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index d2caafa4..37fe6a9c 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -642,23 +642,61 @@ 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) {
+	    let mixedversions = vm.get('mixedversions');
+
+	    if (rec.data.type === 'host') {
+		let buildCommit = 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 (buildCommit === '') {
+		    return `${icon}${value}`;
+		}
+
+		return `${icon}${value} (${buildCommit.substring(0, 9)})`;
+	    }
+
+	    let versionNode = rec.parentNode?.data?.version ?? '';
+
+	    let buildCommit = PVE.Utils.parseCephBuildCommit(rec.data) ?? '';
+	    let buildCommitNode = rec.parentNode?.data?.buildcommit ?? '';
+
+	    let bcChanged =
+		buildCommit !== '' &&
+		buildCommitNode !== '' &&
+		buildCommit !== buildCommitNode;
+
+	    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 buildCommitPart = buildCommit.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');
+		buildCommitPart = `${buildCommit.substring(0, 9)}${arrow}${buildCommitNode.substring(0, 9)}`;
+	    }
+
+	    if (buildCommitPart === '') {
+		return `${icon}${value}`;
+	    }
+
+	    return `${icon}${value} (${buildCommitPart})`;
 	},
 
 	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

[pve-devel] [PATCH v2 pve-manager 09/10] ui: ceph: osd: increase width of version column

[Max Carrara]

.. so that the Ceph build commit as well as differing build commits
are shown properly.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * NEW

 www/manager6/ceph/OSD.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index 37fe6a9c..474e4137 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -811,6 +811,7 @@ Ext.define('PVE.node.CephOsdTree', {
 	    dataIndex: 'version',
 	    align: 'right',
 	    renderer: 'render_version',
+	    width: 240,
 	},
 	{
 	    text: 'weight',
-- 
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 v2 pve-manager 10/10] api: ceph: change version format in OSD metadata endpoint

[Max Carrara]

.. in order to include Ceph's build commit. Instead of e.g.

  18.2.2 (reef)

the string will now contain:

  18.2.2 (e9fe820e7) reef

This format is used in the OSD detail view; the build commit will
therefore also be shown there.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * NEW

NOTE: I'm not sure if this is 100% fine to do here; if it's something
that we consider to be API-breaking, I'll send in a v3 that adds the
build commit as a separate field.

 PVE/API2/Ceph/OSD.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index de4cc72b..a375bf6e 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -747,6 +747,12 @@ __PACKAGE__->register_method ({
 	my $osd_pss_memory = eval { get_proc_pss_from_pid($pid) } // 0;
 	warn $@ if $@;
 
+	my ($ceph_version, $ceph_buildcommit) = PVE::Ceph::Tools::parse_ceph_version(
+	    $metadata->{ceph_version}
+	);
+
+	$ceph_buildcommit = substr($ceph_buildcommit, 0, 9);
+
 	my $data = {
 	    osd => {
 		hostname => $metadata->{hostname},
@@ -755,7 +761,7 @@ __PACKAGE__->register_method ({
 		osd_data => $metadata->{osd_data},
 		osd_objectstore => $metadata->{osd_objectstore},
 		pid => $pid,
-		version => "$metadata->{ceph_version_short} ($metadata->{ceph_release})",
+		version => "$ceph_version ($ceph_buildcommit) $metadata->{ceph_release}",
 		front_addr => $metadata->{front_addr},
 		back_addr => $metadata->{back_addr},
 		hb_front_addr => $metadata->{hb_front_addr},
-- 
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 v2 pve-manager 00/10] Ceph Build Commit in UI

[Max Carrara]

On Mon Jul 1, 2024 at 4:10 PM CEST, Max Carrara wrote:
> Ceph Build Commit in UI - Version 2
> ===================================

Ah, forgot to note (again) that this fixes #5366 [0]. Mea culpa.

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5366

>
> Notable Changes since v1
> ------------------------
>
>   * Use camelCase instead of snake_case for new functions / variables
>     as per our style guide [0] (thanks Lukas!)
>   * Refrain from using `const` for things that aren't actual constants
>     as per our style guide [1] (thanks Lukas!)
>   * NEW: Patch 09: Increase the default width of the version field in
>     the OSD tree so that longer strings are immediately readable without
>     needing to adjust the column widths manually
>     --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space
>     in the column
>   * NEW: Patch 10: Include Ceph build commit in the version string
>     which is part of the object of the `ceph/osd/{osdid}/metadata` call
>
> For a detailed list of changes, please see the comments in the
> individual patches.
>
> NOTE: I added Lukas's T-b and R-b tags to all patches except the new
> ones, as mentioned in a reply to v1 [2].
>
> Older Versions
> --------------
>
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html
>
> References
> ----------
>
> [0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
> [1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> [2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html
>
> Summary of Changes
> ------------------
>
> Max Carrara (10):
>   ceph: tools: refactor installation check as guard clause
>   ceph: tools: parse Ceph version in separate sub and update 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
>   ui: ceph: osd: increase width of version column
>   api: ceph: change version format in OSD metadata endpoint
>
>  PVE/API2/Ceph/OSD.pm             |  9 ++++-
>  PVE/Ceph/Services.pm             | 38 ++++++++++----------
>  PVE/Ceph/Tools.pm                | 59 ++++++++++++++++++++++----------
>  www/manager6/Utils.js            | 17 ++++++++-
>  www/manager6/ceph/OSD.js         | 57 +++++++++++++++++++++++++-----
>  www/manager6/ceph/ServiceList.js | 32 +++++++++++++----
>  www/manager6/ceph/Services.js    | 14 +++++++-
>  7 files changed, 170 insertions(+), 56 deletions(-)



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

Re: [pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

[Igor Moritz Thaller]

I have tested the new ceph GUI feature where if a cluster has an outdated ceph version running, it will inform the user.

My setup consisted of a ceph cluster with three monitors and four nodes, each having their own two osds. Since I didn't want to rebuild ceph I instead modified the 'CEPH_GIT_VER' variable in the python file '/usr/bin/ceph'. I changed the ceph version multiple times to different versions/non-versions, and it correctly updated the GUI with a warning that the currently running version was outdated.

Overall, from what I have tested, it works great!

Tested-by: Igor Thaller <igor.thaller@brg9.at>
________________________________________
Von: pve-devel <pve-devel-bounces@lists.proxmox.com> im Auftrag von Max Carrara <m.carrara@proxmox.com>
Gesendet: Montag, 1. Juli 2024 16:10
An: pve-devel@lists.proxmox.com
Cc: Lukas Wagner
Betreff: [pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

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

Notable Changes since v1
------------------------

  * Use camelCase instead of snake_case for new functions / variables
    as per our style guide [0] (thanks Lukas!)
  * Refrain from using `const` for things that aren't actual constants
    as per our style guide [1] (thanks Lukas!)
  * NEW: Patch 09: Increase the default width of the version field in
    the OSD tree so that longer strings are immediately readable without
    needing to adjust the column widths manually
    --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space
    in the column
  * NEW: Patch 10: Include Ceph build commit in the version string
    which is part of the object of the `ceph/osd/{osdid}/metadata` call

For a detailed list of changes, please see the comments in the
individual patches.

NOTE: I added Lukas's T-b and R-b tags to all patches except the new
ones, as mentioned in a reply to v1 [2].

Older Versions
--------------

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html

References
----------

[0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
[1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
[2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html

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

Max Carrara (10):
  ceph: tools: refactor installation check as guard clause
  ceph: tools: parse Ceph version in separate sub and update 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
  ui: ceph: osd: increase width of version column
  api: ceph: change version format in OSD metadata endpoint

 PVE/API2/Ceph/OSD.pm             |  9 ++++-
 PVE/Ceph/Services.pm             | 38 ++++++++++----------
 PVE/Ceph/Tools.pm                | 59 ++++++++++++++++++++++----------
 www/manager6/Utils.js            | 17 ++++++++-
 www/manager6/ceph/OSD.js         | 57 +++++++++++++++++++++++++-----
 www/manager6/ceph/ServiceList.js | 32 +++++++++++++----
 www/manager6/ceph/Services.js    | 14 +++++++-
 7 files changed, 170 insertions(+), 56 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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

[Max Carrara]

On Fri Jul 19, 2024 at 2:17 PM CEST, Igor Moritz Thaller wrote:
> I have tested the new ceph GUI feature where if a cluster has an outdated ceph version running, it will inform the user.
>
> My setup consisted of a ceph cluster with three monitors and four nodes, each having their own two osds. Since I didn't want to rebuild ceph I instead modified the 'CEPH_GIT_VER' variable in the python file '/usr/bin/ceph'. I changed the ceph version multiple times to different versions/non-versions, and it correctly updated the GUI with a warning that the currently running version was outdated.
>
> Overall, from what I have tested, it works great!
>
> Tested-by: Igor Thaller <igor.thaller@brg9.at>

Thanks a lot, appreciate it!

> ________________________________________
> Von: pve-devel <pve-devel-bounces@lists.proxmox.com> im Auftrag von Max Carrara <m.carrara@proxmox.com>
> Gesendet: Montag, 1. Juli 2024 16:10
> An: pve-devel@lists.proxmox.com
> Cc: Lukas Wagner
> Betreff: [pve-devel] [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI
>
> Ceph Build Commit in UI - Version 2
> ===================================
>
> Notable Changes since v1
> ------------------------
>
>   * Use camelCase instead of snake_case for new functions / variables
>     as per our style guide [0] (thanks Lukas!)
>   * Refrain from using `const` for things that aren't actual constants
>     as per our style guide [1] (thanks Lukas!)
>   * NEW: Patch 09: Increase the default width of the version field in
>     the OSD tree so that longer strings are immediately readable without
>     needing to adjust the column widths manually
>     --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space
>     in the column
>   * NEW: Patch 10: Include Ceph build commit in the version string
>     which is part of the object of the `ceph/osd/{osdid}/metadata` call
>
> For a detailed list of changes, please see the comments in the
> individual patches.
>
> NOTE: I added Lukas's T-b and R-b tags to all patches except the new
> ones, as mentioned in a reply to v1 [2].
>
> Older Versions
> --------------
>
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html
>
> References
> ----------
>
> [0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
> [1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> [2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html
>
> Summary of Changes
> ------------------
>
> Max Carrara (10):
>   ceph: tools: refactor installation check as guard clause
>   ceph: tools: parse Ceph version in separate sub and update 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
>   ui: ceph: osd: increase width of version column
>   api: ceph: change version format in OSD metadata endpoint
>
>  PVE/API2/Ceph/OSD.pm             |  9 ++++-
>  PVE/Ceph/Services.pm             | 38 ++++++++++----------
>  PVE/Ceph/Tools.pm                | 59 ++++++++++++++++++++++----------
>  www/manager6/Utils.js            | 17 ++++++++-
>  www/manager6/ceph/OSD.js         | 57 +++++++++++++++++++++++++-----
>  www/manager6/ceph/ServiceList.js | 32 +++++++++++++----
>  www/manager6/ceph/Services.js    | 14 +++++++-
>  7 files changed, 170 insertions(+), 56 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 mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



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

Re: [pve-devel] [PATCH v2 pve-manager 06/10] ui: ceph: services: parse and display build commit

[Thomas Lamprecht]

Am 01/07/2024 um 16:10 schrieb Max Carrara:
> This commit adds `PVE.Utils.parseCephBuildCommit`, 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>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Changes v1 --> v2:
>   * use camelCase instead of snake_case (thanks Lukas!)
>   * use more descriptive variable names (thanks Lukas!)
>   * use `let` instead of `const` for variables where applicable (thanks Lukas!)
> 
>  www/manager6/Utils.js            | 14 ++++++++++++++
>  www/manager6/ceph/ServiceList.js | 32 ++++++++++++++++++++++++++------
>  www/manager6/ceph/Services.js    | 14 +++++++++++++-
>  3 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 74e46694..f2fd0f7e 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
>  	return undefined;
>      },
>  
> +    parseCephBuildCommit: 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..d994aa4e 100644
> --- a/www/manager6/ceph/ServiceList.js
> +++ b/www/manager6/ceph/ServiceList.js
> @@ -102,21 +102,41 @@ Ext.define('PVE.node.CephServiceController', {
>  	if (value === undefined) {
>  	    return '';
>  	}
> +
> +	let buildCommit = PVE.Utils.parseCephBuildCommit(rec.data) ?? '';
> +
>  	let view = this.getView();
> -	let host = rec.data.host, nodev = [0];
> +	let host = rec.data.host;
> +
> +	let versionNode = [0];
> +	let buildCommitNode = '';
>  	if (view.nodeversions[host] !== undefined) {
> -	    nodev = view.nodeversions[host].version.parts;
> +	    versionNode = view.nodeversions[host].version.parts;
> +	    buildCommitNode = view.nodeversions[host].buildcommit;
>  	}
>  
> +	let bcChanged =

I'd prefer the more telling `buildCommitChanged` variable name here.

> +	    buildCommit !== '' &&
> +	    buildCommitNode !== '' &&
> +	    buildCommit !== buildCommitNode;

above hunk and... 

> +
>  	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 buildCommitPart = buildCommit.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');
> +	    buildCommitPart = `${buildCommit.substring(0, 9)}${arrow}${buildCommitNode.substring(0, 9)}`;
> +	}

... most of the above hunk might be better factored out in a helper
function, as this is basically 1:1 duplication here and in patch 08/10.


The function could e.g. take both current and new commits as parameters
and return the rendered build commit (buildCommitPart) and a boolean about
if it should be interpreted as changed (updated?). That could be in form
of an array or object and then destructured here.

also, maybe rendered this as `build ${buildCommit.substring(0, 9)} ...` to
give some name for users to ask/talk about when wondering what this funny
hex string is.

> +
> +	return `${icon}${value} (${buildCommitPart})`;
>      },
>  
>      getMaxVersions: function(store, records, success) {
> diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
> index b9fc52c8..7ce289dd 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.parseCephBuildCommit(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 buildCommitHost = metadata.node[host]?.buildcommit || "";
>
> +
> +		    const buildCommitShort = result.buildcommit.substring(0, 9);

naming is IMO not ideal, I'd rather use something like `buildCommitInstalled`
and `buildCommitRunning` to clarify what each value actually represents.


> +		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildCommitShort})`);
>  
>  		    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 (buildCommitHost !== "" && result.buildcommit !== buildCommitHost) {
> +			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'),

maybe s/version/build/ or possibly s/version/build of the same release/
to better convey that the major release was unchanged but a newer build
got pulled in.


> +			);
>  		    }
>  		}
>  



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

[pve-devel] partially-applied-series: [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI

[Thomas Lamprecht]

Am 01/07/2024 um 16:10 schrieb Max Carrara:
> Ceph Build Commit in UI - Version 2
> ===================================
> 
> Notable Changes since v1
> ------------------------
> 
>   * Use camelCase instead of snake_case for new functions / variables
>     as per our style guide [0] (thanks Lukas!)
>   * Refrain from using `const` for things that aren't actual constants
>     as per our style guide [1] (thanks Lukas!)
>   * NEW: Patch 09: Increase the default width of the version field in
>     the OSD tree so that longer strings are immediately readable without
>     needing to adjust the column widths manually
>     --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space
>     in the column
>   * NEW: Patch 10: Include Ceph build commit in the version string
>     which is part of the object of the `ceph/osd/{osdid}/metadata` call
> 
> For a detailed list of changes, please see the comments in the
> individual patches.
> 
> NOTE: I added Lukas's T-b and R-b tags to all patches except the new
> ones, as mentioned in a reply to v1 [2].
> 
> Older Versions
> --------------
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html
> 
> References
> ----------
> 
> [0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
> [1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> [2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html
> 
> Summary of Changes
> ------------------
> 
> Max Carrara (10):
>   ceph: tools: refactor installation check as guard clause
>   ceph: tools: parse Ceph version in separate sub and update 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

applied above 5 clean-up patches already, thanks!

>   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
>   ui: ceph: osd: increase width of version column
>   api: ceph: change version format in OSD metadata endpoint
> 
>  PVE/API2/Ceph/OSD.pm             |  9 ++++-
>  PVE/Ceph/Services.pm             | 38 ++++++++++----------
>  PVE/Ceph/Tools.pm                | 59 ++++++++++++++++++++++----------
>  www/manager6/Utils.js            | 17 ++++++++-
>  www/manager6/ceph/OSD.js         | 57 +++++++++++++++++++++++++-----
>  www/manager6/ceph/ServiceList.js | 32 +++++++++++++----
>  www/manager6/ceph/Services.js    | 14 +++++++-
>  7 files changed, 170 insertions(+), 56 deletions(-)
> 


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

Re: [pve-devel] [PATCH v2 pve-manager 06/10] ui: ceph: services: parse and display build commit

[Max Carrara]

On Mon Jul 22, 2024 at 5:38 PM CEST, Thomas Lamprecht wrote:
> Am 01/07/2024 um 16:10 schrieb Max Carrara:
> > This commit adds `PVE.Utils.parseCephBuildCommit`, 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>
> > Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> > Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> > ---
> > Changes v1 --> v2:
> >   * use camelCase instead of snake_case (thanks Lukas!)
> >   * use more descriptive variable names (thanks Lukas!)
> >   * use `let` instead of `const` for variables where applicable (thanks Lukas!)
> > 
> >  www/manager6/Utils.js            | 14 ++++++++++++++
> >  www/manager6/ceph/ServiceList.js | 32 ++++++++++++++++++++++++++------
> >  www/manager6/ceph/Services.js    | 14 +++++++++++++-
> >  3 files changed, 53 insertions(+), 7 deletions(-)
> > 
> > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> > index 74e46694..f2fd0f7e 100644
> > --- a/www/manager6/Utils.js
> > +++ b/www/manager6/Utils.js
> > @@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
> >  	return undefined;
> >      },
> >  
> > +    parseCephBuildCommit: 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..d994aa4e 100644
> > --- a/www/manager6/ceph/ServiceList.js
> > +++ b/www/manager6/ceph/ServiceList.js
> > @@ -102,21 +102,41 @@ Ext.define('PVE.node.CephServiceController', {
> >  	if (value === undefined) {
> >  	    return '';
> >  	}
> > +
> > +	let buildCommit = PVE.Utils.parseCephBuildCommit(rec.data) ?? '';
> > +
> >  	let view = this.getView();
> > -	let host = rec.data.host, nodev = [0];
> > +	let host = rec.data.host;
> > +
> > +	let versionNode = [0];
> > +	let buildCommitNode = '';
> >  	if (view.nodeversions[host] !== undefined) {
> > -	    nodev = view.nodeversions[host].version.parts;
> > +	    versionNode = view.nodeversions[host].version.parts;
> > +	    buildCommitNode = view.nodeversions[host].buildcommit;
> >  	}
> >  
> > +	let bcChanged =
>
> I'd prefer the more telling `buildCommitChanged` variable name here.
>
> > +	    buildCommit !== '' &&
> > +	    buildCommitNode !== '' &&
> > +	    buildCommit !== buildCommitNode;
>
> above hunk and... 
>
> > +
> >  	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 buildCommitPart = buildCommit.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');
> > +	    buildCommitPart = `${buildCommit.substring(0, 9)}${arrow}${buildCommitNode.substring(0, 9)}`;
> > +	}
>
> ... most of the above hunk might be better factored out in a helper
> function, as this is basically 1:1 duplication here and in patch 08/10.
>
>
> The function could e.g. take both current and new commits as parameters
> and return the rendered build commit (buildCommitPart) and a boolean about
> if it should be interpreted as changed (updated?). That could be in form
> of an array or object and then destructured here.
>
> also, maybe rendered this as `build ${buildCommit.substring(0, 9)} ...` to
> give some name for users to ask/talk about when wondering what this funny
> hex string is.
>
> > +
> > +	return `${icon}${value} (${buildCommitPart})`;
> >      },
> >  
> >      getMaxVersions: function(store, records, success) {
> > diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
> > index b9fc52c8..7ce289dd 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.parseCephBuildCommit(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 buildCommitHost = metadata.node[host]?.buildcommit || "";
> >
> > +
> > +		    const buildCommitShort = result.buildcommit.substring(0, 9);
>
> naming is IMO not ideal, I'd rather use something like `buildCommitInstalled`
> and `buildCommitRunning` to clarify what each value actually represents.
>
>
> > +		    result.statuses.push(gettext('Version') + `: ${result.version} (${buildCommitShort})`);
> >  
> >  		    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 (buildCommitHost !== "" && result.buildcommit !== buildCommitHost) {
> > +			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'),
>
> maybe s/version/build/ or possibly s/version/build of the same release/
> to better convey that the major release was unchanged but a newer build
> got pulled in.
>
>
> > +			);
> >  		    }
> >  		}
> >  

To keep it short: I agree with all of your suggestions here; I'll
incorporate them in a v3. Thanks a lot!



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