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

Added resource quota warning message in topology and add page #11962

Merged

Conversation

lokanandaprabhu
Copy link
Contributor

@lokanandaprabhu lokanandaprabhu commented Aug 18, 2022

JIRA:
https://issues.redhat.com/browse/ODC-6771

Solution Description:

  1. If any resource quota is reached, then warning alert will be shown on top of topology and Add page along with link to view the details/list of RQ's.
  2. If resource reached quota in single resource quota, then on click of link, user will be redirected to details page of RQ or else user will be redirected to list page of Resource Quotas.
  3. Added status column in ResourceQuotas and AppliedClusterResourceQuota pages.

Screen shots / Gifs for design review:

new-RQ-warning-alert.mov

---CHROME screenshots---TOPOLOGY PAGE---

Screenshot 2022-08-29 at 8 14 42 PM

Screenshot 2022-08-29 at 8 16 01 PM

Screenshot 2022-08-29 at 8 17 12 PM

Screenshot 2022-08-29 at 8 13 36 PM

Screenshot 2022-08-29 at 8 30 03 PM

----FIREFOX screenshots----TOPOLOGY PAGE---

Screenshot 2022-08-29 at 8 31 17 PM

Screenshot 2022-08-26 at 12 58 01 PM

---CHROME screenshots---ADD PAGE---

Screenshot 2022-08-29 at 8 42 59 PM

Screenshot 2022-08-29 at 8 43 11 PM

----FIREFOX screenshots----ADD PAGE---

Screenshot 2022-08-29 at 8 43 26 PM

Screenshot 2022-08-29 at 8 43 36 PM

Steps to create Applied cluster resource quota:

  1. Login as developer user
  2. Create a project
    $ oc new-project developer-test
  3. cluster admin creates clusterresourcequota and targets projects created by user 'developer'

$ oc create clusterquota for-user --project-annotation-selector openshift.io/requester=developer --hard pods=10 --hard secrets=20

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/kind feature

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 18, 2022
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dev-console Related to dev-console component/topology Related to topology kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Aug 18, 2022
@lokanandaprabhu
Copy link
Contributor Author

/cc @divyanshiGupta

@lokanandaprabhu
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

If any resource quota is reached, then warning alert will be shown on top of topology and Add page along with link to view the details/list of RQ's.

We should be a little careful about this because often you are at quota for something and it's expected and normal (particularly for object counts). It might not be something you can/want to change. If we always show a warning, it could permanently show up for users of those projects. Minimally I think we need to let users dismiss the alert.

This is what we had in 3.x for reference: openshift/origin-web-console#2116

cc @serenamarie125

</Alert>
) : null}
<AddPageLayout title={t('devconsole~Add')} />
</>
Copy link
Contributor

@divyanshiGupta divyanshiGupta Aug 19, 2022

Choose a reason for hiding this comment

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

@lokanandaprabhu I will suggest that you extract the code from this file and create a separate ResourceQuotaAlert component and use that in Add/Topology pages so that we can avoid duplicating code.

@serenamarie125
Copy link
Contributor

Having a notification would be so much better, unfortunately we still don't have a notification drawer for non-cluster admins 😢

Regarding the alert being dismissable - @spadgett I had a couple of concerns there. If it is dismissable, do we need to remember that per user per session per "quota"? How would they discover that this is indeed still an issue if they return to the project a week later?

@serenamarie125
Copy link
Contributor

@andrew-ronaldson FYI this what we discussed this morning. Would be great to get your feedback on the format of the inline notification as well!

@lokanandaprabhu lokanandaprabhu force-pushed the feature/ODC6771 branch 2 times, most recently from 9a1f97d to d069965 Compare August 23, 2022 08:51
@divyanshiGupta
Copy link
Contributor

@lokanandaprabhu code looks good to me. Once we have a +1 for this PR from PM/UX side will add lgtm.

cc: @serenamarie125

@serenamarie125
Copy link
Contributor

serenamarie125 commented Aug 23, 2022

As discussed in our ODC call, would like to see the following updates:

  1. Allow alerts to be dismissable ( remember per user per RQ for the active session )
  2. Change format of alert to be a single line

@spadgett
Copy link
Member

If we make them dismissable, we should remember not just the ResourceQuota, but the specific resource in that quota which was at limit. Related to this, it would be good to show specifically what resource is at quota in the alert (at least if there's only one?).

I don't know how much value making it per session brings if it comes back every time I login :/

How would they discover that this is indeed still an issue if they return to the project a week later?

That's the thing: it might not actually be a problem. Maybe this should be an info alert or displayed some other way that is still visible, but a little less strong?

@serenamarie125
Copy link
Contributor

@spadgett curious why you're saying it shouldn't be a warning. All of the RQ information which is already surfaced in the Project Dashboard is warning level, so we were surfacing it at a higher level.

Related to this, it would be good to show specifically what resource is at quota in the alert (at least if there's only one?).
Do you mean in the text of the alert? @lokanandaprabhu if this is possible I could see this being advantageous.

I don't know how much value making it per session brings if it comes back every time I login :/
We thought we would start with this, an enhance as needed going forward based on customer feedback. We have quite a number of complaints that we are not being transparent in these situations. If we allow users to dismiss these forever, i'm not sure how that helps with discoverability/awareness/transparency of the issue.

@spadgett
Copy link
Member

@spadgett curious why you're saying it shouldn't be a warning. All of the RQ information which is already surfaced in the Project Dashboard is warning level, so we were surfacing it at a higher level.

I guess I see a warning as "you should probably fix this." In the case of quota, if I'm allowed 1 deployment and I'm using that 1 deployment, is it really a problem? Is it something I really need to fix? Or is it more of an "FYI, this is OK, but you won't be able to create more."

On the other hand if I'm importing from a git repo and I can't create any more deployments, then we might warn the user before they get too far ("what you're about to try won't work...").

Do you mean in the text of the alert?

Yeah. "You have reached your pods quota (2/2) for this namespace" or something like that with a link to the quota resource.

@lokanandaprabhu
Copy link
Contributor Author

@serenamarie125 @spadgett @jerolimov @invincibleJai @divyanshiGupta ,

Hi team,

I updated the PR to add label instead of Alert. PTAL. Thanks.

@serenamarie125
Copy link
Contributor

I like this approach so much better! I do have one more question ... I would have thought we'd be using medium and large spacers, but it looks like we are using --pf-global--spacer--xl ?

Also, the spacing to the right of the GOLD label on the Overview page looks larger than on Topology. Would you mind verifying that they are the same on each page? Thanks!

@lokanandaprabhu
Copy link
Contributor Author

link

Hi @serenamarie125 ,

For the Add page, we have decided to use (existing gap + 8px), the existing gap is 24px, so I added 32 px which is --pf-global--spacer--xl . If I made this to 24px, then it looks like the same grouping, that is the reason we thought of extra 8px add on to existing spacing.

For Topology page, we have decided to go for 24px.

@lokanandaprabhu
Copy link
Contributor Author

/retest

@divyanshiGupta
Copy link
Contributor

@lokanandaprabhu Please run yarn i18n and push the changes.

import { getUsedPercentage } from '@console/app/src/components/resource-quota/utils';
import { ResourceQuotaModel } from '../../../../../public/models';

export const checkQuotaLimit = (resourecQuota: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

can we have type here? and as it's a list so can have it as plural resourecQuotas

and will be good to add unit test for checkQuotaLimit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here resourecQuotas will be either ClusterResourceQuotaKind or ResourceQuotaKind, I tried to add union of these two types but getting below error

Each member of the union type '(<U>(callbackfn: (value: ClusterResourceQuotaKindProps, index: number, array: ClusterResourceQuotaKindProps[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: ResourceQuotaKindProps, index: number, array: ResourceQuotaKindProps[]) => U, thisArg?: any) => U[])' has signatures, but none of those signatures are compatible with each other.

When I check about the error, array.map does not correctly handles the union types . https://stackoverflow.com/questions/49510832/how-to-map-over-union-array-type

I tried the solution provided but error still remains.

Any feedback on this will be helpful.

@lokanandaprabhu
Copy link
Contributor Author

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022
Comment on lines 1 to 3
@topology @add-flow @ODC6771
Feature: Update user in topology and add flow if Quotas has been reached in a namespace
If any resource reached resource quota limit, a warning alert will be displayed for the user in Add page and Topology page.
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature for topology and Add page needs to be separated in different packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated e2e tests.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022
@lokanandaprabhu
Copy link
Contributor Author

/retest

@sanketpathak
Copy link
Contributor

Tested on cluster-bot, works as expected
Screenshot from 2022-09-12 18-23-55
Screenshot from 2022-09-12 18-23-26
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 12, 2022
@sanketpathak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyanshiGupta, invincibleJai, lokanandaprabhu, sanketpathak, serenamarie125

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

@divyanshiGupta
Copy link
Contributor

/label px-approved
/label docs-approved

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Sep 13, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 13, 2022

@lokanandaprabhu: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 6016875 into openshift:master Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR kind/feature Categorizes issue or PR as related to a new feature. 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. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants