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 RBD QoS #25233

Merged
merged 2 commits into from Feb 19, 2019

Conversation

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Nov 25, 2018

retest this please

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from a8bca4e to ce24065 Dec 5, 2018

@callithea callithea removed the needs-rebase label Dec 5, 2018

@callithea

This comment has been minimized.

Copy link
Member

callithea commented Dec 5, 2018

screenshot_20181205_151849
IMHO the info text is a bit hard to read in that format - maybe it can be formatted so it looks "prettier"? :)

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch 3 times, most recently from ccfa3f0 to f55600a Dec 5, 2018

@p-na

This comment has been minimized.

Copy link
Contributor Author

p-na commented Dec 7, 2018

IMHO the info text is a bit hard to read in that format - maybe it can be formatted so it looks "prettier"? :)

Yep, that's something I struggled with at the beginning and therefore postponed it, but now I addressed it after your friendly reminder. The result can be seen in the screen shots added in the PR description.

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from f55600a to ad59baf Dec 7, 2018

@p-na p-na requested review from tspmelo , Devp00l and ricardoasmarques Dec 7, 2018

@p-na p-na removed the DNM label Dec 7, 2018

@p-na p-na changed the title [WIP] mgr/dashboard: Add support for managing RBD QoS mgr/dashboard: Add support for managing RBD QoS Dec 7, 2018

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from ad59baf to 7bea19d Dec 7, 2018

@trociny

This comment has been minimized.

Copy link
Contributor

trociny commented Dec 7, 2018

I am not sure we want to configure schedule tick minimum per image (on this screen). This rather looks like a technical param. @dillaman what do you think?

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Dec 7, 2018

Yeah, I would think the schedule tick minimum is more of an advanced option that the average (and even above-average) users would never touch.

Other comments:

  • Perhaps having RBD-specific tuning parameters on the OSD pool edit page might be confusing? One possible suggestion would be to add a "Pool" drop-down to the Block "Image" page's table. When the dashboard eventually gets support for namespaces, a second "Namespace" drop-down could be added as well. Then, once the table is per-pool, an "Edit QoS" action could be added.

  • Thinking longer-term, as an end-user, I would hate to have to keep copying these QoS settings from image-to-image. Instead, I would think it would be better to pre-define QoS levels (e.g. gold, silver, bronze) and the pool QoS defaults and image QoS override would just allow you to pick from one of the pre-defined overrides. The profiles could be serialized into the mon's config store and we can add a new librbd configuration option for "rbd_qos_profile_name" that could be applied to the pool/image and would load the values from the mon config store if set.

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch 2 times, most recently from bbdbc14 to e66e963 Dec 7, 2018

@p-na

This comment has been minimized.

Copy link
Contributor Author

p-na commented Dec 7, 2018

Yeah, I would think the schedule tick minimum is more of an advanced option that the average (and even above-average) users would never touch.

@trociny @dillaman I've removed that option, thanks for the hint!

  • Thinking longer-term, as an end-user, I would hate to have to keep copying these QoS settings from image-to-image. Instead, I would think it would be better to pre-define QoS levels (e.g. gold, silver, bronze) and the pool QoS defaults and image QoS override would just allow you to pick from one of the pre-defined overrides. The profiles could be serialized into the mon's config store and we can add a new librbd configuration option for "rbd_qos_profile_name" that could be applied to the pool/image and would load the values from the mon config store if set.

@dillaman I like the idea very much and therefore created the following issue. I hope you don't mind that I've just copied the majority of your suggestions' text.

mgr/dashboard: Implement profiles for RBD QoS management

@p-na

This comment has been minimized.

Copy link
Contributor Author

p-na commented Jan 2, 2019

@dillaman

Perhaps having RBD-specific tuning parameters on the OSD pool edit page might be confusing? One possible suggestion would be to add a "Pool" drop-down to the Block "Image" page's table. When the dashboard eventually gets support for namespaces, a second "Namespace" drop-down could be added as well. Then, once the table is per-pool, an "Edit QoS" action could be added.

I've put some thought into this and I think, that you're right and users will be confused about having the options on pool edit or create, although others already familiar with Ceph might look for it at the correct place.
But I think those options belong there logically. The pool is the logical parent of the RBD image and options are inherited from pools. Hence, I'd like to keep the settings on the pool create/edit pages. But as I said, I think you're right and we should also enable users to change those settings comfortably. Using Angular it shouldn't be too much effort to take the same form and display it additionally elsewhere. That way we've got both covered, beginners and experienced (CLI) users.

If that's fine for everybody, I'd like to add that after this PR has been merged, as I would prefer to have the functionality in first and then enhance the implementation.

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch 2 times, most recently from 1948caa to 51b2baa Jan 10, 2019

@s0nea s0nea added needs-qa and removed wip-tdehler-testing labels Feb 7, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch 2 times, most recently from 7a0f7c7 to 7e11bf4 Feb 7, 2019

@p-na p-na removed the needs-rebase label Feb 7, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from 7e11bf4 to 456e7ef Feb 12, 2019

@s0nea s0nea removed the needs-rebase label Feb 12, 2019

@s0nea

This comment has been minimized.

@p-na p-na added the needs-rebase label Feb 14, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from 28a3a25 to d31d70d Feb 14, 2019

@p-na p-na removed the needs-rebase label Feb 14, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from d31d70d to fe6f166 Feb 14, 2019

@LenzGr LenzGr added needs-rebase and removed needs-qa labels Feb 14, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from fe6f166 to 0e6fc19 Feb 15, 2019

@p-na p-na added needs-qa and removed needs-rebase labels Feb 15, 2019

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch 2 times, most recently from 92676ca to 2f5927e Feb 18, 2019

@s0nea

s0nea approved these changes Feb 18, 2019

Copy link
Member

s0nea left a comment

LGTM

p-na added some commits Oct 29, 2018

mgr/dashboard: Add support for managing RBD QoS
Fixes: http://tracker.ceph.com/issues/36191

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>
mgr/dashboard: Render error in pool edit dialog
Fixes: http://tracker.ceph.com/issues/38004

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

@p-na p-na force-pushed the p-na:wip-pna-qos-pr branch from 2f5927e to d93b309 Feb 19, 2019

@LenzGr LenzGr merged commit 84d077a into ceph:master Feb 19, 2019

6 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
ceph dashboard tests ceph dashboard tests succeeded
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