Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Reduce the number of vmstats data saved and improve the response speed of the instance list page#8350

Closed
havengit wants to merge 2 commits intoapache:mainapache/cloudstack:mainfrom
havengit:mainhavengit/cloudstack:mainCopy head branch name to clipboard
Closed

Reduce the number of vmstats data saved and improve the response speed of the instance list page#8350
havengit wants to merge 2 commits intoapache:mainapache/cloudstack:mainfrom
havengit:mainhavengit/cloudstack:mainCopy head branch name to clipboard

Conversation

@havengit
Copy link
Contributor

@havengit havengit commented Dec 13, 2023

Description

This PR...

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

improve the following issues
#8266

For vmstats data, the default global settings will cause a large amount of data to be stored in the database. When accessing the instance page, vmstats data will be requested, causing the page to open extremely slowly. Especially when multiple users access the instance page at the same time, the problem will be exacerbated.
My environment has about 200 running instances.

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Because it is a modification of global settings, the test method is to directly modify the global parameters. The corresponding code modifications are for new deployments. There are no tests for newly deployed scenarios.
test environment:
ACS 4.18.0.0 + Galera mariadb server CLuster 10.6
Ubuntu 20.04+KVM

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Dec 13, 2023

Is this still a draft PR, @havengit ?

@havengit
Copy link
Contributor Author

havengit commented Dec 13, 2023

Is this still a draft PR, @havengit ?

This is a complete PR with very few changes.

@BryanMLima
Copy link
Contributor

BryanMLima commented Dec 13, 2023

@DaanHoogland @havengit, the default global configuration values should only be used as a guideline, and it will not work in every environment.

This feature was introduced in PR #5984 with the configuration vm.stats.max.retention.time with value of 1 (to maintain backward compatibility), which would not cause any problems, performance-wise. However, the default value was changed to 720 with PR #6803, which seems to be too high for larger environments. I do not think that changing the default value again is going to help anybody. Documentation regarding the combination of the parameters vm.stats.max.retention.time and vm.stats.interval would benefit everyone. IMHO, the default value vm.stats.max.retention.time should not have been changed since the introduction of the feature, as the operator must know its underlying systems to set its value.

@weizhouapache
Copy link
Contributor

weizhouapache commented Dec 13, 2023

@DaanHoogland @havengit, the default global configuration values should only be used as a guideline, and it will not work in every environment.

This feature was introduced in PR #5984 with the configuration vm.stats.max.retention.time with value of 1 (to maintain backward compatibility), which would not cause any problems, performance-wise. However, the default value was changed to 720 with PR #6803, which seems to be too high for larger environments. I do not think that changing the default value again is going to help anybody. Documentation regarding the combination of the parameters vm.stats.max.retention.time and vm.stats.interval would benefit everyone. IMHO, the default value vm.stats.max.retention.time should not have been changed since the introduction of the feature, as the operator must know its underlying systems to set its value.

+1 with documentation changes.

We should not change the default value of existing configurations, unless the value is extremely unreasonable.

@havengit
Copy link
Contributor Author

havengit commented Dec 14, 2023

That makes sense, I'll close this PR

@havengit havengit closed this Dec 14, 2023
@BryanMLima BryanMLima mentioned this pull request Dec 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

component:management-server

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@havengit @DaanHoogland @BryanMLima @weizhouapache
Morty Proxy This is a proxified and sanitized view of the page, visit original site.