graylog2-server icon indicating copy to clipboard operation
graylog2-server copied to clipboard

Search Cleanup Job: Fix interval checking, skip static referenced searches

Open danotorrey opened this issue 10 months ago • 6 comments

Description

The SearchCleanupJob previously used a fixed previous duration (set at in constructor at injection time), which resulted in the periodical only ever cleaning up expired searches once at server startup time (thanks to @ryan-carroll-graylog for noticing this). The migration is intended to perform the cleanup once per hour. This PR fixes that, so that the expired reports threshold mustBeOlderThan is recalculated on each run, which allows newly expired non-referenced searches to be deleted each time the job runs.

Also adds support for not deleting static referenced search ids. See https://github.com/Graylog2/graylog-plugin-enterprise/pull/7107 for context.

/nocl fixes/adjusts backend process that does not affect visible user functionality

How Has This Been Tested?

Manual testing by overriding the initial delay and run intervals in the migration to get it to execute quicker. Verifying that appropriate static IDs are skipped, unit tests.

Screenshots (if appropriate):

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] Refactoring (non-breaking change)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.

danotorrey avatar Apr 19 '24 13:04 danotorrey

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 19 '24 13:04 CLAassistant

Thanks for fixing this @danotorrey! And really like the approach of using the StaticReferencedSearch binding to preserve searches we don't want cleaned up.

This all looks awesome but just a heads up, I checked out this PR without the enterprise counterpart and I'm getting a Guice error that looks to be because we're never calling PluginModule.addStaticReferencedSearch and therefore nothing is bound to Set<StaticReferencedSearch> staticReferencedSearches for the SearchesCleanUpJob class:

com.google.inject.CreationException: Unable to create injector, see the following errors:

1) [Guice/MissingImplementation]: No implementation for Set<StaticReferencedSearch> was bound.

Requested by:
1  : SearchesCleanUpJob.<init>(SearchesCleanUpJob.java:50)
      \_ for 5th parameter
     at PluginModule.addPeriodical(PluginModule.java:102)

Not sure if this would be a problem for instances without the security module, maybe we can call staticReferencedSearchBinder somewhere in graylog2-server where we know it'll always get bound?

ryan-carroll-graylog avatar Apr 19 '24 13:04 ryan-carroll-graylog

Not sure if this would be a problem for instances without the security module, maybe we can call staticReferencedSearchBinder somewhere in graylog2-server where we know it'll always get bound?

@ryan-carroll-graylog @danotorrey I've run into something similar before when working on the Illuminate lookup tables. We should be able to do exactly that somewhere in an open module, similar to here for the new SystemEntity lookup stuff.

kingzacko1 avatar Apr 19 '24 14:04 kingzacko1

@ryan-carroll-graylog @kingzacko1 Nice find! Thanks for the tip. Just added that fix, and it seems to work.

danotorrey avatar Apr 19 '24 15:04 danotorrey

Just force-pushed the same changeset with a fixed email address to resolve the CLA-signing check failure.

danotorrey avatar Apr 22 '24 19:04 danotorrey

@dennisoelkers @moesterheld Do you have any concerns or see any issues with this? Just wanted to check-in before merging.

danotorrey avatar Apr 24 '24 22:04 danotorrey