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.

Sign up for GitHub

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

Jump to bottom

Bug 1946232: VM List can consume very high CPU #9696

Merged
merged 1 commit into from Aug 8, 2021
Merged

Bug 1946232: VM List can consume very high CPU #9696

merged 1 commit into from Aug 8, 2021

Conversation

glekner
Copy link
Contributor

@glekner glekner commented Aug 2, 2021

This PR adds a lazy status that loads the vm status bundle after the table renders.
This change improves the performance of queries when having 500+ VMs

While the status loads almost instantly, this is how the loading component looks for the status column

Screen.Recording.2021-08-02.at.14.58.01.mov

Here are performance recordings on x4 CPU slowdown, from the moment a query was typed until the new results were rendered:

Before 9033ms
Screen Shot 2021-08-02 at 14 54 44

After 4601ms
Screen Shot 2021-08-02 at 14 48 58

@openshift-ci openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 2, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 2, 2021

@glekner: This pull request references Bugzilla bug 1946232, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 1946232: VM List can consume very high CPU

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.

@openshift-ci openshift-ci bot added component/kubevirt Related to kubevirt-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 2, 2021
@glekner
Copy link
Contributor Author

glekner commented Aug 2, 2021

@rawagner @vojtechszocs @yaacov PTAL

rawagner
rawagner reviewed Aug 2, 2021
View reviewed changes
frontend/packages/kubevirt-plugin/src/components/vm-status/lazy-vm-status.tsx Outdated
setVmStatus(vmStatusBundle);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [loaded]);
Copy link
Contributor

Choose a reason for hiding this comment

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

so the vm status is never updated beside the first one when you load resources ? That does not seem correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I guess there is no going around listening to all resources? @rawagner

Copy link
Contributor

Choose a reason for hiding this comment

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

there isnt. You can optimize it by using useMemo and not the state/effect combo.
You can also try comparing previous state result with current one with deep compare

glekner
glekner commented Aug 2, 2021
View reviewed changes
frontend/packages/kubevirt-plugin/src/components/vms/vm.tsx Outdated
model = VirtualMachineModel;
options = vmMenuActions.map((action) =>
action(model, vm, {
vmStatusBundle,
vmStatusBundle: {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner do you think creating a lazy kebab menu is a viable solution here?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm...maybe let the user know you're waiting for status ? Something like disabled item loading status etc ?

rawagner
rawagner reviewed Aug 2, 2021
View reviewed changes
frontend/packages/kubevirt-plugin/src/components/vm-status/use-vm-status-resources.tsx Outdated
const [dvs, setDvs] = React.useState<V1alpha1DataVolume[]>([]);
const [pvcs, setPvcs] = React.useState<PersistentVolumeClaimKind[]>([]);

const watchedResources = React.useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

you do not have to use useMemo anymore, watchK8s hook is comparing the input args using deep compare

@glekner glekner force-pushed the improve-vm-table-performance branch from f4568f7 to c169145 Compare August 2, 2021 12:38
@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Aug 2, 2021
@glekner glekner force-pushed the improve-vm-table-performance branch 2 times, most recently from 84cad98 to 3e77449 Compare August 2, 2021 14:00
@glekner
Copy link
Contributor Author

glekner commented Aug 2, 2021

@glekner glekner force-pushed the improve-vm-table-performance branch 2 times, most recently from d981e47 to 48a21fa Compare August 2, 2021 20:39
@rawagner
Copy link
Contributor

rawagner commented Aug 3, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2021
@glekner
Copy link
Contributor Author

glekner commented Aug 3, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

6 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@glekner
Copy link
Contributor Author

glekner commented Aug 5, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@yaacov
Copy link
Member

yaacov commented Aug 6, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

19 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-ci openshift-ci bot merged commit 20432e4 into openshift:master Aug 8, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2021

@glekner: All pull requests linked via external trackers have merged:

Bugzilla bug 1946232 has been moved to the MODIFIED state.

In response to this:

Bug 1946232: VM List can consume very high CPU

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.

@openshift-ci openshift-ci bot mentioned this pull request Aug 13, 2021
Merged
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
@openshift-ci openshift-ci bot mentioned this pull request Oct 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rawagner rawagner rawagner left review comments

@yaacov yaacov yaacov left review comments

@gouyang gouyang Awaiting requested review from gouyang

@pcbailey pcbailey Awaiting requested review from pcbailey

Assignees

@rawagner rawagner

@yaacov yaacov

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/kubevirt Related to kubevirt-plugin 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.
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@glekner @rawagner @openshift-bot @yaacov @spadgett