Re: [pve-devel] [PATCH v3 pve-manager 1/5] fix #5366: ui: ceph: services: parse and display build commit

[Aaron Lauterer <a.lauterer@proxmox.com>]

one very small nit inline at the end

On  2024-07-24  17:05, Max Carrara wrote:
> The build commit 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 (build: 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 (build: 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
> icons related to cluster health 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.
> 
> This requires the introduction of two helper functions:
> 
> 1. `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)
> 
> 2. `PVE.Utils.renderCephBuildCommit`, which is used to determine how
>     to render a service's current build commit and which accompanying
>     icon to choose.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5366
> ---
> Changes v2 --> v3:
>    * add new `renderCephBuildCommit` helper function (thanks Thomas!)
>    * add docstrings for helpers
>    * use less ambiguous variable names (thanks Thomas!)
>    * put 'build: ' in front of build commit when rendering (thanks Thomas!)
>    * handle no rendered build commit being available in MGR / MON lists,
>      returning the icon + version without the commit instead
>    * make the modified logic in Services.js more readable
>    * reword message about differing builds in the overview
>    * reword commit title & message
>    * add 'Fixes' trailer
>    * remove outdated R-b and T-b trailers
> 
> 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            | 107 +++++++++++++++++++++++++++++++
>   www/manager6/ceph/ServiceList.js |  33 +++++++---
>   www/manager6/ceph/Services.js    |  19 +++++-
>   3 files changed, 148 insertions(+), 11 deletions(-)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index db86fa9a..1d42be34 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -128,6 +128,113 @@ Ext.define('PVE.Utils', {
>   	return undefined;
>       },
>   
> +    /**
> +     * Parses a Ceph build commit from its version string.

[...]
>   
>       getMaxVersions: function(store, records, success) {
> diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
> index dfafee43..ff6f80e9 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,15 @@ Ext.define('PVE.ceph.Services', {
>   		}
>   
>   		if (result.version) {
> -		    result.statuses.push(gettext('Version') + ": " + result.version);
> +		    let installedBuildCommit = metadata.node[host]?.buildcommit ?? '';
> +		    let runningBuildCommit = result.buildcommit ?? '';
> +
> +		    let statusLine = gettext('Version') + `: ${result.version}`;
> +		    if (runningBuildCommit) {
> +			statusLine += ` (build: ${runningBuildCommit.substring(0, 9)})`;
> +		    }
> +
> +		    result.statuses.push(statusLine);
>   
>   		    if (PVE.Utils.compare_ceph_versions(result.version, maxversion) !== 0) {
>   			let host_version = metadata.node[host]?.version?.parts || metadata.version?.[host] || "";
> @@ -202,6 +211,14 @@ Ext.define('PVE.ceph.Services', {
>   				gettext('Other cluster members use a newer version of this service, please upgrade and restart'),
>   			    );
>   			}
> +		    } else if (installedBuildCommit !== "" && runningBuildCommit !== installedBuildCommit) {
> +			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 build of this release was installed but old build still running, please restart'),

Maybe phrase it like:

A newer build of this release was installed, but the old build is still 
running. Please restart the service.

But that is really just a nice to have thing.

> +			);
>   		    }
>   		}
>   



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