Skip to content

ui: speed up compute instance listing #7911

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

Merged
weizhouapache merged 2 commits into from
Aug 25, 2023
Merged

ui: speed up compute instance listing #7911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4629ad
ui: speed up compute instance listing
yadvr Aug 25, 2023
c10873e
Update AutogenView.vue
yadvr Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ui/src/config/section/compute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export default {
permission: ['listVirtualMachinesMetrics'],
resourceType: 'UserVm',
params: () => {
var params = {}
var params = { details: 'servoff,tmpl,nics' }
Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

@weizhouapache because of these params, details and state...

if (store.getters.metrics) {
params = { state: 'running' }
Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

I've removed running as otherwise metrics doesn't return all VMs in the list. One can select VM state explicitly in the compute/instance page if they want.

Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

just realised I can remove state from the params

params = { details: 'servoff,tmpl,nics,stats' }
Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

VM stats in the API response can cause 5-10x delay compared to normal list API response

Copy link
Contributor

@DaanHoogland DaanHoogland Aug 25, 2023

Choose a reason for hiding this comment

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

is tmpl needed in the details? or stats?

Copy link
Member

@weizhouapache weizhouapache Aug 25, 2023

Choose a reason for hiding this comment

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

my concern is opposite to @DaanHoogland :-D
is 'servoff,tmpl,nics' or servoff,tmpl,nics,stats enough for UI ?

Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

Yes, these are enough just for list view (table view); the stats are only needed when the metrics button is clicked. For the resource view, we delete the params.details, so by default details=all is used when listing a single VM.

Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

@weizhouapache @DaanHoogland
I tested this again both as root admin and normal user account; for VMs and templates with and without resource icons; these are enough details to render the list view (table); when the user clicks any VM/instance, then we call the API remove the details and all details are listed/used in the resource view. While not ideal, this speeds up the listing.

Copy link
Member

@weizhouapache weizhouapache Aug 25, 2023

Choose a reason for hiding this comment

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

great, thanks @rohityadavcloud
I will test it

}
return params
},
Expand Down
1 change: 1 addition & 0 deletions ui/src/store/mutation-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const DOMAIN_STORE = 'DOMAIN_STORE'
export const DARK_MODE = 'DARK_MODE'
export const VUE_VERSION = 'VUE_VERSION'
export const CUSTOM_COLUMNS = 'CUSTOM_COLUMNS'
export const RELOAD_ALL_PROJECTS = 'RELOAD_ALL_PROJECTS'
Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

This fixes a warning


export const CONTENT_WIDTH_TYPE = {
Fluid: 'Fluid',
Expand Down
4 changes: 4 additions & 0 deletions ui/src/views/AutogenView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,10 @@ export default {
delete params.showunique
}

if (['listVirtualMachinesMetrics'].includes(this.apiName) && this.dataView) {
delete params.details
Copy link
Member Author

@yadvr yadvr Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we remove the details as it may not be all, and set for the list view (tabular view)

}
Copy link
Member

@weizhouapache weizhouapache Aug 25, 2023

Choose a reason for hiding this comment

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

@rohityadavcloud
can you explain why state and details are removed ?

all others look good

Copy link
Member Author

@yadvr yadvr Aug 25, 2023

Choose a reason for hiding this comment

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

These are added by the router config, I'll ping you on that code; so we must remove if the keys exists for dataView (or resource view, when only one item is being listed such as the VM)


this.loading = true
if (this.$route.params && this.$route.params.id) {
params.id = this.$route.params.id
Expand Down