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: Create OSDs section in cluster creation wizard #42583
mgr/dashboard: Create OSDs section in cluster creation wizard #42583
Conversation
6503c4d
to
8a3fa0c
Compare
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.ts
Outdated
Show resolved
Hide resolved
4e6abf1
to
b53698b
Compare
jenkins test make check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, I don't see a point in showing the OSD List table. OSD's wont be showing up in that table unless it is added to the cluster. And osds are only added only after adding the host (In our case, after exiting the hosts out of maintenance/removing the _no_schedule
label).
My suggestion will be to use the osd-creation-form instead of the list. The whole point of showing this list is to preview the user of the output and we get that information from the osd-creation-form itself as a preview.
7a5cafb
to
9e4add7
Compare
jenkins test dashboard cephadm |
I think osds can be created in _no_schedule mode. @avanthakkar Do you have any idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for hijacking this PR, but while testing it the following things come to my mind:
- There is too much
Expand Cluster
shown on this page. The breadcrumb in the upper left corner should be removed, it is pretty useless here on this page. - Change
Expand Cluster
in the red button toContinue
orStart
.
- IMO
Expand Cluster
is also too much mentioned here. Remove the breadcrumb, it is also pretty useless here. I'm aware of that all pages display the breadcrumb in Ceph Dashboard, but IMO a wizard is something different than a regular page.
- I suggest to rename the red
Expand cluster
button toFinish
orApply
.
<h4 class="title" | ||
i18n>Create OSDs</h4> | ||
<br> | ||
<cd-osd-list [clusterCreation]="true"></cd-osd-list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@votdev In the particular case I think that the title is helpful as yo can easily overlook the left column.
@votdev @aaSharma14 These comments are being followed up on this PR. #42557 |
d259061
to
c7bda38
Compare
...frontend/cypress/integration/orchestrator/workflow/03-create-cluster-create-osds.e2e-spec.ts
Outdated
Show resolved
Hide resolved
ac113a2
to
8b4acdc
Compare
c380bcb
to
05379c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! One thing I noticed is that after selecting the devices in the create osd section, I click next and came back to Previous step to see that the osd device preview is gone. But the drive-group spec is kept and previously selected storage information is still shown in Review Section. (If I change it and select new spec, it gets updated too). And it even created OSDs as expected. So only a minor thing to improve.
Thank you for addressing all my reviews. Great work @aaSharma14
src/pybind/mgr/dashboard/frontend/src/app/shared/services/wizard-steps.service.ts
Outdated
Show resolved
Hide resolved
e527bc7
to
a70aa71
Compare
class="bold">Storage Capacity</td> | ||
<td><span i18n | ||
*ngIf="filteredDevices && capacity">Number of devices: {{ filteredDevices.length }}. Raw capacity: | ||
{{ capacity | dimlessBinary }}.</span></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right to me. Maybe just Storage capaticy: x GiB
or acc. to @pcuzner suggestion
"just use a table showing columns for hdd, flash showing sum for the total capacity and drive count per type - so you can do a quick visual check "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Something like this may work too: 2xHDD xGiB, 3xFlash yGiB
what u think? @nizamial09 @epuertat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could work. Also I believe you are taking care of this in a separate PR right @avanthakkar ? So its best to do it separately on that PR where you are following up on the review page (#42488) because I believe this PR has went through a lot to get here. Does that works for you @avanthakkar ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I like separating them into different columns better. Its more visually catchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could work. Also I believe you are taking care of this in a separate PR right @avanthakkar ? So its best to do it separately on that PR where you are following up on the review page (#42488) because I believe this PR has went through a lot to get here. Does that works for you @avanthakkar ?
I've already introduced the review section changes in my current PR for gather facts itself. But yeah I agree some minor things can be introduced in later PR, maybe having a general expand cluster workflow remaining cleanups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working fine. One thing that worries me is when adding primary devices you have to select them with filters. I don't remember if this was discussed or something, but why isn't there a way to select with just a click in addition of using filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and works perfect! Great job @aaSharma14 @nizamial09 !!
Yup, I am actually trying to propose another design for it. Next week or so I'll do one. |
c1ad477
to
35c5aae
Compare
jenkins test dashboard cephadm |
1 similar comment
jenkins test dashboard cephadm |
35c5aae
to
4a721eb
Compare
jenkins test api |
Create OSDs section in cluster creation wizard Fixes: https://tracker.ceph.com/issues/51991 Signed-off-by: Aashish Sharma <aasharma@redhat.com>
4a721eb
to
223c91a
Compare
jenkins test dashboard cephadm |
@epuertat, cephadm dashboard test failure is because of https://tracker.ceph.com/issues/52407 Locally, along with the fix from Sebastian, its passing.
|
Create OSDs section in cluster creation wizard
Fixes: https://tracker.ceph.com/issues/51991
Signed-off-by: Aashish Sharma aasharma@redhat.com
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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox