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

feat(uploads): add maxFiles configuration policy to jvmId uploaded recordings #1333

Merged
merged 8 commits into from
Jan 17, 2023

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Jan 16, 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: #1306

Description of the change:

This change allows any client which posts a recording to the target-specific upload handler RecordingsFromIdPostHandler to specify the maxFiles parameter which keeps a set amount of archived recording files per target, depending on the given parameter. If the parameter is not specified, then it is defaulted to the env variable CRYOSTAT_PUSH_MAX_FILES. If that env var is not specified, the global maxFiles value is Integer.MAX_INT.

Motivation for the change:

See #1306 for discussion.

How to manually test:

  1. Run normally...
  2. On a target e.g. (cryostat:9093), start a recording and archive it (turn off archiveOnStop to make it easier).
  3. Find the target's jvmId.
  4. Then ->
$ https --verify=no -f -v POST :8181/api/beta/recordings/${jvmId} recording@testfolder/jfr/quarkus-test-agent_default_20230109T001426Z.jfr maxFiles=1 --auth=user:pass 

Notice the extra new paramter maxFiles. If we set it to 1, it will delete all archived files within the target except the one that was included in this upload form. If set to anything above 1, it will keep the old archived recording, plus the recent uploaded one that is included in the form.

Experiment with settings, (e.g. without a maxFiles, this should default to the environment variable config that was either set, or defaulted to Integer.MAX_INT)

@maxcao13 maxcao13 added the feat New feature or request label Jan 16, 2023
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@mergify mergify bot added the safe-to-test label Jan 16, 2023
@maxcao13 maxcao13 changed the title added maxFiles config to idPostHandler feat(uploads): add maxFiles configuration policy to jvmId uploaded recordings Jan 16, 2023
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
mergify[bot]
mergify bot previously requested changes Jan 16, 2023
Copy link

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Pull Request blocked. web-client submodule updates are performed automatically by CI when that repository is updated. Please revert or drop all changes to the web-client submodule from this PR and perform any required frontend work by opening and merging a PR against cryostat-web.

@maxcao13 maxcao13 force-pushed the target-upload-policies branch from 757236b to 4cb90b9 Compare January 16, 2023 21:58
@maxcao13 maxcao13 removed the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1333-4cb90b936aabdba6487f63bf2001ffcdb23df30f sh smoketest.sh

@maxcao13
Copy link
Member Author

Thoughts before tests?

@maxcao13 maxcao13 force-pushed the target-upload-policies branch from 4cb90b9 to 681e442 Compare January 17, 2023 00:02
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 17, 2023
@maxcao13 maxcao13 removed the needs-triage Needs thorough attention from code reviewers label Jan 17, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1333-681e4425914aada56d3a839cd47d721340c4316e sh smoketest.sh

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 17, 2023
@maxcao13 maxcao13 removed the needs-triage Needs thorough attention from code reviewers label Jan 17, 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 to me!

I was just seeing some front-end bug that renders duplicated archives. Try uploading an archive while in archive table view and there will be duplicate recordings.

Below, a single uploaded notifications renders an additional of 3 rows.

Screenshot from 2023-01-16 20-20-10

Probably another hook deps issue that eslint misses. Should be similar the upload table issue but we might forget to fix this too.

EDIT: See cryostatio/cryostat-web#820

I think PR is looking great. This bug probably can be solved separately on front-end.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1333-d6e1bd9a47a85aa46bf33e86a2dfe7952ead9316 sh smoketest.sh

Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13 maxcao13 force-pushed the target-upload-policies branch from d6e1bd9 to 6662bbe Compare January 17, 2023 16:42
@maxcao13 maxcao13 marked this pull request as ready for review January 17, 2023 16:42
@maxcao13 maxcao13 requested a review from mergify[bot] January 17, 2023 16:43
@tthvo
Copy link
Member

tthvo commented Jan 17, 2023

Spotless?

Signed-off-by: Max Cao <macao@redhat.com>
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1333-af3185d17464f853b2b73adcf206d1562d909f49 sh smoketest.sh

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Just needs a note added to the README describing the new variable.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1333-e23fb62ebde3d109e5efc116b663ef56da6f90cb sh smoketest.sh

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 great to me :D Worked well.

@andrewazores andrewazores merged commit c980a6f into cryostatio:main Jan 17, 2023
@maxcao13 maxcao13 deleted the target-upload-policies branch January 17, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Policies for pruning target-specific uploads
3 participants