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.

Sign up for GitHub

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

Jump to bottom

Add compression card to pool dashboard #9507

Merged
merged 1 commit into from Jul 25, 2021
Merged

Add compression card to pool dashboard #9507

merged 1 commit into from Jul 25, 2021

Conversation

aruniiird
Copy link
Contributor

Adding compression card to pool dashboard, that means this PR is on top of @GowthamShanmugam 's PR#9423.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@openshift-ci openshift-ci bot requested review from bipuladh and jhadvig July 14, 2021 13:37
@openshift-ci openshift-ci bot added component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jul 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

Hi @aruniiird. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 14, 2021
@aruniiird
Copy link
Contributor Author

@GowthamShanmugam , @afreen23 , @bipuladh , @cloudbehl , please take a look.

GowthamShanmugam
GowthamShanmugam reviewed Jul 15, 2021
View reviewed changes
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated Show resolved Hide resolved
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated Show resolved Hide resolved
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated Show resolved Hide resolved
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated Show resolved Hide resolved
frontend/packages/ceph-storage-plugin/src/queries/ceph-queries.ts Outdated
'|',
)}'}`,
[StorageDashboardQuery.POOL_COMPRESSION_RATIO]: `((ceph_pool_compress_under_bytes / ceph_pool_compress_bytes_used > 0) and on(pool_id) (((ceph_pool_compress_under_bytes > 0) / ceph_pool_stored_raw) * 100 > 0.5)) * on (pool_id) group_left(name)ceph_pool_metadata{name=~'${poolNames.join(
'|',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering these queries are correct, Please check with @anmolsachan about these queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked with @anmolsachan . Queries generally look good. But as per @anmolsachan , some general changes (which is general for all the querries, not just for compression querries), we may have to add prometheus recording rules, but on a later stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

@GowthamShanmugam
Copy link
Contributor

Please resolve the conflicts and add some screenshots also

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from d6d297b to 857c75c Compare July 16, 2021 10:08
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2021
@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from 857c75c to 20c0372 Compare July 16, 2021 11:11
@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch 3 times, most recently from 4345738 to f9ba66b Compare July 19, 2021 10:37
@GowthamShanmugam
Copy link
Contributor

Please share some screenshot here

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from f9ba66b to aa41226 Compare July 19, 2021 16:53
afreen23
afreen23 suggested changes Jul 20, 2021
View reviewed changes
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated Show resolved Hide resolved
...ackages/ceph-storage-plugin/src/components/block-pool/dashbaord/compression-details-card.tsx Outdated
{compressionEnabled && (
<>
<DetailItem isLoading={loading} title={t('ceph-storage-plugin~Storage efficiency')}>
{''}
Copy link
Contributor

Choose a reason for hiding this comment

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

why empty ?
can you share screenshot as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compression card itself

Screenshot from 2021-07-21 13-20-35

Compression card in the dashboard page

Screenshot from 2021-07-21 13-19-40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GowthamShanmugam , @afreen23 , please take a look at the screen shot

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch 3 times, most recently from 963e40b to f8bcd80 Compare July 21, 2021 10:48
@aruniiird
Copy link
Contributor Author

aruniiird commented Jul 23, 2021 •
edited

Looks better than the last one but there are still couple of issues

* There is a larger gap between Storage efficiency and compression eligibility.

Because in the mock-up there is a larger gap. Please see the side by side image below.

  If we check above there is not that much difference in the Compression status and enabled. They both should be uniform

* The heading should be larger than the contents. `Storage efficiency` and `Compression status` should have larger text size but here it looks the same and even smaller than its body.

Please see the mock-up, heading/title is not that bigger than the value. Heading is just bold.

* There is a large amount of space after 2nd columns in storage efficiency. I would suggest to add larger margin to move it at the end of the card.

True, not an expert here, will try to push the value part to the end of the card.

............. Mockup Card ...................................................................... Actual Card ..........

Screenshot from 2021-07-23 23-59-11

PS: Mock-up card's screenshot taken from the following link: https://marvelapp.com/prototype/64gfhgf/screen/80819536

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from 304c28a to a6591ab Compare July 23, 2021 19:40
@aruniiird
Copy link
Contributor Author

@vbnrh , I have updated the code and here are the screen shots,

........... Mockup Card ......................................................... Actual Card ..................

Screenshot from 2021-07-24 01-15-28

Over all picture

Screenshot from 2021-07-24 01-00-30

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from a6591ab to 9abe54f Compare July 24, 2021 10:15
@aruniiird
Copy link
Contributor Author

Addressed all the conflicts and rebased to the latest upstream master.
@afreen23 , @vbnrh , @GowthamShanmugam , please take a look...

@afreen23
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 24, 2021
@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2021
@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from 9abe54f to dfe6805 Compare July 24, 2021 13:39
@aruniiird
Copy link
Contributor Author

@afreen23 , addressed all the review comments. Please take a look...

@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from dfe6805 to ec7358f Compare July 25, 2021 04:39
bipuladh
bipuladh suggested changes Jul 25, 2021
View reviewed changes
...ckages/ceph-storage-plugin/src/components/dashboards/block-pool/compression-details-card.tsx Outdated
{!compressionEnabled ? 'Disabled' : 'Enabled'}
</DetailItem>
</DetailsBody>
<DetailsBody>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<DetailsBody>

afreen23
afreen23 reviewed Jul 25, 2021
View reviewed changes
Copy link
Contributor

@afreen23 afreen23 left a comment

Choose a reason for hiding this comment

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

/lgtm

Please resolve #9507 (comment) in a followup PR. Functionality looks good hence tagging.

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2021
@aruniiird
Adding compression details card
37d31df 
Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
@aruniiird aruniiird force-pushed the add-compression-card-to-pool-dashboard branch from ec7358f to 37d31df Compare July 25, 2021 15:34
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2021
@aruniiird
Copy link
Contributor Author

aruniiird commented Jul 25, 2021 •
edited

@bipuladh , the suggestion provided here, #9507 (comment), if added will change the way the card look.

........Your suggestion .......................................................................... Current......................
image

Tag was added intentionally. In order to avoid some confusion, I moved the <DetailsBody> into compressionEnabled part. Let me know if you really want to remove the tag.

aruniiird
aruniiird commented Jul 25, 2021
View reviewed changes
...ckages/ceph-storage-plugin/src/components/dashboards/block-pool/compression-details-card.tsx
Comment on lines +97 to +107
<DetailsBody>
<div>
<DetailsBody>
<DetailItem isLoading={loading} title={t('ceph-storage-plugin~Storage efficiency')}>
<EfficiencyItemBody {...compressionEligibilityProps} />
<EfficiencyItemBody {...compressionRatioProps} />
<EfficiencyItemBody {...compressionSavingsProps} />
</DetailItem>
</DetailsBody>
</div>
</DetailsBody>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bipuladh , moved the <DetailsBody> into compressionEnabled true part.

@bipuladh
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, aruniiird, bipuladh, GowthamShanmugam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c45c31f into openshift:master Jul 25, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afreen23 afreen23 afreen23 requested changes

@bipuladh bipuladh bipuladh requested changes

@vbnrh vbnrh vbnrh requested changes

@GowthamShanmugam GowthamShanmugam GowthamShanmugam approved these changes

@jhadvig jhadvig Awaiting requested review from jhadvig

Assignees

@afreen23 afreen23

@bipuladh bipuladh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aruniiird @GowthamShanmugam @vbnrh @afreen23 @bipuladh @spadgett @openshift-merge-robot