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

HPA In-Context (Delete | Edit | Topology Sidebar) #6150

Merged

Conversation

andrewballantyne
Copy link
Contributor

@andrewballantyne andrewballantyne commented Jul 29, 2020

Combined for easy reviewing. Former PRs: #6037, #6039, #6087

Fixes:
https://issues.redhat.com/browse/ODC-4270
https://issues.redhat.com/browse/ODC-4271
https://issues.redhat.com/browse/ODC-4272

Analysis / Root cause:

  • As a user, I want a way to delete an in-context HPA.
  • As a user, I'd like to be able to in-context edit an associated HPA.
  • As a user, I'd like to be able to see my HPA resource in the sidebar and not be able to change pods as the HPA will fight me

Solution Description:

  • Add a Kebab action and modal to support deleting the associated HPA on a workload.
  • Using the same form from the Add HPA, allow editing.
  • Update to add a resource section for HPAs and to stop manual scaling of the pods; through kebab and scale arrows on the pod donut that is on the details tab.

Screen shots / Gifs for design review:
@openshift/team-devconsole-ux & @lwrigh

Screen Shot 2020-07-22 at 7 32 11 AM
Screen Shot 2020-07-22 at 7 32 19 AM

When a custom metric is in play, we auto select the YAML - they can still use the Form when they so desire to get access to cpu / memory (if applicable), the hpa name and min/max replica counts.
Screen Shot 2020-07-22 at 7 50 51 AM

Sidebar:
Screen Shot 2020-07-23 at 1 24 52 PM
Screen Shot 2020-07-23 at 1 25 06 PM
Screen Shot 2020-07-23 at 1 42 05 PM

Unit test coverage report:

  doesHpaMatch checks if it aligns to a workload
    ✓ expect not to match when hpa does not target workload
    ✓ expect to match when hpa does target workload
  hasCustomMetrics accurately determines if an hpa contains non-default metrics
    ✓ expect no metrics to mean no custom metrics
    ✓ expect cpu and memory metrics to not be custom
    ✓ expect any unknown metric to be custom
    ✓ expect an unknown metric to trump known metrics

------------------------------------------------------------|----------|----------|----------|----------|-------------------|
File                                                        |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------------------------------------------|----------|----------|----------|----------|-------------------|
  src/components/hpa/hpa-utils.ts                           |    97.26 |    90.63 |      100 |    96.83 |            92,100 |
------------------------------------------------------------|----------|----------|----------|----------|-------------------|

Test setup:

  • Add an HPA to a D/DC Workload that is in Topology
  • Use the Kebab action to delete || edit
  • See Topology Sidebar Details page for no pod scaling and new text

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jul 29, 2020
@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console component/shared Related to console-shared labels Jul 29, 2020
@vikram-raj
Copy link
Member

vikram-raj commented Jul 30, 2020

After adding a HPA I am still seeing the Add HPA option however Remove and Edit HPA option should present. Am I missing something?

Screenshot 2020-07-30 at 9 45 14 AM

@dtaylor113
Copy link
Contributor

@spadgett Another 'Silence Alert' flake where 'Watchdog' isn't listed, although I don't see the 'test for rows' before applying filter test?

@dtaylor113
Copy link
Contributor

/retest

@andrewballantyne
Copy link
Contributor Author

After adding a HPA I am still seeing the Add HPA option however Remove and Edit HPA option should present. Am I missing something?

Odd, not sure what happened. Somewhere from last week to now through rebases the association code found an unlikely home in the wrong spot. HPAs were not associated with the resource anymore. Should be fixed now though.

frontend/public/components/overview/hpa-overview.tsx Outdated Show resolved Hide resolved
};

export const HPAOverview: React.FC<HPAOverviewProps> = ({ hpas }) => {
if (!hpas?.length) {
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
if (!hpas?.length) {
if (hpas?.length === 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hpas being null won't match 0 absolutely. So that changes the logic. Either we don't have hpas OR we have an array of zero is the logic today, if we wanted to keep that and compare against 0:

Suggested change
if (!hpas?.length) {
if (!hpas || hpas.length === 0) {

@jerolimov
Copy link
Member

Could verify that the "Edit HPA" menu was shown with the latest commit (97f28cd) 👍

Some comments are still open, but it lgtm. I tested it with local web console on a shared cluster.

Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

Verify all the scenarios and it works as expected.

/lgtm

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

I've logged a couple bugs to cover the missing things found:

https://issues.redhat.com/browse/ODC-4411
https://issues.redhat.com/browse/ODC-4412

/assign @christianvogt

Christian, we'll need an approval from you to get this in before EOD tomorrow.

@andrewballantyne
Copy link
Contributor Author

/retest

@invincibleJai
Copy link
Member

/test e2e-gcp-console

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, vikram-raj

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2020
@andrewballantyne
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Contributor

@andrewballantyne: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console e530edf link /test e2e-gcp-console

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit dc3a26e into openshift:master Jul 31, 2020
@spadgett spadgett added this to the v4.6 milestone Aug 1, 2020
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/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet