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

DW: Implement "Requested resources" bullet charts on Project Metrics page (formerly "Resource usage" charts) #2673

Merged

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Apr 3, 2024

Closes RHOAIENG-2844.

Description

Tech debt items we need to revisit after feature freeze are marked by ⚠️ below.

  • Adds new util getQueueRequestedResources for calculating the sum of requested resources from localQueues (for "requested by this project" metrics) and clusterQueues (for "requested by all projects") metric.
  • Adds new util getTotalSharedQuota for calculating the sum of available resources from the clusterQueue.
  • Replaces plain 'cpu' and 'memory' strings in the ClusterQueueKind and LocalQueueKind's flavorsReservation and resourceGroups with the existing ContainerResourceAttributes enum to simplify the new utilities.
  • Renames the "Resource usage" section to "Requested resources" after design discussions
  • Adds a help tooltip on this section if you are not an admin describing that you might not have access to all the described resources (via new helpTooltip prop in DWSectionCard)
  • Replaces ResourceUsage component with new RequestedResources, which implements the new bullet charts.
    • These charts are a little weird with how they handle numbers outside their range. Based on the design, we show actual values in the chart until they reach 110% of the available capacity (grey range), after which we cap the rendered values. However, we still show full-precision values in the tooltips. ⚠️ As in DW: Implement "Workload resource metrics" table on the Project Metrics tab #2658, We will likely want to revisit this full-precision display after feature freeze. We also show a warning bar when any value is 150% of the available capacity, but that warning bar is also displayed at the 110% mark because that is the edge of the chart's display area. ⚠️ We may want to circle back on this design and see if it makes sense to instead horizontally scale the chart so we aren't capping values like this, but the concern there was that if requests are wildly beyond the total quota, the actual requested values will be too small to see.
    • To make this capping and rounding behavior more manageable, the chart component has its own internal CappedBulletChartDataItem type that keeps track of the true preciseValue, roundedValue and cappedValue for each data point. This simplifies the rendering of the chart, where we need to be a little repetitive with where we pass in different forms of this data in the labels, primarySegmentedMeasureData, qualitativeRangeData, comparativeWarningMeasureData and colorScale props.
    • Data is converted to the correct units (cores and GiB) before being passed into the RequestedResourcesBulletChart component.
    • Chart sizing is funky - this looks fine on common screen sizes but the charts are too big on very wide screens. This is also true of the other existing charts on this page. ⚠️ We should revisit the chart layout code with help from PatternFly.
  • Updates mockClusterQueueK8sResource and mockLocalQueueK8sResource with params isCpuOverQuota and isMemoryOverQuota for testing purposes. Not currently in use because it doesn't affect the behavior of utils being unit tested and we don't yet have Cypress tests that assert anything about the rendered charts, but was useful for mocking things out during development. These mocks now return requested resource data by default.

Known gaps between implementation and design mockups:

  • The warning icons next to each legend item when requested resources are over the warning threshold of 150% are missing. Adding icons like this within the legend is not directly supported by PatternFly charts and would require custom SVG components. ⚠️ We may want to revisit these when time allows or consider changing the design.
  • The custom tooltip showing both "requested by this project" and "requested by all projects" values in the same tooltip that is supposed to appear only when both bars are past the maximum chart domain are difficult for similar reasons. Instead, when both bars are past the chart domain, only the "requested by this project" tooltip is reachable, but the "requested by all projects" value is still shown in the legend. ⚠️ We may want to revisit this when time allows or consider changing the design.
  • The number above the warning bar showing the actual 150% value would also require custom SVG components. ⚠️ We may want to revisit this when time allows or consider changing the design.
Screenshot 2024-04-03 at 7 04 03 PM Screenshot 2024-04-03 at 7 04 16 PM Screenshot 2024-04-03 at 7 04 27 PM Screenshot 2024-04-03 at 7 04 43 PM Screenshot 2024-04-03 at 7 01 44 PM Screenshot 2024-04-03 at 7 02 46 PM Screenshot 2024-04-03 at 7 03 14 PM

cc @xianli123 @cfullam1 @yannnz

How Has This Been Tested?

Tested the new charts by running dummy jobs provided by @Fiona-Waters. Tested that changes impacting other areas of the code have no effect on existing behavior.

Test Impact

  • Unit tests added for new utils getQueueRequestedResources and getTotalSharedQuota
  • Cypress tests updated to correctly check that the charts are rendered

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Apr 3, 2024
@mturley mturley added the do-not-merge/hold This PR is hold for some reason label Apr 3, 2024
@mturley mturley changed the title [WIP] DW: Implement "Requested resource" bullet charts on Project Metrics page (formerly "Resource usage" charts) [WIP] DW: Implement "Requested resources" bullet charts on Project Metrics page (formerly "Resource usage" charts) Apr 3, 2024
@mturley mturley force-pushed the tmp-2844-bullet-charts branch 2 times, most recently from 26d5501 to 9fe0586 Compare April 3, 2024 22:45
@dgutride dgutride added the priority/high Important issue that needs to be resolved asap. Releases should not have too many of these. label Apr 4, 2024
@mturley mturley force-pushed the tmp-2844-bullet-charts branch 2 times, most recently from dd9f6da to 5b13eec Compare April 4, 2024 14:42
@mturley
Copy link
Contributor Author

mturley commented Apr 4, 2024

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Apr 4, 2024
@mturley mturley changed the title [WIP] DW: Implement "Requested resources" bullet charts on Project Metrics page (formerly "Resource usage" charts) DW: Implement "Requested resources" bullet charts on Project Metrics page (formerly "Resource usage" charts) Apr 4, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Apr 4, 2024
…age (formerly "Resource usage" charts)

Signed-off-by: Mike Turley <[email protected]>
Comment on lines +67 to +70
isAdmin
? undefined
: 'In this section, all projects refers to all of the projects that share the specified resource. You might not have access to all of these projects.'
}
Copy link
Member

Choose a reason for hiding this comment

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

Being an admin does not mean you get access to all projects -- the two are separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... I apologize, I did learn that the hard way once already and forgot when I wrote this originally 😕 I imagine we should just show this icon/message for all users. Even if a cluster admin sees it that seems fine to me

@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Apr 5, 2024
@mturley mturley removed the do-not-merge/hold This PR is hold for some reason label Apr 5, 2024
Copy link
Contributor

openshift-ci bot commented Apr 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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-ci openshift-ci bot added the approved label Apr 5, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d2117a5 into opendatahub-io:main Apr 5, 2024
6 checks passed
@mturley mturley deleted the tmp-2844-bullet-charts branch April 5, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm priority/high Important issue that needs to be resolved asap. Releases should not have too many of these.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants