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

libbeat/processors/cache: add file-backed cache #36686

Merged

efd6 merged 5 commits into elastic:main from efd6:i2816-cache_processor_file_backed

Sep 28, 2023

Contributor

efd6 commented Sep 27, 2023

Proposed commit message

See title.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

Related issues

For Crowdstrike FDR hostname/username enrichment integrations#2816

Use cases

Screenshots

Logs

efd6 added enhancement libbeat Team:Security-External Integrations backport-skip 8.11-candidate labels

efd6 self-assigned this

botelastic bot added needs_team and removed needs_team labels

efd6 force-pushed the i2816-cache_processor_file_backed branch from 0afe564 to 8675f2f Compare

September 27, 2023 06:43


          libbeat/processors/cache: add file-backed cache

d6fcef4

efd6 force-pushed the i2816-cache_processor_file_backed branch from 8675f2f to d6fcef4 Compare

September 27, 2023 06:45

efd6 marked this pull request as ready for review

September 27, 2023 07:57

efd6 requested a review from a team as a code owner

September 27, 2023 07:57

Collaborator

elasticmachine commented Sep 27, 2023

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

Collaborator

elasticmachine commented Sep 27, 2023 •

edited by jenkins-beats-ci bot

Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-09-28T03:13:08.501+0000
Duration: 76 min 5 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28350
Skipped	2013
Total	30363

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)


          add periodic writes to file-backed cache

e0327b6

efd6 force-pushed the i2816-cache_processor_file_backed branch from 829ffe3 to e0327b6 Compare

September 27, 2023 22:10

andrewkroh reviewed

View reviewed changes

libbeat/processors/cache/config.go Outdated

               	ID string `config:"id"`
               }
+              type fileConfig struct {
+              	ID            string        `config:"id"`
+              	WriteOutEvery time.Duration `config:"write_frequency"`

Member

andrewkroh Sep 28, 2023

I need to look over the rest of the code base to see if there is any consistency for naming this type of setting. My initial thought is that this is an "interval" rather than a "frequency", because an interval is a specified as duration between events.

libbeat/processors/cache/file_store.go Outdated

+              		err = dec.Decode(&e)
+              		if err != nil {
+              			if err != io.EOF {
+              				c.log.Errorw("failed to read state element", "error", err)

Member

andrewkroh Sep 28, 2023

Might be nice to know the input_offset from the decoder.

libbeat/processors/cache/file_store.go

+              			return
+              		}
+              		// Try to be atomic.
+              		err = os.Rename(tmp, c.path)

Member

andrewkroh Sep 28, 2023

There are a lot traps with Rename that vary by OS. If you can put the "temporary" file in the same directory at the final file you can avoid most of them. For example, you cannot rename a file if the source and dest are on different volumes.

Contributor Author

efd6 Sep 28, 2023

Putting it in filepath.Dir(c.path) should get us there then.

libbeat/processors/cache/file_store.go

+              	}
+              	tmp := f.Name()
+              	defer func() {
+              		err = f.Close()

Member

andrewkroh Sep 28, 2023

One thing to be aware of is that write operations are cached on Windows. So even though this may have closed the file, it's not guaranteed to be synced to disk. You could incorporate an f.Sync() call to ensure this is flush from the write cache to disk (it does a https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-flushfilebuffers).

Contributor Author

efd6 Sep 28, 2023

This is theoretically true on posix as well (I was considering this, but didn't do it).

libbeat/processors/cache/file_store.go

+              		return
+              	}
+              	// We are writing into tmp, so make sure we are private.
+              	err = os.Chmod(f.Name(), 0o600)

Member

andrewkroh Sep 28, 2023

I assume this does nothing for Windows. Does that pose a problem for users? 🤔

Contributor Author

efd6 Sep 28, 2023

If it goes in the path.data directory, we are at least not making things worse.

efd6 requested a review from andrewkroh

September 28, 2023 02:11


          address pr comments

5d57495

efd6 force-pushed the i2816-cache_processor_file_backed branch from 35de580 to 5d57495 Compare

September 28, 2023 02:17

andrewkroh approved these changes

View reviewed changes

Member

andrewkroh left a comment

LGTM

libbeat/processors/cache/file_store.go Outdated Show resolved Hide resolved

efd6 force-pushed the i2816-cache_processor_file_backed branch 3 times, most recently from fca8d75 to 878dcb4 Compare

September 28, 2023 03:01


          address pr comment

8238e85

efd6 force-pushed the i2816-cache_processor_file_backed branch from 878dcb4 to 8238e85 Compare

September 28, 2023 03:07

andrewkroh approved these changes

View reviewed changes


          fix config validation bug

dc1eb11

efd6 requested a review from andrewkroh

September 28, 2023 03:13

andrewkroh approved these changes

View reviewed changes

efd6 merged commit 9e503b4 into elastic:main

6 of 8 checks passed

Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request


          libbeat/processors/cache: add file-backed cache (elastic#36686)

2eee678

This adds a file-backed cache implementation to the cache processor. Caching
between put and get operations is done in-memory using the memory cache, but
the file cache will load previously written cache state on start-up and will
write cache contents to file when the cache is dropped. Depending on user
configuration, the file cache will also periodically write the cache state to
the backing file to reduce state loss in the event of a crash.

For simplicity, the cache state is stored as a JSON stream of objects with
fields for the key, value and expiry timestamp of cached entities.

efd6 mentioned this pull request

[libbeat] Cache processor docs and memory fixes. #38561

Merged

6 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.11-candidate backport-skip enhancement libbeat