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.

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

Improve cleanup scripts #12906

Conversation

haydenseitz
Copy link
Contributor

@haydenseitz haydenseitz commented May 21, 2021

update lock_and_purge shared function to do dbDelete in blocks of 1000 rows, similar to how the cleanup for syslog currently happens

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/polling-stops-every-day-during-daily-sh-run/15805/6

@haydenseitz
Copy link
Contributor Author

updated lock_and_purge function. not sure how to implement with lock_and_purge_query, as if uses dbQuery instead of dbDelete.

Are there integration tests for the daily.sh script?

@haydenseitz haydenseitz changed the title WIP: Improve cleanup scripts Improve cleanup scripts May 21, 2021
@@ -2056,9 +2057,16 @@ function lock_and_purge($table, $sql)

$name = str_replace('_', ' ', ucfirst($table));
if (is_numeric($purge_days)) {
if (dbDelete($table, $sql, [$purge_days])) {
echo "$name cleared for entries over $purge_days days\n";
if (is_numeric($count)) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI $count is always numeric, you said it must be an int in the parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, removed the check

@haydenseitz
Copy link
Contributor Author

after running this a few days in production, I see the loop creates a sort of race condition on certain tables that are updated frequently (device_perf). Any of the SQL statements that use NOW() in the WHERE to delete old rows can potentially never return 0 rows, as there are new entries every time it runs.

I'm thinking I can update each of the SQL statements passed to the lock_and_purge function to either:

  1. get the current time in PHP and use that in each the statement, then it's always looping over the same timestamp until all rows are deleted
  2. update each statement to use CURDATE(), which will only delete records up to midnight of X days before, instead of actually X days before

I'm leaning toward option 2 - suggestions?

@haydenseitz haydenseitz marked this pull request as draft June 3, 2021 23:31
@murrant
Copy link
Member

murrant commented Jun 15, 2021

2 seems like the better choice, a fixed time instead of a rolling window.

@murrant
Copy link
Member

murrant commented Sep 24, 2021

Closing due to inactivity

@murrant murrant closed this Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants