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(topology): correct GraphQL schema for Topology actions, implement missing mutations #315

Merged
merged 19 commits into from
Mar 15, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 5, 2024

Welcome to Cryostat3! 👋

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


Based on #294
Fixes #302
Fixes #304
Fixes #305

Description of the change:

Implements missing features, fixes up a few unnecessary schema differences, and does some refactoring.

Motivation for the change:

These changes support the existing Topology view.

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... bash smoketest.bash...
  2. ...

Known Issues

Using the group-level Topology actions for starting a recording and then archiving that recording will frequently end up with the recording files uploaded into the wrong S3 subdirectory. The metadata labels will reflect the correct JVM ID, but the file will be listed under the wrong Target. Need to investigate why this happens.

Copy link

github-actions bot commented Mar 5, 2024

This PR/issue depends on:

@andrewazores
Copy link
Member Author

The following subqueries/mutations are still missing from the schema:

type ArchivedRecording implements Recording {
    doDelete: ArchivedRecording!
    doPutMetadata(metadata: Object): ArchivedRecording!
}

type ActiveRecording implements Recording {
    doPutMetadata(metadata: Object): ActiveRecording
}

@aali309 would you like to tackle those? The archived recording deletion action already exists in Recordings.java, so it would be a matter of refactoring the implementation somewhere reusable (like RecordingHelper) and then adding the subquery to call that refactored code. In Recordings and RecordingHelper there are also already several places where recording metadata is handled, so adding some methods and subqueries to allow updating that via GraphQL should not be too complicated.

@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Mar 5, 2024

Workflow started at 3/5/2024, 9:35:11 AM. View Actions Run.

Copy link

github-actions bot commented Mar 5, 2024

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8158013179

Copy link

github-actions bot commented Mar 5, 2024

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8158013179

@andrewazores andrewazores requested review from aali309 and mwangggg March 5, 2024 18:31
@andrewazores
Copy link
Member Author

Using the group-level Topology actions for starting a recording and then archiving that recording will frequently end up with the recording files uploaded into the wrong S3 subdirectory. The metadata labels will reflect the correct JVM ID, but the file will be listed under the wrong Target. Need to investigate why this happens.

Examining the smoketest Seaweed instance's own file explorer UI (localhost:8888) and using other non-GraphQL HTTP API requests, the bug is something wrong with the GraphQL responses. The data is correctly filed in the S3 storage backend and the other API endpoints return it correctly. But for whatever reason, GraphQL queries like targetNodes { target { recordings { archived { aggregate { count } } } } get it wrong. What looks like is happening is that the aggregate and data fields of the archived recordings structure is incompletely populated. When expanding one of the table rows here a follow-up query is made, which ends up requesting archived recordings belonging to no JVM ID, which then results in all of the archived recordings being displayed within that table. So there are two distinct bugs:

  1. the archived recordings structure is incorrectly populated
  2. the follow-up request for per-target archived recordings does not pick up the correct target for the query and results in listing all

@andrewazores
Copy link
Member Author

@aali309 sorry for all the noise and new commits, but I did some cleaning up and I believe between this PR and cryostatio/cryostat-web#1224 I have also solved the bug with the Archives > All Targets view.

@andrewazores
Copy link
Member Author

Sometimes there is still an odd bug where Topology > Group Action > Archive Recording does not actually result in recordings from all the targets in the group being archived. That's also worth investigating some more but I'll do that as a follow-up. It seems like the Start/Stop/Delete actions on the group level all work reliably.

@andrewazores andrewazores requested a review from a team as a code owner March 7, 2024 17:37
@andrewazores andrewazores force-pushed the graphql-topology branch 2 times, most recently from 667cd94 to df7bef9 Compare March 7, 2024 19:18
@andrewazores
Copy link
Member Author

Last couple commits fix up a behavioural conflict between #291, the GraphQL POST endpoint(s), and the discovery publication endpoint. I'm done messing with this now, anything further will come in separate follow-up PRs. @aali309 @mwangggg please re-review.

@andrewazores andrewazores merged commit e9f58a8 into cryostatio:graphql Mar 15, 2024
12 checks passed
@andrewazores andrewazores deleted the graphql-topology branch March 15, 2024 15:37
andrewazores added a commit to andrewazores/cryostat3 that referenced this pull request Mar 20, 2024
andrewazores added a commit that referenced this pull request Mar 20, 2024
andrewazores added a commit to andrewazores/cryostat3 that referenced this pull request Mar 20, 2024
andrewazores added a commit that referenced this pull request Mar 20, 2024
andrewazores added a commit that referenced this pull request Mar 22, 2024
andrewazores added a commit that referenced this pull request Apr 13, 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.

2 participants