Skip to content

Conversation

rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Aug 31, 2021

Splitting out the CPU and Memory column work from #9575 per @spadgett so we can roll it into a future release.

In order to add sorting for these columns, we had to move to the new virtualized table.

Screenshot (note that "test" MachineSet has junk data so I could test the sorting; it does not reflect actual data for the instance type):

Screen Shot 2021-09-13 at 2 17 39 PM

Related to https://issues.redhat.com/browse/CONSOLE-2319.

Fixes https://issues.redhat.com/browse/CONSOLE-2967.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2021
@openshift-ci openshift-ci bot requested review from jhadvig and zherman0 August 31, 2021 17:56
@openshift-ci openshift-ci bot added 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 Aug 31, 2021
@rebeccaalpert rebeccaalpert added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2021
@rebeccaalpert rebeccaalpert changed the title [WIP] CONSOLE-2319: Expose CPU and memory in MachineSet list/details [WIP] CONSOLE-2967: Expose CPU and memory in MachineSet list/details Sep 13, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2021
@rebeccaalpert rebeccaalpert removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 13, 2021
@rebeccaalpert rebeccaalpert changed the title [WIP] CONSOLE-2967: Expose CPU and memory in MachineSet list/details CONSOLE-2967: Expose CPU and memory in MachineSet list/details Sep 13, 2021
@rebeccaalpert
Copy link
Contributor Author

CC @andybraren for visibility.

@rebeccaalpert
Copy link
Contributor Author

/test frontend

@rebeccaalpert
Copy link
Contributor Author

INFO[2021-09-14T19:28:16Z] ci-operator version v20210914-9ee4485b0-dirty 
INFO[2021-09-14T19:28:16Z] Loading configuration from https://config.ci.openshift.org for openshift/console@master 
INFO[2021-09-14T19:28:16Z] Resolved source https://github.com/openshift/console to master@a6684324, merging: #9956 349b81ec @rebeccaalpert 
INFO[2021-09-14T19:28:17Z] Building release initial from a snapshot of ocp/4.10 
INFO[2021-09-14T19:28:17Z] Building release latest from a snapshot of ocp/4.10 
INFO[2021-09-14T19:28:17Z] Using namespace https://console.build02.ci.openshift.org/k8s/cluster/projects/ci-op-zq3b7lr7 
INFO[2021-09-14T19:28:17Z] Running [input:root], src, test-bin, frontend 
INFO[2021-09-14T19:28:18Z] Tagging openshift/release:tectonic-console-builder-v22 into pipeline:root. 
INFO[2021-09-14T19:28:18Z] Building src                                 
INFO[2021-09-14T19:30:33Z] Build src succeeded after 2m11s              
INFO[2021-09-14T19:30:33Z] Building test-bin                            
INFO[2021-09-14T19:49:33Z] Build test-bin succeeded after 18m59s        
INFO[2021-09-14T19:49:34Z] Executing test frontend                      
INFO[2021-09-14T20:19:34Z] pod didn't start running within 30m0s: 
Found 5 events for Pod frontend:
* 0x : 0/72 nodes are available: 3 node(s) had taint {node-role.kubernetes.io/infra: }, that the pod didn't tolerate, 3 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate, 66 Insufficient cpu.
* 0x : 0/72 nodes are available: 3 node(s) had taint {node-role.kubernetes.io/infra: }, that the pod didn't tolerate, 3 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate, 66 Insufficient cpu.
* 1x cluster-autoscaler: pod didn't trigger scale-up: 1 node(s) had taint {node-role.kubernetes.io/infra: }, that the pod didn't tolerate, 3 in backoff after failed scale-up
* 1x cluster-autoscaler: pod triggered scale-up: [{MachineSet/openshift-machine-api/build0-gstfj-w-b 19->25 (max: 40)}]
* 1x cluster-autoscaler: pod triggered scale-up: [{MachineSet/openshift-machine-api/build0-gstfj-w-c 32->38 (max: 40)}] 
INFO[2021-09-14T20:19:34Z] Ran for 51m17s                               
ERRO[2021-09-14T20:19:34Z] Some steps failed:                           
ERRO[2021-09-14T20:19:34Z] 
  * could not run steps: step frontend failed: test "frontend" failed: pod didn't start running within 30m0s: 

@rebeccaalpert
Copy link
Contributor Author

/test frontend

@yapei
Copy link
Contributor

yapei commented Sep 16, 2021

@rebeccaalpert I tested the PR on a cluster created via cluster-bot, so it looks like we show size of the first node belonging to that MachineSet for CPU and Memory data, is it working by design?
It looks reasonable because all machines belong to the machineset should have the same capacity, right?

@rebeccaalpert
Copy link
Contributor Author

@yapei - Yes, Andy and Ali only wanted to show the size of the node. Andy said the machines should have the same capacity, based on what the instance type is.

@rebeccaalpert
Copy link
Contributor Author

I'm getting a frontend test error that I can't recreate locally (test is passing fine):

PolicySectionFormData should submit the right value when switching to parallel: PolicySectionFormData should submit the right value when switching to parallel

TypeError: Cannot read property 'dashboards' of undefined
    at /go/src/github.com/openshift/console/frontend/public/actions/dashboards.ts:66:32
    at Generator.next (<anonymous>)
    at /go/src/github.com/openshift/console/frontend/public/actions/dashboards.ts:8:71
    at new Promise (<anonymous>)
    at Object.<anonymous>.__awaiter (/go/src/github.com/openshift/console/frontend/public/actions/dashboards.ts:4:12)
    at fetchPeriodically (/go/src/github.com/openshift/console/frontend/public/actions/dashboards.ts:65:5)
    at /go/src/github.com/openshift/console/frontend/public/actions/dashboards.ts:78:13
    at Timeout.callback [as _onTimeout] (/go/src/github.com/openshift/console/frontend/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/browser/Window.js:523:19)
    at listOnTimeout (internal/timers.js:554:17)
    at processTimers (internal/timers.js:497:7)

@rebeccaalpert
Copy link
Contributor Author

/test frontend

@rebeccaalpert
Copy link
Contributor Author

Yay! Tests are finally not flaking. This is ready for review after bug bash Monday is over.

Copy link
Member

Choose a reason for hiding this comment

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

@rebeccaalpert I'm not sure if this is in the scope of this pr...maybe @spadgett would know. Are we supposed to be referencing api's from the shared package for the purpose of providing core only vs admin plugins?

Copy link
Member

@jcaianirh jcaianirh left a comment

Choose a reason for hiding this comment

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

small change requested plus a question for you and sam on the imports. Looks really great!!!

@rebeccaalpert
Copy link
Contributor Author

I updated the cores text to use count per Joe's feedback.

Copy link
Member

@jcaianirh jcaianirh 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
@rebeccaalpert
Copy link
Contributor Author

/assign @ahardin-rh @opayne1
for docs approval

/assign @yapei
for QE approval

/assign @RickJWagner
for px approval

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2021
@opayne1
Copy link
Contributor

opayne1 commented Sep 23, 2021

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 23, 2021
@RickJWagner
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 23, 2021
@yapei
Copy link
Contributor

yapei commented Sep 24, 2021

CPU and Memory column shows an intermediate value 0 before correct number is finally loaded, screen recording is attached

Screen.Recording.2021-09-24.at.11.08.17.AM.mov

Added the memory and number of CPU cores for each MachineSet to the MachineSet list view.

Fixes https://issues.redhat.com/browse/CONSOLE-2967
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021
@rebeccaalpert
Copy link
Contributor Author

Thanks @yapei - I'm having trouble recreating this locally, but it makes sense this would happen if the machines or nodes weren't loaded yet. I pushed a change that would load the skeleton loading table instead if this happens. I'll try to run the PR with cluster bot so I can see if the problem is fixed with that change. Please feel free to try on your end also when it's your working hours.

@yapei
Copy link
Contributor

yapei commented Sep 26, 2021

@rebeccaalpert The new changes look good to me in my local testing, no other issues

@yapei
Copy link
Contributor

yapei commented Sep 26, 2021

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 26, 2021
Copy link
Member

@jhadvig jhadvig 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaianirh, jhadvig, rebeccaalpert

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 99783aa into openshift:master Sep 27, 2021
@spadgett spadgett added this to the v4.10 milestone Oct 7, 2021
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/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR 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.

9 participants