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: Add support for managing individual OSD settings/characteristics in the frontend #24606

Merged
merged 5 commits into from Oct 26, 2018

Conversation

Projects
None yet
9 participants
@p-na
Contributor

p-na commented Oct 16, 2018

Adds the possibility to reweight OSDs, mark them down, out or lost and enables to destroy and remove OSDs.

opera snapshot_2018-10-19_103820_localhost

Related tasks (to be done):

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
@@ -238,4 +228,18 @@ export class CdValidators {
);
};
}
static uuid(required = false): ValidatorFn {

This comment has been minimized.

@s0nea

s0nea Oct 16, 2018

Contributor

I guess you removed the description accidentally?

This comment has been minimized.

@s0nea

s0nea Oct 16, 2018

Contributor

But nice improvement ;)

This comment has been minimized.

@p-na

p-na Oct 16, 2018

Contributor

I've been so bold to leave out the type declarations as they are already covered by TypeScript. I hope that's okay.

This comment has been minimized.

@s0nea

s0nea Oct 18, 2018

Contributor

For the sake of documentation, consistency and completeness it should be kept here, but that's only my opinion.

This comment has been minimized.

@p-na

p-na Oct 23, 2018

Contributor

I've checked the documentation tools available and they all seem to support extraction of the types directly from the code, so personally, I see no reason to document and maintain it twice (as it's already documented in the code and covered by documentation generators).
For the sake of consistency, I'd remove the types in the doc strings if we concluded to leave them out (in a follow up PR).

This comment has been minimized.

@s0nea

s0nea Oct 24, 2018

Contributor

Okay, fine with me

@p-na p-na force-pushed the p-na:osd-actions-fe branch 2 times, most recently from f3e0182 to 4cd6b59 Oct 16, 2018

@ricardoasmarques

I'm getting a "500 - Error" after creating an OSD (Performance counter?):

peek 2018-10-16 13-28

[form]="reweightForm"
[disabled]="reweightForm.invalid"
i18n>
Submit

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Oct 16, 2018

Member

IMO, action buttons must have a more meaningful name (e.g. Reweightinstead of Submit), just like you did on Mark OSD out.

screenshot from 2018-10-16 13-32-35

screenshot from 2018-10-16 13-32-48

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Oct 16, 2018

Help tooltips are very useful here, but we should try to make them uniform in look and feel, across all features.

OSD:

screenshot from 2018-10-16 14-28-08

RBD (here we are using the cd-helper component):

screenshot from 2018-10-16 14-27-55

I do not have a preference for any of them, I just argue that they should be equal.

@p-na

This comment has been minimized.

Contributor

p-na commented Oct 16, 2018

@ricardoasmarques I don't see the 500 error when I click on performance counter. Can you add some information about that problem?
screen

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Oct 16, 2018

@ricardoasmarques I don't see the 500 error when I click on performance counter. Can you add some information about that problem?

@p-na only happen on python 3 environment:

[b'{"status": "500 Internal Server Error", "detail": "The server encountered an unexpected condition which prevented it from fulfilling the request.", "traceback": "Traceback (most recent call last):\\n  File \\"/ceph/src/pybind/mgr/dashboard/controllers/perf_counters.py\\", line 18, in get\\n    schema = schema_dict[\\"{}.{}\\".format(self.service_type, service_id)]\\nKeyError: \'osd.3\'\\n\\nDuring handling of the above exception, another exception occurred:\\n\\nTraceback (most recent call last):\\n  File \\"/usr/lib/python3.6/site-packages/cherrypy/_cprequest.py\\", line 670, in respond\\n    response.body = self.handler()\\n  File \\"/usr/lib/python3.6/site-packages/cherrypy/lib/encoding.py\\", line 221, in __call__\\n    self.body = self.oldhandler(*args, **kwargs)\\n  File \\"/usr/lib/python3.6/site-packages/cherrypy/_cptools.py\\", line 237, in wrap\\n    return self.newhandler(innerfunc, *args, **kwargs)\\n  File \\"/ceph/src/pybind/mgr/dashboard/services/exception.py\\", line 88, in dashboard_exception_handler\\n    return handler(*args, **kwargs)\\n  File \\"/usr/lib/python3.6/site-packages/cherrypy/_cpdispatch.py\\", line 60, in __call__\\n    return self.callable(*self.args, **self.kwargs)\\n  File \\"/ceph/src/pybind/mgr/dashboard/controllers/__init__.py\\", line 536, in inner\\n    ret = func(*args, **kwargs)\\n  File \\"/ceph/src/pybind/mgr/dashboard/controllers/__init__.py\\", line 716, in wrapper\\n    return func(*vpath, **params)\\n  File \\"/ceph/src/pybind/mgr/dashboard/controllers/perf_counters.py\\", line 20, in get\\n    raise cherrypy.HTTPError(404, \\"{0} not found\\".format(e.message))\\nAttributeError: \'KeyError\' object has no attribute \'message\'\\n", "version": "10.2.1"}']

(you should do str(e) instead of e.message)

@p-na

This comment has been minimized.

Contributor

p-na commented Oct 16, 2018

@ricardoasmarques thanks for providing the crucial hint! As we've talked about, I created a PR to solve the issue: #24617

@@ -26,7 +26,7 @@ export class CdTableAction {
// You can define the condition to disable the action.

This comment has been minimized.

@tspmelo

tspmelo Oct 16, 2018

Contributor

Could you also fix the documentation of canBePrimary?

This comment has been minimized.

@p-na

p-na Oct 17, 2018

Contributor

Can you please be more specific?

This comment has been minimized.

@tspmelo

tspmelo Oct 17, 2018

Contributor

// You can define the condition to disable the action. is the documentation for disable.
This should express what canBePrimary is.

Show resolved Hide resolved ...shboard/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.ts
Show resolved Hide resolved src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/cluster.module.ts Outdated
Show resolved Hide resolved ...rd/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.spec.ts Outdated
Show resolved Hide resolved ...shboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.ts Outdated
Show resolved Hide resolved ...ceph/cluster/osd/osd-reweight-modal/osd-reweight-modal.component.spec.ts Outdated
Show resolved Hide resolved ...rd/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.spec.ts Outdated
Show resolved Hide resolved ...rd/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.spec.ts
Show resolved Hide resolved ...shboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.ts Outdated
Show resolved Hide resolved ...rd/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.spec.ts Outdated
@liewegas

This comment has been minimized.

Member

liewegas commented Oct 17, 2018

What is the purpose of the "osd create" button/dialog? The 'osd create' function is mostly unused these days.. osds should be created via ceph-volume, which calls 'osd new'.

@p-na

This comment has been minimized.

Contributor

p-na commented Oct 17, 2018

What is the purpose of the "osd create" button/dialog? The 'osd create' function is mostly unused these days.. osds should be created via ceph-volume, which calls 'osd new'.

Thanks for the hint! I created an issue for the OSD add feature and will remove the osd create from this PR.

@p-na p-na force-pushed the p-na:osd-actions-fe branch from 4cd6b59 to 4ec81d8 Oct 19, 2018

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

@p-na p-na force-pushed the p-na:osd-actions-fe branch from 4ec81d8 to 6ba909a Oct 22, 2018

@p-na p-na force-pushed the p-na:osd-actions-fe branch 2 times, most recently from cd71730 to 0c49b7f Oct 23, 2018

@p-na p-na removed the needs-rebase label Oct 23, 2018

@p-na p-na force-pushed the p-na:osd-actions-fe branch from 0c49b7f to 9ef829b Oct 23, 2018

@s0nea s0nea added the needs-rebase label Oct 24, 2018

@p-na p-na force-pushed the p-na:osd-actions-fe branch from 9ef829b to b0a1d3f Oct 24, 2018

p-na added some commits Oct 15, 2018

mgr/dashboard: Cleanup OSD components
Signed-off-by: Patrick Nawracay <pnawracay@suse.com>
mgr/dashboard: Rename `DeletionModalComponent` to `CriticalConfirmati…
…onModalComponent`

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>
mgr/dashboard: TableActionsComponent cleanup
Rename `buttonCondition` to `canBePrimary`

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>
mgr/dashboard: Simplify frontend test helper
Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

@p-na p-na force-pushed the p-na:osd-actions-fe branch from b0a1d3f to cacb8db Oct 24, 2018

@s0nea s0nea removed the needs-rebase label Oct 24, 2018

@capri1989

This comment has been minimized.

Contributor

capri1989 commented Oct 24, 2018

I tested the newly added functions and all of them seem to work. Also the notifications are absolutely helpful specially for remove/lost. From this perspective it looks good to me.

@Devp00l

As discussed I will create 2 RFC PRs after this PR has been merged.

providers: [
{ provide: AuthStorageService, useValue: fakeAuthStorageService },
TableActionsComponent,
OsdService,

This comment has been minimized.

@tspmelo

tspmelo Oct 24, 2018

Contributor

This is already provided in the SharedModule, you can remove it.

Show resolved Hide resolved ...rd/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.spec.ts
import { TabsModule } from 'ngx-bootstrap/tabs';
import { EMPTY, of } from 'rxjs';
import Spy = jasmine.Spy;

This comment has been minimized.

@tspmelo

tspmelo Oct 24, 2018

Contributor

Please use the following suggestion:

Suggested change Beta
import Spy = jasmine.Spy;
import { Spy } from 'jasmine.core';

This comment has been minimized.

@p-na

p-na Oct 25, 2018

Contributor

Hm, my IDE complains about that : Cannot find module jasmine.core but the test continues to work. It also turned out that the import isn't required at all. I'd prefer to not import if not required instead of having an import that's highlighted as error. What's your opinion on that?

@@ -2,7 +2,11 @@ import { Component, OnInit, TemplateRef, ViewChild } from '@angular/core';
import { BsModalRef, BsModalService } from 'ngx-bootstrap/modal';

This comment has been minimized.

@tspmelo

tspmelo Oct 24, 2018

Contributor

Please remove empty line.

configureTestBed({
imports: [ReactiveFormsModule],
declarations: [OsdReweightModalComponent, ModalComponent, SubmitButtonComponent],
providers: [OsdService, BsModalRef, HttpClient, HttpHandler, CdFormBuilder]

This comment has been minimized.

@tspmelo

tspmelo Oct 24, 2018

Contributor

Please remove HttpClient, HttpHandler and import HttpClientTestingModule.

mgr/dashboard: Add support for managing individual OSD settings/chara…
…cteristics in the frontend

Fixes: http://tracker.ceph.com/issues/35448

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

@p-na p-na force-pushed the p-na:osd-actions-fe branch from cacb8db to 7a0b4e7 Oct 25, 2018

@s0nea

s0nea approved these changes Oct 25, 2018

LGTM

@callithea

This comment has been minimized.

Member

callithea commented Oct 26, 2018

Unfortunately the QA run failed: http://pulpito.ceph.com/laura-2018-10-26_08:51:40-rados:mgr-wip-lpaduano-testing-24606-1-distro-basic-smithi/
but the errors does not seem to be related with your PR:
Test failure: test_delete_s3 (tasks.mgr.dashboard.test_rgw.RgwUserKeyTest)

@callithea

This comment has been minimized.

@callithea callithea merged commit b9e9741 into ceph:master Oct 26, 2018

5 checks passed

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

@p-na p-na deleted the p-na:osd-actions-fe branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment