Skip to content
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: rbd striping setting pre-population and pop-over #46014

Merged
merged 1 commit into from Jul 14, 2022

Conversation

vrushch
Copy link
Contributor

@vrushch vrushch commented Apr 24, 2022

Pre-populating the stripe count to 1 (now it's empty). "1" means no "fancy striping", anything else enables the fancy striping.
Adding a pop-over explaining each setting for striping (object size, stripe unit and stripe count).

New default value to 1

image

New helper explaining the RBD striping features

image

Fixes: https://tracker.ceph.com/issues/39726
Signed-off-by: Vrushal Chaudhari vrushalcs@gmail.com

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)
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

@vrushch vrushch requested a review from a team as a code owner April 24, 2022 20:46
@vrushch vrushch requested review from avanthakkar and epuertat and removed request for a team April 24, 2022 20:46
@epuertat epuertat added this to In progress in Dashboard via automation Apr 25, 2022
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! @vrushch

You need to fix the make-check error though

[lint:html    ] src/app/ceph/block/rbd-form/rbd-form.component.html: line 282, col NaN, Tabs not allowed
[lint:html    ] src/app/ceph/block/rbd-form/rbd-form.component.html: line 307, col NaN, Tabs not allowed
[lint:html    ] 
[lint:html    ] [htmllint] found 2 errors out of 163 files
[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:prettier] src/app/ceph/block/rbd-form/rbd-form.component.ts
[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.

npm run fix should solve that.

@@ -262,7 +262,7 @@
<div class="form-group row">
<label i18n
class="cd-col-form-label"
for="size">Object size</label>
for="size">Object size<cd-helper>Object Size: Objects in the Ceph Storage Cluster have a maximum configurable size (e.g., 2MB, 4MB, etc.). The object size should be large enough to accommodate many stripe units, and should be a multiple of the stripe unit.</cd-helper></label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emphasizing on some of the important words and making it bold will make it easier for someone to read it better I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nizamial09 ,
I have fixed the linting issue now. will check about making some important words bold in pop-over

Dashboard automation moved this from In progress to Reviewer approved Apr 25, 2022
@vrushch
Copy link
Contributor Author

vrushch commented Apr 26, 2022

Hi @epuertat ,
Now as we are pre-populating Stripe Count to 1. This makes the selection of Stripe Unit compulsory. (we are currently not pre-populating Stripe Unit field).
From User perspective this means, everytime user wants to create a RBD, user will need to click on Advanced and select Stripe Unit.

I tried creating RBD with different size without this PR, I observed Stripe Unit defaults to 4 MiB when Stripe Count is not given.
should I make change to pre-populate Stripe Unit to 4MiB?

@epuertat
Copy link
Member

Hi @epuertat , Now as we are pre-populating Stripe Count to 1. This makes the selection of Stripe Unit compulsory. (we are currently not pre-populating Stripe Unit field). From User perspective this means, everytime user wants to create a RBD, user will need to click on Advanced and select Stripe Unit.

I tried creating RBD with different size without this PR, I observed Stripe Unit defaults to 4 MiB when Stripe Count is not given. should I make change to pre-populate Stripe Unit to 4MiB?

Hi @vrushch, I tested this and it looks nice (I added a few screenshots to the PR description).

Yes, we need to ensure that the RBD image can be created without the user opening the Advanced section. In fact, now when a user clicks "Create" nothing happens and the user doesn't receive any feedback (unless they expand the Advanced section).

So yes, if we preset the striping size to 1, we need also to ensure that we provide defaults for the other fields.

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this contribution, @vrushch ! It's almost ready to be merged, but it needs first to deal with the stripe unit issue, and I left some suggestions for improvement on the helper messages.

Dashboard automation moved this from Reviewer approved to Review in progress May 10, 2022
@vrushch
Copy link
Contributor Author

vrushch commented May 10, 2022

Hi @epuertat ,
I have pushed changes to remove redundant information in pop-over at all 3 places.
for stripe unit issue, I have made stripe unit value default to 4 MiB. (referenced It by creating number of RBD's with different sizes while keeping Object Size to default 4 MiB , Everytime stripe unit got set to 4 MiB)

Please review and suggest changes if any.
Thanks again for taking time to review and giving review comments

@djgalloway djgalloway changed the base branch from master to main May 25, 2022 20:00
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@nizamial09
Copy link
Member

@vrushch could you please rebase the PR, thanks

Pre-populating the stripe count to 1 (now it's empty). "1" means no "fancy striping", anything else enables the fancy striping.
Adding a pop-over explaining each setting for striping (object size, stripe unit and stripe count).

Fixes: https://tracker.ceph.com/issues/39726
Signed-off-by: Vrushal Chaudhari <vrushalcs@gmail.com>
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @vrushch for your contribution here!

Dashboard automation moved this from Review in progress to Reviewer approved Jun 30, 2022
@pereman2
Copy link
Contributor

jenkins test dashboard

@pereman2
Copy link
Contributor

jenkins test dashboard cephadm

1 similar comment
@pereman2
Copy link
Contributor

jenkins test dashboard cephadm

@pereman2 pereman2 moved this from Reviewer approved to Ready-to-merge in Dashboard Jul 14, 2022
@pereman2 pereman2 merged commit 45ecfc6 into ceph:main Jul 14, 2022
Dashboard automation moved this from Ready-to-merge to Done Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
5 participants