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: OSD Creation Workflow initial works #45116
mgr/dashboard: OSD Creation Workflow initial works #45116
Conversation
|
jenkins test dashboard cephadm |
|
jenkins test dashboard |
|
Jenkins retest this please |
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.html
Outdated
Show resolved
Hide resolved
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
bdd11d0
to
3b4905f
Compare
3b4905f
to
f809888
Compare
f809888
to
f27ca14
Compare
|
@sunilangadi2 Can you also test this one? keep in mind as of now only the cost-capacity optimized option is testable. |
src/pybind/mgr/dashboard/frontend/cypress/integration/cluster/osds.po.ts
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/cluster.module.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/cluster.module.ts
Outdated
Show resolved
Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/create-cluster/create-cluster.component.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/shared/models/osd-deployment-options.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.ts
Outdated
Show resolved
Hide resolved
f27ca14
to
cd9e70e
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.
Is this type of behaviour expected in the accordion component?
Ceph_.Expand.Cluster.mp4
src/pybind/mgr/dashboard/frontend/src/app/shared/api/osd.service.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/shared/models/osd-deployment-options.ts
Outdated
Show resolved
Hide resolved
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.
Nice progress @nizamial09 ! More feedback from my end :D Have a nice weekend!
| OsdDeploymentOptions.COST_CAPACITY.value: | ||
| DeploymentOption(OsdDeploymentOptions.COST_CAPACITY.value), |
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.
Just a comment on code: don't feel this hard to read. First of all, there are 2 classes with very similar names (OsdDeploymentOptions and DeploymentOption), and I'd expect that both are connected (the plural "...Options" is just an list containing multiple "DeploymentOption"s), but that's not the case.
If you ever end up writing a chunk of code that is as unintuitive to read as this one, please consider that as a smell that something going wrong.
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.
Changed the function name for clarity
...ind/mgr/dashboard/frontend/src/app/ceph/cluster/create-cluster/create-cluster.component.scss
Show resolved
Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/create-cluster/create-cluster.component.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.html
Show resolved
Hide resolved
| const deploymentOptions: DeploymentOptions = { | ||
| options: { | ||
| cost_capacity: { | ||
| name: OsdDeploymentOptions.COST_CAPACITY, | ||
| available: true, | ||
| capacity: 0, | ||
| used: 0, | ||
| hdd_used: 0, | ||
| ssd_used: 0, | ||
| nvme_used: 0, | ||
| option_name: 'Cost/Capacity-optimized', | ||
| msg: 'All the available HDDs are selected' | ||
| }, |
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.
All the things mentioned for the back-end data structs apply here, and they're more evident since most of these fields are not needed in the UI (ssd_used?), only the recommended option, right? That means that we're coupling in the back-end 2 or more different data classes.
The class we'd need here is what, at back-end, I defined as:
# Named tuples allows us to quickly prototype immutable objects with initialization, defaults and asdict()
class OsdDeploymentOption(NamedTuple):
id: str
title: str
desc: strIf we need some extra data (like total capacity, or number of disks), we could append some "hints" to this class (e.g.: the getDeploymentRecommendation() service returns the recommended DeploymentOption + extras: available + capacity + etc.).
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 we decided on this in the previous PR #45190, and fields like hdd_used etc are needed on the later iterations when we try to fill the pie chart in the frontend (to visualize the storage used infos).
cd9e70e
to
8f00545
Compare
Yup, https://getbootstrap.com/docs/4.6/components/collapse/#accordion-example. But I removed the focus so we don't have a broken border when clicking it. @pereman2 |
8f00545
to
b0dfa4e
Compare
c8d034a
to
d3714c5
Compare
|
jenkins test dashboard |
| </div> | ||
| </div> | ||
| </fieldset> | ||
| <!-- DB slots --> |
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'm not sure what these extra spaces for..maybe can be removed?
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.
@avanthakkar Those are indendation changes to proper align it and it was not a mistake. If I remove it it will be misaligned with the above and below lines.
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.
@avanthakkar Those are indendation changes to proper align it and it was not a mistake. If I remove it it will be misaligned with the above and below lines.
|
@nizamial09 based on the above screenshot it looks like the Advanced mode is "additional" to the first 3 modes, while in reality it should be exclusive (either you select the simple modes or the advanced modes). Wouldn't it be better to display that in a 4th option? |
@epuertat But either the Advanced mode or the Deployment option is visible at a time. If the user clicks on ADvanced mode then the Deployment option will be hidden. (The Feature on the other hand will be always shown) |
d3714c5
to
6dc219b
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!
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-form/osd-form.component.html
Outdated
Show resolved
Hide resolved
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.
Thanks, @nizamial09 ! changes look good to me.
Introducing the Cost/Capacity Optimized deployment option Used bootstrap accordion Adapted the e2e but not written new tests for the deployment option Fixes: https://tracker.ceph.com/issues/54340 Fixes: https://tracker.ceph.com/issues/54563 Signed-off-by: Nizamudeen A <nia@redhat.com> Signed-off-by: Sarthak0702 <sarthak.0702@gmail.com>
6dc219b
to
e3fe284
Compare
|
jenkins test make check |
1 similar comment
|
jenkins test make check |
1330ffc
into
ceph:feature-54330-osd-creation-workflow

Depends on #45190
This is kind of a skeleton PR. As per the design of the feature, backend needs to provide frontend all the data's and UI will just present it to the user. So I'll wait for the backend to setup first to adapt the frontend. Currently, the frontend design is just static.
Also, the current OSD Creation is moved to the Advanced Mode Option
Introducing the Cost/Capacity Optimized deployment option
Used bootstrap accordion
Adapted the e2e but not written new tests for the deployment option
Deployment Scenarios

Advanced Mode

View from Cluster Expansion wizard

Fixes: https://tracker.ceph.com/issues/54340
Fixes: https://tracker.ceph.com/issues/54563
Signed-off-by: Nizamudeen A nia@redhat.com
Co-authored-by: Sarthak0702 sarthak.0702@gmail.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox