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

[receiver/awscontainerinsight] Fix memory leak on shutdown in k8s API server #32405

Conversation

crobert-1
Copy link
Member

Description:

This change has 4 main parts:

  1. Shutdown the k8s API server's broadcaster during Shutdown as it has its own running goroutines that need shutdown to avoid leaking.
  2. Call Shutdown in the failure scenarios of New. This is required because once again, goroutines are started during the k8s API server's New call stack. Since it returns nil on error, there's no way to shutdown once it returns. This means Shutdown must be called on error.
  3. Only set the server's broadcaster after passed in options are complete. This will help avoid a goroutine leak as well, as explained in the added comment in the code.
  4. Add goleak checks to help ensure no goroutines are being leaked.

Link to tracking Issue:
#30438

Testing:
All existing tests are passing, as well as added goleak checks.

@crobert-1 crobert-1 requested a review from Aneurysm9 as a code owner April 15, 2024 23:59
@crobert-1 crobert-1 requested a review from a team April 15, 2024 23:59
@crobert-1
Copy link
Member Author

Failures are frequencies of #32391 and #32396, I've added references on both issues.

Copy link
Contributor

github-actions bot commented May 7, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 7, 2024
@crobert-1 crobert-1 removed the Stale label May 7, 2024
@fatsheep9146
Copy link
Contributor

nicely ping @Aneurysm9 for review

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 18, 2024
@crobert-1 crobert-1 removed the Stale label Jun 18, 2024
@atoulme
Copy link
Contributor

atoulme commented Jun 19, 2024

@Aneurysm9 please review

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 15, 2024
@crobert-1 crobert-1 removed the Stale label Aug 15, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 30, 2024
@atoulme
Copy link
Contributor

atoulme commented Sep 4, 2024

Pinging code owners @Aneurysm9 @pxaws

Copy link
Contributor

github-actions bot commented Oct 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Copy link
Contributor

github-actions bot commented Nov 2, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants