[auditbeat] - Migration of system/package module to flatbuffers #34817

ShourieG · 2023-03-13T15:16:03Z

Type of change

Enhancement

What does this PR do?

This PR is responsible for migrating the storage encoding of packages in auditbeat system/package module from traditional
gob encoding to flatbuffers encoding. This change also makes sure that existing gob encoded packages for users are seamlessly migrated to flatbuffer encoding without loss of any data.

Why is it important?

This migration greatly improves the performance of the system/package module and brings it upto modern standards.
Benchmark tests will soon be added to record performance.

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 .

Related issues

Relates [Auditbeat] Prepare System Package to be GA #33765 [Auditbeat Enhancement] Implement flat buffers & improve state persistence #34441

elasticmachine · 2023-03-13T15:16:29Z

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

botelastic · 2023-03-13T15:16:38Z

This pull request doesn't have a Team:<team> label.

mergify · 2023-03-13T15:16:39Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ShourieG? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-03-13T15:27:50Z

💚 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-04-04T09:24:12.355+0000
Duration: 42 min 30 sec

Test stats 🧪

Test	Results
Failed	0
Passed	602
Skipped	81
Total	683

💚 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!)

x-pack/auditbeat/module/system/package/faltbuffers.go

x-pack/auditbeat/module/system/package/package.fbs

x-pack/auditbeat/module/system/package/package.go

x-pack/auditbeat/module/system/package/faltbuffers.go

x-pack/auditbeat/module/system/package/flatbuffers_test.go

x-pack/auditbeat/module/system/package/faltbuffers.go

ShourieG · 2023-03-20T09:26:21Z

@efd6 addressed all the issues, added support for errors in the package flatbuff and also added a pool poison test.

x-pack/auditbeat/module/system/package/flatbuff.go

x-pack/auditbeat/module/system/package/flatbuff_test.go

x-pack/auditbeat/module/system/package/schema.fbs

ShourieG · 2023-03-23T09:47:41Z

Updated the PR with the recommended changes @efd6

x-pack/auditbeat/module/system/package/flatbuff.go

andrewkroh · 2023-03-24T17:50:10Z

x-pack/auditbeat/module/system/package/schema.fbs

+	summary:string;
+	url:string;
+	type:string;
+	error:string;


What is the persisted error needed for?

I tracked this down. When a package is removed the same data that was original detected (including the errors) are transmitted in the removal event.

Correction... so the error field is not exported. And to quote the encoding/gob docs, "Structs encode and decode only exported fields."

beats/x-pack/auditbeat/module/system/package/package.go

Line 127 in 62a2700

error error

This means that the error is not serialized by encoding/gob and the value is not persisted. So this will have a slightly different behavior than we had before. But originally this was designed to persist the error, and there were some issues with serialization (see #18536) so it was changed. Given that the original design was to include the error let's keep it in the flatbuffer schema.

x-pack/auditbeat/module/system/package/package.go

x-pack/auditbeat/module/system/package/schema.fbs

x-pack/auditbeat/module/system/package/flatbuff.go

…during metric set initialization

ShourieG · 2023-03-29T13:18:47Z

@andrewkroh reworked the bucket migration process to have it migrate to a new bucket during metricset init

x-pack/auditbeat/module/system/package/flatbuffers.go

andrewkroh · 2023-03-30T21:24:38Z

auditbeat/datastore/datastore.go

+
+	// Initialize the Bolt DB.
+	if ds.db == nil {
+		var err error


This code is repeated a lot. How about adding a boltDatastore.init() error method.

x-pack/auditbeat/module/system/package/package.go

andrewkroh · 2023-03-31T13:54:36Z

x-pack/auditbeat/module/system/package/schema.fbs

+	summary:string;
+	url:string;
+	type:string;
+	error:string;


Correction... so the error field is not exported. And to quote the encoding/gob docs, "Structs encode and decode only exported fields."

beats/x-pack/auditbeat/module/system/package/package.go

Line 127 in 62a2700

error error

This means that the error is not serialized by encoding/gob and the value is not persisted. So this will have a slightly different behavior than we had before. But originally this was designed to persist the error, and there were some issues with serialization (see #18536) so it was changed. Given that the original design was to include the error let's keep it in the flatbuffer schema.

x-pack/auditbeat/module/system/package/package.go

x-pack/auditbeat/module/system/package/flatbuffers.go

x-pack/auditbeat/module/system/package/flatbuffers_test.go

x-pack/auditbeat/module/system/package/flatbuffers.go

andrewkroh · 2023-03-31T20:04:47Z

While I was reviewing I was making changes in my local copy to test some ideas. I’m going to push to push that commit into your branch so you can see it. I think it better expresses some of the thoughts I had in my review. You don’t have to keep the commit (we can rebase to remove it).

Use a bolt write transaction to perform the entire schema migration such that failures result in a full rollback. The transaction also prevents any accidently concurrent migration issues. I added a test case for the migration that works on a boltdb file that contains package.v1 data from homebrew. Any test cases involving encoding/gob encoding were removed. We only care that we can continue to decode the old gob data. I also addressed all golangci-lint warnings.

ShourieG · 2023-04-04T09:22:24Z

@andrewkroh I went through the changes you made and they look really good, and the migration occurring inside of a bolt transaction solves the multiple instance issue. I think we can move ahead with your suggested changes.

mergify · 2023-04-04T09:23:00Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ShourieG? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

* initial working commit * flatbuffers migration now working with tests * updated changelog * removed comments * resolved PR suggetions and updated license * updated notice * updated tests, added benchmarks, renamed files * updated tests & added error storage in schema * license issue fixed * updated according to PR suggestions * refactored package migration based on bucket version and is now done during metric set initialization * Perform migration inside of a bolt transaction, fix linter warnings Use a bolt write transaction to perform the entire schema migration such that failures result in a full rollback. The transaction also prevents any accidently concurrent migration issues. I added a test case for the migration that works on a boltdb file that contains package.v1 data from homebrew. Any test cases involving encoding/gob encoding were removed. We only care that we can continue to decode the old gob data. I also addressed all golangci-lint warnings. --------- Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>

ShourieG added 2 commits March 10, 2023 19:40

initial working commit

dcf6e76

flatbuffers migration now working with tests

870305f

ShourieG requested a review from a team as a code owner March 13, 2023 15:16

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 13, 2023

ShourieG self-assigned this Mar 13, 2023

ShourieG added the Team:Security-External Integrations label Mar 13, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 13, 2023

ShourieG added the 8.8-candidate label Mar 13, 2023

updated changelog

cc8592b

ShourieG requested review from andrewkroh and efd6 March 13, 2023 15:20

ShourieG added 2 commits March 13, 2023 20:52

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

c38ae22

removed comments

1cffa01

efd6 reviewed Mar 13, 2023

View reviewed changes

resolved PR suggetions and updated license

aaf3fc2

ShourieG requested a review from a team as a code owner March 14, 2023 11:47

ShourieG requested review from rdner and leehinman and removed request for a team March 14, 2023 11:47

updated notice

5d0f6e4

efd6 reviewed Mar 14, 2023

View reviewed changes

x-pack/auditbeat/module/system/package/faltbuffers.go Outdated Show resolved Hide resolved

ShourieG added 4 commits March 15, 2023 11:37

updated tests, added benchmarks, renamed files

4ac7ef8

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

2d55de8

updated tests & added error storage in schema

7525656

license issue fixed

7166fb3

efd6 reviewed Mar 22, 2023

View reviewed changes

ShourieG added 2 commits March 23, 2023 14:53

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

4d7bee9

updated according to PR suggestions

d6f553a

andrewkroh reviewed Mar 24, 2023

View reviewed changes

ShourieG added 3 commits March 28, 2023 11:31

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

2aa8112

refactored package migration based on bucket version and is now done …

892e37e

…during metric set initialization

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

15e1fd1

ShourieG added the backport-v8.7.0 Automated backport with mergify label Mar 30, 2023

Merge remote-tracking branch 'elastic/main' into auditbeat/flat_buffers

f8369d9

andrewkroh reviewed Mar 31, 2023

View reviewed changes

ShourieG removed the backport-v8.7.0 Automated backport with mergify label Apr 4, 2023

Merge remote-tracking branch 'upstream/main' into auditbeat/flat_buffers

c7feb02

andrewkroh approved these changes Apr 4, 2023

View reviewed changes

ShourieG merged commit 804e9c5 into elastic:main Apr 5, 2023
25 checks passed

ShourieG deleted the auditbeat/flat_buffers branch April 5, 2023 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[auditbeat] - Migration of system/package module to flatbuffers #34817

[auditbeat] - Migration of system/package module to flatbuffers #34817

ShourieG commented Mar 13, 2023 •

edited

elasticmachine commented Mar 13, 2023

botelastic bot commented Mar 13, 2023

mergify bot commented Mar 13, 2023

elasticmachine commented Mar 13, 2023 •

edited by jenkins-beats-ci bot

Build stats

Test stats 🧪

ShourieG commented Mar 20, 2023 •

edited

ShourieG commented Mar 23, 2023

andrewkroh Mar 24, 2023

andrewkroh Mar 24, 2023

andrewkroh Mar 31, 2023

ShourieG commented Mar 29, 2023

andrewkroh Mar 30, 2023

andrewkroh Mar 31, 2023

andrewkroh commented Mar 31, 2023

ShourieG commented Apr 4, 2023

mergify bot commented Apr 4, 2023

[auditbeat] - Migration of system/package module to flatbuffers #34817

[auditbeat] - Migration of system/package module to flatbuffers #34817

Conversation

ShourieG commented Mar 13, 2023 • edited

Type of change

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Mar 13, 2023

botelastic bot commented Mar 13, 2023

mergify bot commented Mar 13, 2023

elasticmachine commented Mar 13, 2023 • edited by jenkins-beats-ci bot

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

ShourieG commented Mar 20, 2023 • edited

ShourieG commented Mar 23, 2023

andrewkroh Mar 24, 2023

Choose a reason for hiding this comment

andrewkroh Mar 24, 2023

Choose a reason for hiding this comment

andrewkroh Mar 31, 2023

Choose a reason for hiding this comment

ShourieG commented Mar 29, 2023

andrewkroh Mar 30, 2023

Choose a reason for hiding this comment

andrewkroh Mar 31, 2023

Choose a reason for hiding this comment

andrewkroh commented Mar 31, 2023

ShourieG commented Apr 4, 2023

mergify bot commented Apr 4, 2023

ShourieG commented Mar 13, 2023 •

edited

elasticmachine commented Mar 13, 2023 •

edited by jenkins-beats-ci bot

ShourieG commented Mar 20, 2023 •

edited