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

Add do.Once protection to beater interface #33971

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

fearful-symmetry
Copy link
Contributor

What does this PR do?

This is a fix for #32608, which in turn I'm hoping might fix some of the issues with the pidfile fix here: #31670 (comment)

This wraps the beater Stop() call used by the beats in a once.Do() statement in order to prevent a "close of closed channel" panic. This appears to happening under agent, presumably due to a SIGINT handler calling Stop() at the same time another component is shutting down and also trying to call Stop(). There's no added changes for auditbeat, since it just imports the metricbeat beater interface.

I've tested this by prodding elastic-agent with a restart command, can't get the panic again.

Why is it important?

We don't want beats to randomly panic.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@fearful-symmetry fearful-symmetry self-assigned this Dec 7, 2022
@fearful-symmetry fearful-symmetry requested review from a team as code owners December 7, 2022 00:10
@fearful-symmetry fearful-symmetry requested review from belimawr and faec and removed request for a team December 7, 2022 00:10
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 7, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 7, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @fearful-symmetry? 🙏.
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

@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @fearful-symmetry? 🙏.
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
Copy link
Collaborator

elasticmachine commented Dec 7, 2022

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-12-07T20:40:31.666+0000

  • Duration: 36 min 34 sec

❕ Flaky test report

No test was executed to be analysed.

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

@cmacknz cmacknz added the backport-v8.6.0 Automated backport with mergify label Dec 7, 2022
@cmacknz
Copy link
Member

cmacknz commented Dec 7, 2022

Do need to make this change for x-pack/osquerybeat? That's the only beat I don't see touched or explained already.

@fearful-symmetry
Copy link
Contributor Author

@cmacknz osquerybeat has its own protections built in:

func (bt *osquerybeat) close() {

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@fearful-symmetry
Copy link
Contributor Author

/test

@fearful-symmetry fearful-symmetry merged commit 0c77112 into elastic:main Dec 7, 2022
mergify bot pushed a commit that referenced this pull request Dec 7, 2022
* add channel protection to beater interface

* add changelog

(cherry picked from commit 0c77112)
fearful-symmetry added a commit that referenced this pull request Dec 8, 2022
* add channel protection to beater interface

* add changelog

(cherry picked from commit 0c77112)

Co-authored-by: Alex K <[email protected]>
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
* add channel protection to beater interface

* add changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.6.0 Automated backport with mergify bug Team:Elastic-Agent Label for the Agent team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants