New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mgr/dashboard: Adds ECP management to the frontend #24627

Merged
merged 3 commits into from Nov 9, 2018

Conversation

Projects
None yet
8 participants
@Devp00l
Contributor

Devp00l commented Oct 17, 2018

Now you can create, delete and get information about profiles inside
the pool form.

The erasure code profile form has a lot of tooltips to guide you through
the creation. It can create profiles with different plugins.

Fixes: https://tracker.ceph.com/issues/25156
Signed-off-by: Stephan Müller smueller@suse.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
  • #24631 merged
  • #24632 merged
  • #24633 merged

This PR includes changes from #24632, #24633.

Some impressions:
createecpprofile
epc-deletion-modal
ecp-creation-modal
ecp-info

@Devp00l Devp00l added the dashboard label Oct 17, 2018

@Devp00l Devp00l force-pushed the Devp00l:wip-manage-ec-profiles branch from facbe01 to 27e3112 Oct 17, 2018

@Devp00l Devp00l referenced this pull request Oct 17, 2018

Merged

mgr/dashboard: Add left padding to helper icon #24631

1 of 3 tasks complete

@Devp00l Devp00l changed the title from mgr/dashboard: Adds ECP management to the frontend to [After #24631, #24632, #24633] mgr/dashboard: Adds ECP management to the frontend Oct 17, 2018

@sebastian-philipp

This comment has been minimized.

Member

sebastian-philipp commented Oct 17, 2018

backend looks sane to me.

@ricardoasmarques

Lack of consistency on help component

In the same form, user will have 3 different ways to get help / additional info:

  1. By clicking in the help button next to the label:
    screenshot from 2018-10-17 11-11-56

  2. By mouse over an input:
    screenshot from 2018-10-17 11-13-04

  3. By pressing a button next to an input:
    screenshot from 2018-10-17 11-11-35

From the usability point of view, shouldn't we stick to a single way to get help information (e.g. by using option 1 everywhere?)

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Oct 17, 2018

Maybe a topic for a separate PR, but would be great if we can avoid line breaks in labels to make it easier to read:

screenshot from 2018-10-17 11-44-13

@Devp00l

This comment has been minimized.

Contributor

Devp00l commented Oct 17, 2018

@ricardoasmarques I first implemented it with the first option but a table in a popover looks really ugly, but I can amend the popover for the crush rules, but I think a separate PR would suit it better.

@Devp00l Devp00l force-pushed the Devp00l:wip-manage-ec-profiles branch 2 times, most recently from 9b8131a to ca3ddb1 Oct 17, 2018

@Devp00l Devp00l changed the title from [After #24631, #24632, #24633] mgr/dashboard: Adds ECP management to the frontend to [After #24632, #24633] mgr/dashboard: Adds ECP management to the frontend Oct 19, 2018

@LenzGr LenzGr added the needs-review label Oct 19, 2018

@Devp00l Devp00l changed the title from [After #24632, #24633] mgr/dashboard: Adds ECP management to the frontend to [After #24633] mgr/dashboard: Adds ECP management to the frontend Oct 24, 2018

@Devp00l Devp00l force-pushed the Devp00l:wip-manage-ec-profiles branch 2 times, most recently from fdbd713 to ad80990 Oct 24, 2018

@Devp00l

This comment has been minimized.

Contributor

Devp00l commented Oct 26, 2018

@ricardoasmarques I amended the cursh rule info in this PR

@tspmelo tspmelo removed the needs-rebase label Oct 26, 2018

@ricardoasmarques

IMO we can improve the way we are displaying the help, but maybe we can work on this latter.

screenshot from 2018-10-26 13-51-30

Apart from this, LGTM.

@p-na

This comment has been minimized.

Contributor

p-na commented Oct 29, 2018

I agree with @ricardoasmarques . The steps in the table aren't shown very user friendly with the JSON objects inside that field, but nothing that should prevent us from merging this PR.

import { ErasureCodeProfileService } from '../../../shared/api/erasure-code-profile.service';
import { PoolService } from '../../../shared/api/pool.service';
import { DeletionModalComponent } from '../../../shared/components/deletion-modal/deletion-modal.component';

This comment has been minimized.

@votdev

votdev Oct 29, 2018

Contributor

Got the following error message when running npm start:

Date: 2018-10-29T13:27:18.008Z
Hash: 6d56854c0816f59aff3f
Time: 9670ms
chunk {main} main.js, main.js.map (main) 1.93 kB [initial] [rendered]
chunk {polyfills} polyfills.js, polyfills.js.map (polyfills) 685 bytes [initial] [rendered]
chunk {runtime} runtime.js, runtime.js.map (runtime) 5.22 kB [entry] [rendered]
chunk {scripts} scripts.js, scripts.js.map (scripts) 536 kB  [rendered]
chunk {styles} styles.js, styles.js.map (styles) 292 kB [initial] [rendered]
chunk {vendor} vendor.js, vendor.js.map (vendor) 325 kB [initial] [rendered]

ERROR in src/app/ceph/pool/pool-form/pool-form.component.ts(11,40): error TS2307: Cannot find module '../../../shared/components/deletion-modal/deletion-modal.component'.

ℹ 「wdm」: Failed to compile.

This comment has been minimized.

@votdev

votdev Oct 29, 2018

Contributor

Component has been renamed, see e8218ac#diff-89efc18d06edcfa1553c63af45784e9c

@votdev votdev added the needs-rebase label Oct 29, 2018

@LenzGr LenzGr removed the needs-rebase label Oct 29, 2018

@Devp00l Devp00l changed the title from [After #24633] mgr/dashboard: Adds ECP management to the frontend to mgr/dashboard: Adds ECP management to the frontend Nov 5, 2018

@Devp00l Devp00l force-pushed the Devp00l:wip-manage-ec-profiles branch from ad80990 to e3cc964 Nov 5, 2018

@Devp00l Devp00l added the needs-qa label Nov 5, 2018

@callithea

This comment has been minimized.

Member

callithea commented Nov 6, 2018

Unfortunately the QA tests failed: http://pulpito.ceph.com/laura-2018-11-05_17:35:24-rados:mgr-wip-lpaduano-testing-24627-distro-basic-smithi/
but I think those failures are not directly related to your PR, got this error on another PR QA run as well.

@callithea

This comment has been minimized.

Member

callithea commented Nov 7, 2018

jenkins test dashboard

@Devp00l

This comment has been minimized.

Contributor

Devp00l commented Nov 8, 2018

Addressed all comments

'failure_domains': JList(six.string_types),
'plugins': JList(six.string_types),
'devices': JList(six.string_types),
"directory": six.string_types,

This comment has been minimized.

@votdev

votdev Nov 8, 2018

Contributor

Use single quotes

osd_map_crush = mgr.get('osd_map_crush')
return {
# Because 'shec' is experimental it's not included
"plugins": config['osd_erasure_code_plugins'].split() + ['shec'],

This comment has been minimized.

@votdev

votdev Nov 8, 2018

Contributor

Use single quotes in following lines

tooltips = new ErasureCodeProfileFormTooltips();
PLUGIN = {
LRC: 'lrc', // Locally Repairable Erasure Code

This comment has been minimized.

@votdev

votdev Nov 8, 2018

Contributor

Why not LREC? Otherwise SHC would be sufficient. Or is this name used by Ceph in general?

This comment has been minimized.

@Devp00l

Devp00l Nov 9, 2018

Contributor

The name is chosen by Ceph or the authors of those plugins. It seems to me too, that they have no name convention between the plugins.

Devp00l added some commits Aug 1, 2018

mgr/dashboard: Adds ECP management to the frontend
Now you can create, delete and get information about profiles inside
the pool form.

The erasure code profile form has a lot of tooltips to guide you through
the creation. It can create profiles with different plugins.

Fixes: https://tracker.ceph.com/issues/25156
Signed-off-by: Stephan Müller <smueller@suse.com>
mgr/dashboard: Adds ECP info endpoint
The new info endpoint will provide the frontend with the necessary
information it needs to create new profiles.

Fixes: https://tracker.ceph.com/issues/25156
Signed-off-by: Stephan Müller <smueller@suse.com>
mgr/dashboard: Show info button for crush rules in pool form
Signed-off-by: Stephan Müller <smueller@suse.com>

@Devp00l Devp00l force-pushed the Devp00l:wip-manage-ec-profiles branch from e3cc964 to 3261c4f Nov 9, 2018

@Devp00l

This comment has been minimized.

Contributor

Devp00l commented Nov 9, 2018

Addressed all comments

@votdev

votdev approved these changes Nov 9, 2018

@LenzGr LenzGr merged commit 5b0a957 into ceph:master Nov 9, 2018

5 of 6 checks passed

ceph dashboard tests ceph dashboard tests failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment