mgr/dashboard: Add text to empty life expectancy column #47876

torchiaf · 2022-08-30T15:50:27Z

Add life_expectancy_enabled field to /api/osd/{svc_id}/devices
Add new value 'n/a' for 'Life Expectancy' column

Fixes: https://tracker.ceph.com/issues/43116
Signed-off-by: Francesco Torchia francesco.torchia@suse.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

torchiaf · 2022-08-31T14:08:37Z

Below, a demo on how the fix was implemented on the web-gui:

torchiaf · 2022-08-31T14:12:39Z

The CI steps which are failed are not apparently related to the changes included in this PR.

votdev · 2022-09-01T07:57:09Z

src/pybind/mgr/dashboard/controllers/osd.py

@@ -391,8 +391,17 @@ def safe_to_delete(self, svc_ids):

    @RESTController.Resource('GET')
    def devices(self, svc_id):
+        devices: list = CephService.send_command(


Which is right type now? Is it a list or a dict as specified in line 403 (# (str) -> dict)?

@epuertat How are type hints handled in the Dashboard code nowadays?

Having a look at it, it can return both a str or anything returned by json.loads:

ceph/src/pybind/mgr/dashboard/services/ceph_service.py

Lines 194 to 230 in 14189b7

def send_command(srv_type, prefix, srv_spec='', **kwargs):

# type: (str, str, Optional[str], Any) -> Any

"""

:type prefix: str

:param srv_type: mon |

:param kwargs: will be added to argdict

:param srv_spec: typically empty. or something like "<fs_id>:0"

:raises PermissionError: See rados.make_ex

:raises ObjectNotFound: See rados.make_ex

:raises IOError: See rados.make_ex

:raises NoSpace: See rados.make_ex

:raises ObjectExists: See rados.make_ex

:raises ObjectBusy: See rados.make_ex

:raises NoData: See rados.make_ex

:raises InterruptedOrTimeoutError: See rados.make_ex

:raises TimedOut: See rados.make_ex

:raises ValueError: return code != 0

"""

argdict = {

"prefix": prefix,

"format": "json",

}

argdict.update({k: v for k, v in kwargs.items() if v is not None})

result = CommandResult("")

mgr.send_command(result, srv_type, srv_spec, json.dumps(argdict), "")

r, outb, outs = result.wait()

if r != 0:

logger.error("send_command '%s' failed. (r=%s, outs=\"%s\", kwargs=%s)", prefix, r,

outs, kwargs)

raise SendCommandError(outs, prefix, argdict, r)

try:

return json.loads(outb or outs)

except Exception: # pylint: disable=broad-except

return outb

epuertat · 2022-09-01T09:37:41Z

src/pybind/mgr/dashboard/controllers/osd.py

+        life_expectancy_enabled = any(
+            item.startswith('diskprediction_') for item in modules['enabled_modules'])
+        for device in devices:
+            device['life_expectancy_enabled'] = life_expectancy_enabled


We should think of a nicer way to deal with cross-module deps. @pereman2 as you're familiar with the ceph-mgr code, isn't there any way to detect whether another module is enabled/running? Or even better, declare that a module depends on others (new global var MODULE_DEPENDS or something like that?) and automatically enable/load them. What do you think?

It should be possible ernesto, let me take a look.

@pereman2 Any news here how to get a list of enabled mgr modules without running a send_command?

@torchiaf I think mgr.get('mgr_map') and mgr.get('mgr_map')['modules'] or mgr.get('mgr_map')['available_modules'] is the way to go. Have a look at https://github.com/ceph/ceph/blob/main/src/pybind/mgr/dashboard/controllers/mgr_modules.py#L47 how to get the required information. @pereman2 I'm right?

There is, MGRMap has all these info and you can get it with mgr.get('mgr_map'). I'll paste a snippet from controllers/mgr_modules.py where this is done.

result = [] mgr_map = mgr.get('mgr_map') always_on_modules = mgr_map['always_on_modules'].get(mgr.release_name, []) for module_config in mgr_map['available_modules']: module_name = module_config['name'] if module_name not in self.ignore_modules: always_on = module_name in always_on_modules enabled = module_name in mgr_map['modules'] or always_on result.append({ 'name': module_name, 'enabled': enabled, 'always_on': always_on, 'options': self._convert_module_options( module_config['module_options']) })

Thanks guys, I will adapt my fix.

@votdev @pereman2 available modules are now fetched by mgr.get('mgr_map').

epuertat · 2022-09-01T09:40:04Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html

+  <span *ngIf="!value.life_expectancy_enabled"
+        i18n>N/A</span>


Rather than dealing with null to N/A converstions case by case, what about implementing this in the datatable behaviour itself? I remember that @avanthakkar did this in other places, so I'd rather avoid doing this ad-hoc and implement it as a default behaviour of the column/cell template. What do you think?

@epuertat do you mean to put a new property, e.g. notAvailable: () => boolean, in TableColumn implementation ?

@torchiaf: the component in which the Dashboard datatable is based on, ngx-datatable, supports a pipe property for each column. We already have a notAvailable pipe that converts an empty string to n/a:

ceph/src/pybind/mgr/dashboard/frontend/src/app/shared/pipes/not-available.pipe.ts

Lines 5 to 15 in f4df409

@Pipe({

name: 'notAvailable'

})

export class NotAvailablePipe implements PipeTransform {

transform(value: any, text?: string): any {

if (value === '') {

return _.defaultTo(text, $localize`n/a`);

}

return value;

}

}

Instead of implementing this same behaviour case by case, IMHO we should probably:

Add that pipe by default to the cd-datatable pipe property,

And extend that pipe to also check for undefined or null and display the n/a on those cases.

Otherwise, we'll end up having to patch every single place where an empty/null/undefined string may appear.

@epuertat I agree with you to put a default value when the value of a cell is null or undefined.
In our case however, I think It is different from the general case.

The value of lifeExpectancy column depends on many values (min, max, life_expectancy_enabled), it is not a simple translation null -> N/A.

ceph/src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html

Lines 14 to 21 in 23f85cc

<ng-template #lifeExpectancy

let-value="value">

N/A

> {{value.min | i18nPlural: translationMapping}}

< {{value.max | i18nPlural: translationMapping}}

{{value.min}} to {{value.max | i18nPlural: translationMapping}}

</ng-template>

My fix is supposed to works this way :

when min and max are empty, show empty value, because it means there are no enough data to calculate the lifeExpectancy value

when life_expectancy_enabled is false, show N/A, because it means that the life expectancy module is disabled.

Probably, I should rename life_expectancy_enabled -> life_expectancy_module_enabled.

What do you think about it?

IMO the column we're talk about is too specific to use generic pipes. In that case the implementation of a template within the component is right.

What can be done to limit the number of variations (N/A vs n/a) which are sadly spread across the whole code is using this:

{{ "" | notAvailable }}

@votdev suggestion is good enough for this!

votdev · 2022-09-16T12:00:10Z

jenkins retest this please

src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html

Pegonzal · 2022-09-21T12:12:44Z

@torchiaf make check is failing due to linting issues, could you fix that please?

[lint:tslint ] TSLint's support is discontinued and we're deprecating its support in Angular CLI.
[lint:tslint ] To opt-in using the community driven ESLint builder, see: https://github.com/angular-eslint/angular-eslint#migrating-an-angular-cli-project-from-codelyzer-and-tslint.
[lint:html ]
[lint:html ] [htmllint] found 0 errors out of 166 files
[lint:html ]
[lint:html ] ERROR in /home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html
[lint:html ]
[lint:html ] src/app/ceph/shared/device-list/device-list.component.html
[lint:html ] 16:8 Attributes should vertically align
[lint:html ]
[lint:html ]
[lint:tslint ] Linting "ceph-dashboard"...
[lint:tslint ] All files pass linting.

Try running 'npm run fix' to fix some linting errors. Some errors might need a manual fix.

And also, try to keep commits squashed to have the history easier to read.

- Add life_expectancy_enabled field to /api/osd/{svc_id}/devices - Add new value 'n/a' for 'Life Expectancy' column Fixes: https://tracker.ceph.com/issues/43116 Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>

torchiaf · 2022-09-21T12:29:00Z

@torchiaf make check is failing due to linting issues, could you fix that please?

[lint:tslint ] TSLint's support is discontinued and we're deprecating its support in Angular CLI. [lint:tslint ] To opt-in using the community driven ESLint builder, see: https://github.com/angular-eslint/angular-eslint#migrating-an-angular-cli-project-from-codelyzer-and-tslint. [lint:html ] [lint:html ] [htmllint] found 0 errors out of 166 files [lint:html ] [lint:html ] ERROR in /home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html [lint:html ] [lint:html ] src/app/ceph/shared/device-list/device-list.component.html [lint:html ] 16:8 Attributes should vertically align [lint:html ] [lint:html ] [lint:tslint ] Linting "ceph-dashboard"... [lint:tslint ] All files pass linting.

Try running 'npm run fix' to fix some linting errors. Some errors might need a manual fix.

And also, try to keep commits squashed to have the history easier to read.

Hi @Pegonzal , sure, i just pushed the fix, thanks.

nizamial09 · 2022-09-23T06:38:29Z

jenkins test dashboard

torchiaf requested a review from a team as a code owner August 30, 2022 15:50

torchiaf requested review from aaSharma14 and bryanmontalvan and removed request for a team August 30, 2022 15:50

github-actions bot added dashboard pybind labels Aug 30, 2022

votdev self-requested a review August 31, 2022 07:29

votdev added this to In progress in Dashboard via automation Aug 31, 2022

votdev moved this from In progress to Review in progress in Dashboard Aug 31, 2022

torchiaf force-pushed the fix/43116-2 branch from 443588c to 23f85cc Compare August 31, 2022 17:50

votdev requested changes Sep 1, 2022

View reviewed changes

epuertat reviewed Sep 1, 2022

View reviewed changes

torchiaf force-pushed the fix/43116-2 branch 2 times, most recently from 7dbec7a to d3f40f0 Compare September 6, 2022 08:11

torchiaf force-pushed the fix/43116-2 branch from d3f40f0 to fdb78b2 Compare September 15, 2022 11:13

Dashboard automation moved this from Review in progress to Reviewer approved Sep 16, 2022

votdev approved these changes Sep 16, 2022

View reviewed changes

nizamial09 approved these changes Sep 19, 2022

View reviewed changes

src/pybind/mgr/dashboard/frontend/src/app/ceph/shared/device-list/device-list.component.html Outdated Show resolved Hide resolved

mgr/dashboard: Add text to empty life expectancy column

811bba7

- Add life_expectancy_enabled field to /api/osd/{svc_id}/devices - Add new value 'n/a' for 'Life Expectancy' column Fixes: https://tracker.ceph.com/issues/43116 Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>

torchiaf force-pushed the fix/43116-2 branch from 2e55bf3 to 811bba7 Compare September 21, 2022 12:27

nizamial09 requested review from pereman2 and epuertat September 23, 2022 06:38

epuertat approved these changes Sep 26, 2022

View reviewed changes

pereman2 approved these changes Sep 26, 2022

View reviewed changes

nizamial09 merged commit 395daf2 into ceph:main Sep 27, 2022
11 of 13 checks passed

Dashboard automation moved this from Reviewer approved to Done Sep 27, 2022

nizamial09 mentioned this pull request Sep 27, 2022

quincy: mgr/dashboard: Add text to empty life expectancy column #48271

Merged

torchiaf mentioned this pull request Sep 28, 2022

pacific: mgr/dashboard: Add text to empty life expectancy column #48276

Merged

mgr/dashboard: Add text to empty life expectancy column #47876

mgr/dashboard: Add text to empty life expectancy column #47876

torchiaf commented Aug 30, 2022

torchiaf commented Aug 31, 2022

torchiaf commented Aug 31, 2022

votdev Sep 1, 2022

epuertat Sep 1, 2022

epuertat Sep 1, 2022

pereman2 Sep 3, 2022

votdev Sep 8, 2022

votdev Sep 12, 2022 •

edited

pereman2 Sep 12, 2022

torchiaf Sep 12, 2022

torchiaf Sep 15, 2022

epuertat Sep 1, 2022

torchiaf Sep 1, 2022 •

edited

epuertat Sep 1, 2022

torchiaf Sep 1, 2022 •

edited

votdev Sep 2, 2022

epuertat Sep 5, 2022

votdev commented Sep 16, 2022

Pegonzal commented Sep 21, 2022

torchiaf commented Sep 21, 2022

nizamial09 commented Sep 23, 2022

	def send_command(srv_type, prefix, srv_spec='', **kwargs):
	# type: (str, str, Optional[str], Any) -> Any
	"""
	:type prefix: str
	:param srv_type: mon \|
	:param kwargs: will be added to argdict
	:param srv_spec: typically empty. or something like "<fs_id>:0"

	:raises PermissionError: See rados.make_ex
	:raises ObjectNotFound: See rados.make_ex
	:raises IOError: See rados.make_ex
	:raises NoSpace: See rados.make_ex
	:raises ObjectExists: See rados.make_ex
	:raises ObjectBusy: See rados.make_ex
	:raises NoData: See rados.make_ex
	:raises InterruptedOrTimeoutError: See rados.make_ex
	:raises TimedOut: See rados.make_ex
	:raises ValueError: return code != 0
	"""
	argdict = {
	"prefix": prefix,
	"format": "json",
	}
	argdict.update({k: v for k, v in kwargs.items() if v is not None})
	result = CommandResult("")
	mgr.send_command(result, srv_type, srv_spec, json.dumps(argdict), "")
	r, outb, outs = result.wait()
	if r != 0:
	logger.error("send_command '%s' failed. (r=%s, outs=\"%s\", kwargs=%s)", prefix, r,
	outs, kwargs)

	raise SendCommandError(outs, prefix, argdict, r)

	try:
	return json.loads(outb or outs)
	except Exception: # pylint: disable=broad-except
	return outb

	@Pipe({
	name: 'notAvailable'
	})
	export class NotAvailablePipe implements PipeTransform {
	transform(value: any, text?: string): any {
	if (value === '') {
	return _.defaultTo(text, $localize`n/a`);
	}
	return value;
	}
	}

	<ng-template #lifeExpectancy
	let-value="value">
	<span *ngIf="!value.life_expectancy_enabled"
	i18n>N/A</span>
	<span *ngIf="value.min && !value.max">> {{value.min \| i18nPlural: translationMapping}}</span>
	<span *ngIf="value.max && !value.min">< {{value.max \| i18nPlural: translationMapping}}</span>
	<span *ngIf="value.max && value.min">{{value.min}} to {{value.max \| i18nPlural: translationMapping}}</span>
	</ng-template>

mgr/dashboard: Add text to empty life expectancy column #47876

mgr/dashboard: Add text to empty life expectancy column #47876

Conversation

torchiaf commented Aug 30, 2022

Contribution Guidelines

Checklist

torchiaf commented Aug 31, 2022

torchiaf commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

votdev Sep 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torchiaf Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torchiaf Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

votdev commented Sep 16, 2022

Pegonzal commented Sep 21, 2022

torchiaf commented Sep 21, 2022

nizamial09 commented Sep 23, 2022

votdev Sep 12, 2022 •

edited

torchiaf Sep 1, 2022 •

edited

torchiaf Sep 1, 2022 •

edited