You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "JoaoJandre (via GitHub)" <gi...@apache.org> on 2024/03/04 13:23:20 UTC
[PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
JoaoJandre opened a new pull request, #8740:
URL: https://github.com/apache/cloudstack/pull/8740
### Description
ACS has a job that periodically removes records from the `vm_stats` table that are older than the retention time (setting `vm.stats.max.retention.time`). In environments that have a large amount of statistics being collected and have a long retention time, if the retention time is shortened, table cleaning may fail to finish due to the huge amount of records being deleted leading to a query timeout.
The `vm.stats.remove.batch.size` configuration has been added to define the batch size of statistics removal queries. This way, instead of trying to remove all records in a single query, ACS will remove records in batches, with the batch size being defined with the `vm.stats.remove.batch.size setting`. The default value is -1, where no limit will be applied; thus, the current behavior is maintained.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Bug fix (non-breaking change which fixes an issue)
- [X] 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
#### Feature/Enhancement Scale
- [ ] Major
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### How Has This Been Tested?
In a local lab, I artificially generated 60 million rows on `vm_stats` and set the retention time to one minute.
Before the change, the delete job would timeout and no rows would be removed.
After applying the change and setting `vm.stats.max.retention.time` to 100000, the table was successfully cleaned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1512752381
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -290,6 +287,11 @@ public String toString() {
protected static ConfigKey<Boolean> vmStatsCollectUserVMOnly = new ConfigKey<>("Advanced", Boolean.class, "vm.stats.user.vm.only", "false",
"When set to 'false' stats for system VMs will be collected otherwise stats collection will be done only for user VMs", true);
+ protected static ConfigKey<Long> vmStatsRemoveBatchSize = new ConfigKey<>("Advanced", Long.class, "vm.stats.remove.batch.size", "0", "Indicates the" +
Review Comment:
We could have a generic configuration for other operations, that is a good idea, however, this would incur in a lot more changes that escape this PR's scope. Furthermore, I don't think that we should reuse `detail.batch.query.size` for deletion, it is better to have more granularity on these configurations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1513204284
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1208,9 +1208,14 @@ public boolean expunge(final ID id) {
}
}
- // FIXME: Does not work for joins.
@Override
public int expunge(final SearchCriteria<T> sc) {
+ return expunge(sc, -1);
+ }
+
+ // FIXME: Does not work for joins.
+ @Override
+ public int expunge(final SearchCriteria<T> sc, long limit) {
Review Comment:
I'd rather stick with the simpler long limit, as I don't see any need to use the Filter's features on an expunge.
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1223,6 +1228,11 @@ public int expunge(final SearchCriteria<T> sc) {
str.append(sc.getWhereClause());
}
+ if (limit > 0) {
Review Comment:
Could you give an example where we would want to order the deletes? This would only make the deletes even slower.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1514463855
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1223,6 +1228,11 @@ public int expunge(final SearchCriteria<T> sc) {
str.append(sc.getWhereClause());
}
+ if (limit > 0) {
Review Comment:
I see how the index might be beneficial to SELECTs; however, we're only talking about DELETEs here, do you have any test cases using DELETEs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1513378431
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1208,9 +1208,14 @@ public boolean expunge(final ID id) {
}
}
- // FIXME: Does not work for joins.
@Override
public int expunge(final SearchCriteria<T> sc) {
+ return expunge(sc, -1);
+ }
+
+ // FIXME: Does not work for joins.
+ @Override
+ public int expunge(final SearchCriteria<T> sc, long limit) {
Review Comment:
Added explanation in the comment below.
https://github.com/apache/cloudstack/pull/8740#discussion_r1513377957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1513377957
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1223,6 +1228,11 @@ public int expunge(final SearchCriteria<T> sc) {
str.append(sc.getWhereClause());
}
+ if (limit > 0) {
Review Comment:
@JoaoJandre If you add order by and limit on a query, it can be faster if there is an index on the order by column. This depends on the query plan and the data distribution.
Also, while deleting stats, it would be better to remove the older entries first. Otherwise the user might see some missing timestamps if they fetch the data while it's being deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#discussion_r1512245114
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1208,9 +1208,14 @@ public boolean expunge(final ID id) {
}
}
- // FIXME: Does not work for joins.
@Override
public int expunge(final SearchCriteria<T> sc) {
+ return expunge(sc, -1);
+ }
+
+ // FIXME: Does not work for joins.
+ @Override
+ public int expunge(final SearchCriteria<T> sc, long limit) {
Review Comment:
```suggestion
public int expunge(final SearchCriteria<T> sc, final Filter filter) {
```
I would suggest keeping the parameters for this method similar to how it is for other methods for this class.
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -290,6 +287,11 @@ public String toString() {
protected static ConfigKey<Boolean> vmStatsCollectUserVMOnly = new ConfigKey<>("Advanced", Boolean.class, "vm.stats.user.vm.only", "false",
"When set to 'false' stats for system VMs will be collected otherwise stats collection will be done only for user VMs", true);
+ protected static ConfigKey<Long> vmStatsRemoveBatchSize = new ConfigKey<>("Advanced", Long.class, "vm.stats.remove.batch.size", "0", "Indicates the" +
Review Comment:
Should we have a generic global setting which we can use for other operations as well?
We can also reuse `detail.batch.query.size` for deletion as well.
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1223,6 +1228,11 @@ public int expunge(final SearchCriteria<T> sc) {
str.append(sc.getWhereClause());
}
+ if (limit > 0) {
Review Comment:
Use `addFilter` method with filter to add limit. This will also allow to add order by in the Delete queries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add configuration to limit the number of rows deleted from vm_stats [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8740:
URL: https://github.com/apache/cloudstack/pull/8740#issuecomment-1976590519
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8740?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.39%. Comparing base [(`a7ec873`)](https://app.codecov.io/gh/apache/cloudstack/commit/a7ec8738a27aaa7fae7a03f1f1aa8bdbaea1def3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`a2f238b`)](https://app.codecov.io/gh/apache/cloudstack/pull/8740?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.19 #8740 +/- ##
============================================
- Coverage 30.93% 4.39% -26.54%
============================================
Files 5353 361 -4992
Lines 376055 28630 -347425
Branches 54691 4996 -49695
============================================
- Hits 116317 1258 -115059
+ Misses 244443 27233 -217210
+ Partials 15295 139 -15156
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8740/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.39% <ø> (ø)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8740?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org