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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

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
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.

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.

None yet

2 participants