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(recordings): use safe recording close on cleanup #763

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

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jan 2, 2025

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 all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #760

Description of the change:

  1. Uses the safeCloseRecording method in two places where it wasn't before. Without this, any target connection failure would throw an exception and cause an early return from the callsites, which could lead to unexpected behaviour like orphaned resources left behind. This is the real fix.
  2. Extracts some constants. This is just some cleanup.
  3. Sets the smoketest logging verbosity as an optional setting. This was previously increased from the default INFO level to ALL in all cases, but I found that this actually interfered with the smoketest operation at times due to the extremely high verbosity - actions like archiving recordings actually saw a performance degradation because every byte written out by Cryostat would be logged, which drastically slowed the I/O and could cause operations to time out.

Motivation for the change:

This change is helpful because users may want to...

How to manually test:

  1. Check out and build PR
  2. ./smoketest.bash -Ot (pass -v one or two times to enable verbose logging)
  3. On various targets, try creating recordings, stopping them, archiving them, etc. and ensure no "Clone of x" recordings appear

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 2, 2025
@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Jan 2, 2025
@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Jan 2, 2025

Workflow started at 1/2/2025, 10:55:42 AM. View Actions Run.

@andrewazores andrewazores marked this pull request as ready for review January 2, 2025 15:56
@andrewazores andrewazores requested a review from a team January 2, 2025 15:56
Copy link

github-actions bot commented Jan 2, 2025

No OpenAPI schema changes detected.

Copy link

github-actions bot commented Jan 2, 2025

No GraphQL schema changes detected.

Copy link

github-actions bot commented Jan 2, 2025

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat/actions/runs/12585092981

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.

[Bug] "Clone of x" recordings appear
1 participant