[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 at proxmox.com>
> > Tested-by: Lukas Wagner <l.wagner at proxmox.com>
> > Reviewed-by: Lukas Wagner <l.wagner at 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!