Skip to content

Conversation

@rawagner
Copy link
Contributor

@rawagner rawagner commented Jun 14, 2019

This PR adds Capacity card to Dashboard Overview page. The card can be extended by replacing the default queries (ie ceph plugin will want to replace the default storage query for their own)

Loaded Capacity card
capacity_loaded

Capacity card in loading state
capacity_loading

Capacity card when prometheus is not available
capacity_na

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 14, 2019
@openshift-ci-robot
Copy link
Contributor

Hi @rawagner. 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.

Details

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.

@rawagner
Copy link
Contributor Author

@rawagner rawagner force-pushed the dashboard-capacity-2 branch 3 times, most recently from 154f69e to b41ae3a Compare June 14, 2019 12:34
@spadgett
Copy link
Member

fyi @TheRealJon is looking at transitioning our existing gauge charts to pf4/victory

@spadgett
Copy link
Member

/assign @TheRealJon

Jon, can you help review?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cloudbehl @spadgett This can be used in ceph-storage-plugin to customize storage related queries.

@jelkosz We must have the ability to gate extensions by feature flags, i.e. use Ceph storage capacity queries only if CEPH feature flag is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rawagner In relation to extension validation (#1708), are there any constraints to replacing the base Prometheus queries?

For example, if plugin A defines custom CAPACITY_QUERY.STORAGE_TOTAL, can plugin B redefine that query again?

@christianvogt Plugins are currently treated as completely independent of each other, including the order in which their extensions are applied, with one exception being that console-app's extensions always come first.

Therefore, I think we should have an extension check which fails if multiple plugins attempt to replace the same query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vojtechszocs I think it makes sense to add such test once #1708 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rawagner, I'll add that test once the infra PR #1708 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rawagner We should ensure that given queryKey is replaced at most once.

const pluginQueries = {};
plugins.registry.getDashboardsOverviewCapacityQueries().forEach(cq => {
  const queryKey = cq.properties.queryKey;
  if (!pluginQueries[queryKey]) {
    pluginQueries[queryKey] = cq.properties.query;
  }
});
return _.defaults(pluginQueries, defaultQueries);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to using _.defaults . I've also added warn log message when such case happens

Copy link
Contributor

@vojtechszocs vojtechszocs Jun 18, 2019

Choose a reason for hiding this comment

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

I've also added warn log message when such case happens

Please don't, extension tests introduced in #1708 should catch and report on these issues at test/build time.

Code I've posted above enforces the "query can be replaced at most once" behavior (guarding against e.g. when you disable some tests), but it doesn't log anything on purpose. This keeps the code simple(r) and browser console log clean(er).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed the log message

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from b41ae3a to 84bfdbf Compare June 18, 2019 10:30
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from 6ca429d to 7684e94 Compare June 18, 2019 12:10
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from 7684e94 to 9317f23 Compare June 19, 2019 04:50
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from 9317f23 to 02afd73 Compare June 19, 2019 06:01
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from 02afd73 to 5eb4d40 Compare June 19, 2019 15:51
@TheRealJon
Copy link
Member

@openshift/team-ux-review

@TheRealJon
Copy link
Member

@lizsurette

@lizsurette
Copy link

@rawagner
Copy link
Contributor Author

@vojtechszocs all review comments addressed and rebased on master (which already has #1742)

@rawagner
Copy link
Contributor Author

since it is assigned to @TheRealJon he is the one who should give us LGTM :)

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019
@rawagner
Copy link
Contributor Author

@TheRealJon thanks! Still missing approve. Should that be given by you or @spadgett ?

@spadgett
Copy link
Member

/approve

But needs rebase :/

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from 7dc99ea to 65a8709 Compare June 27, 2019 10:24
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2019
@rawagner rawagner force-pushed the dashboard-capacity-2 branch 2 times, most recently from d5cc71b to c2a59b3 Compare June 27, 2019 10:37
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from c2a59b3 to a724ff9 Compare June 27, 2019 10:55
@rawagner
Copy link
Contributor Author

rawagner commented Jun 27, 2019

since #1783 is merged and it has migrated gauge to PF4 I've reused it here too.

loaded charts
cap-new

loading charts
cap-new-l

no data available
cap-new-no

@spadgett @TheRealJon

@rawagner
Copy link
Contributor Author

/test e2e-aws-console-olm

@TheRealJon
Copy link
Member

@rawagner I found a bug where some of the Area and Bar chart labels are objects instead of strings. I think this comes from the change to the humanize functions in frontend/public/components/utils/units.js. Those functions used to return a string, but now return an object. The following files need to have the formatX and formatY props updated for Bar and Area components:

  • frontend/public/components/namespace.jsx (Bar and Area)
  • frontend/public/components/build.tsx (Area)
  • frontend/public/components/node.tsx (Area)
  • frontend/public/components/pod.tsx (Area)

@rawagner
Copy link
Contributor Author

rawagner commented Jun 28, 2019

@TheRealJon thank you again for the commit. I've added one more and it should all be fine now. Please take a look.

update image-stream-tag.tsx humanize function usage
@rawagner rawagner force-pushed the dashboard-capacity-2 branch from f925182 to 9995763 Compare June 28, 2019 08:23
@rawagner
Copy link
Contributor Author

/retest

@rawagner
Copy link
Contributor Author

/test e2e-aws-console
/test e2e-aws

@rawagner
Copy link
Contributor Author

/test e2e-aws-console

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett, TheRealJon, vojtechszocs

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

The pull request process is described here

Details 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 2c455bf into openshift:master Jul 1, 2019
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. lgtm Indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.