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

Limit maximum metrics to be deleted simultaniously #1125

Merged
merged 1 commit into from
Jul 11, 2021
Merged

Limit maximum metrics to be deleted simultaniously #1125

merged 1 commit into from
Jul 11, 2021

Conversation

felixhuettner
Copy link
Contributor

Currently gnocchi-metricd tries to collect all metrics that should be deleted
and deletes them in one big loop. If a large amount of old metrics has
accumulated this will cause high load on the database as it needs to find
and sort all of these rows. It will also cause a high amount of memory usage
for gnocchi-metricd as it needs to also sort all of these rows in memory.

To fix this the amount of rows being deleted in one run is limited to 10000.
This is sufficient to allow gnocchi to cleanup metrics in a fast way under
normal circumstances while still allowing it to catch up if no deletion has
been performed for a while.

Signed-off-by: Felix Huettner [email protected]

@felixhuettner felixhuettner marked this pull request as ready for review March 18, 2021 14:37
@felixhuettner
Copy link
Contributor Author

Hi everyone,
i'm not sure about the CI Runs but the issues do not seem to be related to the PR in any way

jd
jd previously requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

Can we make this configurable?

@mergify mergify bot dismissed jd’s stale review March 18, 2021 15:14

Pull request has been modified.

@felixhuettner
Copy link
Contributor Author

Implemented a configuration option. Not sure about the name though

jd
jd previously requested changes Mar 18, 2021
View reviewed changes
gnocchi/opts.py Outdated Show resolved Hide resolved
gnocchi/opts.py Outdated Show resolved Hide resolved
@mergify mergify bot dismissed jd’s stale review March 18, 2021 15:39

Pull request has been modified.

jd
jd reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

Thank you!
IIRC the doc is autogenerated. Maybe adding a release note with reno would be cool? :)

@felixhuettner
Copy link
Contributor Author

I hope i got the release note format correctly :)

jd
jd previously approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

Yeah this is great! Thank you!

releasenotes/notes/configurable_clenaup_batch_size-a785c1aa6ee29058.yaml Outdated Show resolved Hide resolved
@jd
Copy link
Member

jd commented Mar 19, 2021

The CI seems broken of the latest tenacity, somebody should rewrite the current code to stop using the deprecated now-removed API.

@felixhuettner
Copy link
Contributor Author

What is then the best way to go forward here? Should we just leave it sitting here until the CI is fixed (that would be fine with me)

@mergify mergify bot dismissed jd’s stale review March 22, 2021 10:38

Pull request has been modified.

jd
jd requested changes Mar 22, 2021
View reviewed changes
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

Yes, feel free to give a hand to fix the CI. Not sure anybody is working on it 😢

@felixhuettner
Copy link
Contributor Author

Would you have any details where i could start taking a look? Then i'll see if i can get some time for that

@jd
Copy link
Member

jd commented Mar 22, 2021

The latest tenacity release https://pypi.org/project/tenacity/ 7.0.0 removed old compat code for retry functions.
It seems Gnocchi is providing its own retry function and they need to be rewritten for the new API. That should not be hard.

@mrunge
Copy link
Contributor

mrunge commented Mar 25, 2021

I've put together a PR to fix CI by pinning tenacity and sqlalchemy, #1124
There is also a bump for oslo.policy included. This patch currently fails in upgrade tests; I've also submitted the same for the previous branch, (which fails in ubgrade checks from 4.2...)

@tobias-urdin
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Mar 26, 2021

Command rebase: failure

Pull request can't be updated with latest base branch changes
Mergify needs the permission to update the base branch of the pull request.
gnocchixyz needs to authorize modification on its base branch.
err-code: 49C48

jd
jd previously approved these changes Mar 26, 2021
View reviewed changes
@jd
Copy link
Member

jd commented Mar 26, 2021

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Mar 26, 2021

Command update: failure

Base branch update has failed
user doesn't have permission to update head repository
err-code: 328A3

@jd
Copy link
Member

jd commented Mar 26, 2021

@felixhuettner you really need to click that checkbox 😆

@felixhuettner
Copy link
Contributor Author

:D ok that is fixed now.
Let me try this
@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2021

@felixhuettner is not allowed to run commands

@felixhuettner
Copy link
Contributor Author

:D ok, let's do it manually

@mergify mergify bot dismissed jd’s stale review March 29, 2021 11:37

Pull request has been modified.

@tobias-urdin
Copy link
Contributor

@Mergifyio rebase

@felixhuettner
Limit maximum metrics to be deleted simultaniously
d5050c5 
Currently gnocchi-metricd tries to collect all metrics that should be deleted
and deletes them in one big loop. If a large amount of old metrics has
accumulated this will cause high load on the database as it needs to find
and sort all of these rows. It will also cause a high amount of memory usage
for gnocchi-metricd as it needs to also sort all of these rows in memory.

To fix this the amount of rows being deleted in one run is limited to 10000.
This is sufficient to allow gnocchi to cleanup metrics in a fast way under
normal circumstances while still allowing it to catch up if no deletion has
been performed for a while.

Signed-off-by: Felix Huettner <[email protected]>
@jd jd force-pushed the limit_cleanup branch from cecf952 to d5050c5 Compare July 7, 2021 21:31
@mergify
Copy link
Contributor

mergify bot commented Jul 7, 2021

Command rebase: success

Branch has been successfully rebased

@tobias-urdin tobias-urdin merged commit cadaadc into gnocchixyz:master Jul 11, 2021
@felixhuettner felixhuettner deleted the limit_cleanup branch July 12, 2021 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jd jd jd approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@felixhuettner @jd @mrunge @tobias-urdin