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

fix(fd): close leaked file descriptors #1445

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Apr 12, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #1444
Depends on cryostatio/cryostat-core#207 (these changes are really separate, but I want to update -core and include that dependency update in this same PR for simplicity)

Description of the change:

This simply closes some file descriptors that are currently leaked.

Motivation for the change:

Don't leak resources over time.

How to manually test:

See test procedure in cryostatio/cryostat-core#207 . Same thing applies. ls -l /proc/$pid/fd is useful for checking which descriptors look like they've been leaked. I used the following to ease tracing where those descriptors got opened:

https://github.com/jenkinsci/lib-file-leak-detector downloaded and placed into my cryostat/clientlib project directory, named as file-leak-detector.jar.

diff --git a/src/main/extras/app/entrypoint.bash b/src/main/extras/app/entrypoint.bash
index cd83fb51..2ed548ef 100755
--- a/src/main/extras/app/entrypoint.bash
+++ b/src/main/extras/app/entrypoint.bash
@@ -166,6 +166,8 @@ if [ -n "$CRYOSTAT_JUL_CONFIG" ]; then
     FLAGS+=("-Djava.util.logging.config.file=$CRYOSTAT_JUL_CONFIG")
 fi
 
+FLAGS+=("-javaagent:/clientlib/file-leak-detector.jar=threshold=200,trace,strong")
+

tthvo
tthvo previously approved these changes Apr 12, 2023
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good! Another flanky credential tests :))

@github-actions
Copy link
Contributor

This PR/issue depends on:

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1445-8c2a54485e0b5f9f5fa80344560ddb8cdde385ac sh smoketest.sh

@andrewazores andrewazores merged commit ef36883 into cryostatio:main Apr 12, 2023
@andrewazores andrewazores deleted the close-file-descriptors branch April 12, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Cryostat keeps failing with "Too many open files", exhausts descriptors
4 participants