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

Update system/socket dataset to support config reloading #21693

Merged
merged 3 commits into from
Nov 24, 2020
Merged

Update system/socket dataset to support config reloading #21693

merged 3 commits into from
Nov 24, 2020

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Oct 8, 2020 •
edited
Loading

What does this PR do?

This updates the system/socket dataset so that it behaves correctly when run under config reloader.

When a dataset is run from a static module definition in auditbeat.yml:

  • Factory method New is called.
  • (instance).Run is called.

When the dataset is configured in the context of configuration reloading:

  • Factory method New is called to validate that the config is OK. The returned instance is discarded without its context being closed.
  • Factory method New is called again.
  • (instance).Run is called for the instance returned by the second New.

Also, if reload.enable is true, everytime the configuration file changes:

  • The metricset context is closed.
  • A new instance is created using New.
  • (instance).Run is called.

This behavior didn't play well with a dataset with complex initialization (can take up to 20s to start) and that uses global OS resources (kprobes) that need to be released before a new dataset can be instantiated. There's no mechanism for the reloader to wait for a metricset's termination.

This updates the dataset factory method to keep track of an already running instance. Also it checks if the config has changed to prevent initializing two metricsets at startup.

Why is it important?

Currently Auditbeat will fail with a cryptic error when the socket dataset is started using config reloading. See #20851.

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.

How to test this PR locally

See the linked issue.

Related issues

@adriansr adriansr requested a review from a team as a code owner October 8, 2020 17:13
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 8, 2020
adriansr
adriansr commented Oct 8, 2020
View reviewed changes
x-pack/auditbeat/module/system/socket/guess/creds.go
@@ -51,7 +51,7 @@ const (
)

func init() {
if err := Registry.AddGuess(&guessStructCreds{}); err != nil {
if err := Registry.AddGuess(func() Guesser { return &guessStructCreds{} }); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the changes to guesses are required because, turns out, every guess instance could only run once, as some of them keep internal state (counters, etc.). Replacing the registration with a factory method ensures that when config is reloaded a fresh set of guesses is run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update #21011 at some point to reflect this and probably get rid of the global registry for Guesses.

@adriansr adriansr requested review from a team and removed request for a team October 8, 2020 17:26
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 8, 2020 •
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
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #21693 updated]

  • Start Time: 2020-11-24T09:38:46.037+0000

  • Duration: 23 min 4 sec

Test stats 🧪

Test Results
Failed 0
Passed 228
Skipped 33
Total 261

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 228
Skipped 33
Total 261

@botelastic
Copy link

botelastic bot commented Nov 12, 2020

Hi!
We just realized that we haven't looked into this PR in a while. We're sorry!

We're labeling this issue as Stale to make it hit our filters and make sure we get back to it in as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1.
Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Nov 12, 2020
andrewstucki
andrewstucki approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

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

This seems fine to me, but quick question about the execution context of the metricset initialization code.

x-pack/auditbeat/module/system/socket/socket_linux.go Outdated Show resolved Hide resolved
@botelastic botelastic bot removed the Stalled label Nov 12, 2020
@adriansr adriansr merged commit 04b064e into elastic:master Nov 24, 2020
adriansr added a commit to adriansr/beats that referenced this pull request Nov 24, 2020
@adriansr
Update system/socket dataset to support config reloading (elastic#21693)
861a6a4 
* Update system/socket dataset to support config reloading

* Thread-safe singleton

(cherry picked from commit 04b064e)
@adriansr adriansr mentioned this pull request Nov 24, 2020
Merged
3 tasks
@adriansr adriansr added v7.11.0 and removed needs_backport PR is waiting to be backported to other branches. labels Nov 24, 2020
adriansr added a commit to adriansr/beats that referenced this pull request Nov 24, 2020
@adriansr
Update system/socket dataset to support config reloading (elastic#21693)
d72ebb7 
* Update system/socket dataset to support config reloading

* Thread-safe singleton

(cherry picked from commit 04b064e)
@adriansr adriansr mentioned this pull request Nov 24, 2020
Merged
3 tasks
adriansr added a commit that referenced this pull request Nov 24, 2020
@adriansr
Update system/socket dataset to support config reloading (#21693) (#2…
a0e2346 
…2730)

Update system/socket dataset to support config reloading

(cherry picked from commit 04b064e)
adriansr added a commit that referenced this pull request Nov 24, 2020
@adriansr
Update system/socket dataset to support config reloading (#21693) (#2…
772ffb3 
…2731)

* Update system/socket dataset to support config reloading

(cherry picked from commit 04b064e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewstucki andrewstucki andrewstucki approved these changes

Assignees
No one assigned
Labels
Auditbeat bug review v7.10.1 v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auditbeat system/socket fails when started by config reloading
3 participants
@adriansr @elasticmachine @andrewstucki