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(agent): enable Agent HTTP communications #342

Merged
merged 47 commits into from
Apr 23, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 26, 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


Related to #2
Depends on cryostatio/cryostat-core#363
Depends on #343

Description of the change:

Reimplements Agent HTTP communications. AgentConnection and AgentJFRService are basically copy-pasted from the 2.4 codebase. AgentClient is mostly a copy-paste but needed a little more touching up. Overall within these classes there are relatively minor adjustments to use Uni rather than CompletableFuture, and to use Jackson serialization instead of Gson.

There is also other refactoring and cleanup:

  • handle cases where there are multiple target definitions pointing to the same actual target (see also [Epic] Redefine Target data model to allow multiple Connection URLs #71). In most other situations this would be deemed an inappropriate deployment scenario where multiple discovery mechanisms are overlapping, though the smoketest does this intentionally to exercise how it is handled. However in the case of Agent-equipped applications then this is completely normal, since the Agent will always register itself with HTTP and will secondarily register with JMX if it detects that JMX is enabled on its host JVM
  • handle cases where recordings exist on targets, where Cryostat did not create the recording to begin with, and the recording was not already there at the time that Cryostat discovered the target. Previously Cryostat would only ever know about active recordings that it either started on its own, or which it observed when performing a discovery-time probe of the target. Additional recordings created later either by the application itself or by other external tools - including ones created by the Agent and its Harvester - could never be observed
  • improvements for WebSocket message queue handling
  • improvements for error handling during discovery plugin registration

Current bugs:

  • stopping Agent HTTP recordings results in a (infinite?) loop of stoppage requests
  • create a recording name X. Delete that recording. Attempt to create a new recording also named X - this fails due to the name being a duplicate. Navigating away and back shows that the deletion actually failed. Server stack trace appears to indicate that the deletion request to the agent fails, but the agent's own logs do not indicate that it ever received the deletion request.

Motivation for the change:

#2 feature parity, 3.0 should be able to interact with Agent HTTP targets with full feature parity as JMX connections.

How to manually test:

  1. Check out this PR, ./mvnw clean package
  2. ./bash smoketest.bash -Ot
  3. Check the localhost:0 and cryostat3:9091 targets. They should now both list the same two startup recordings. Select one of them and create a new recording, observe that it is listed in the recordings table, then switch targets and observe that the recording is also listed in this table. There is a minor bug here: if the recording is fixed-duration and has "archive on stop" selected, then the recording table state will only update if the selected target definition is the one where the recording was created, not for other aliases. The real fix for this bug would be [Epic] Redefine Target data model to allow multiple Connection URLs #71. Navigating away and back or otherwise refreshing the view will result in an updated table content anyway.
  4. Check the various http:// Agent targets. The quarkus-test-agent one is configured with API_WRITES_ENABLED, the others are not. All of the Agent targets should allow you to list event templates and event types, and list active recordings. Only the quarkus-test-agent should also allow creating new custom recordings and new snapshots. You should also be able to archive, generate reports, view in Grafana, etc. on these active recordings from Agent HTTP targets.

@andrewazores andrewazores force-pushed the agent-client branch 2 times, most recently from 4ef597e to c3def0a Compare March 28, 2024 20:30
@andrewazores andrewazores marked this pull request as ready for review April 2, 2024 18:46
@andrewazores andrewazores requested a review from a team as a code owner April 2, 2024 18:46
@andrewazores andrewazores requested review from aali309 and mwangggg April 2, 2024 18:47
@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Apr 2, 2024

Workflow started at 4/2/2024, 2:47:43 PM. View Actions Run.

Copy link

github-actions bot commented Apr 2, 2024

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

Copy link

github-actions bot commented Apr 2, 2024

OpenAPI schema change detected:

diff --git a/schema/openapi.yaml b/schema/openapi.yaml
index b7db9ef..21cc2e9 100644
--- a/schema/openapi.yaml
+++ b/schema/openapi.yaml
@@ -2015,20 +2015,21 @@ paths:
                   type: integer
                 metadata:
                   nullable: true
                   type: string
                 recordingName:
                   type: string
                 replace:
                   nullable: true
                   type: string
                 restart:
+                  deprecated: true
                   nullable: true
                   type: boolean
                 toDisk:
                   nullable: true
                   type: boolean
               type: object
       responses:
         "200":
           description: OK
         "401":

Copy link

github-actions bot commented Apr 2, 2024

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

@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Apr 9, 2024

Workflow started at 4/9/2024, 4:11:36 PM. View Actions Run.

Copy link

github-actions bot commented Apr 9, 2024

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

Copy link

github-actions bot commented Apr 9, 2024

OpenAPI schema change detected:

diff --git a/schema/openapi.yaml b/schema/openapi.yaml
index b7db9ef..21cc2e9 100644
--- a/schema/openapi.yaml
+++ b/schema/openapi.yaml
@@ -2015,20 +2015,21 @@ paths:
                   type: integer
                 metadata:
                   nullable: true
                   type: string
                 recordingName:
                   type: string
                 replace:
                   nullable: true
                   type: string
                 restart:
+                  deprecated: true
                   nullable: true
                   type: boolean
                 toDisk:
                   nullable: true
                   type: boolean
               type: object
       responses:
         "200":
           description: OK
         "401":

Copy link

github-actions bot commented Apr 9, 2024

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

@mwangggg mwangggg added security Pull requests that address a security vulnerability and removed security Pull requests that address a security vulnerability labels Apr 15, 2024
@mwangggg
Copy link
Member

I noticed that there are snapshots being created every 30 seconds in the quarkus-test-agent target when I first start up Cryostat. They are stopped after I create a recording in quarkus-test-agent and they can't be deleted- neither can their reports be generated.
Screenshot from 2024-04-14 22-19-42

@andrewazores
Copy link
Member Author

andrewazores commented Apr 15, 2024

The 30 second snapshots/archives are expected, those are from the Agent's Harvester which the sample app in the smoke test is configured for. But IIRC they're supposed to be closed (deleted) on the Agent side and probably shouldn't show up in Cryostat's recording listing... But if you hit the timing right I suppose you would see it while it exists, and then the Agent deletes it and Cryostat wouldn't know. Then by the time you ask to interact with that recording it has already been deleted by the Harvester so your request fails.

Maybe there's some better handling for it I can apply somehow. My first thought is that the Agent can apply a label to the snapshots to tag them in the listing results, and Cryostat can filter those out (or the Agent can just not report about them).

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/19/2024, 4:23:28 PM. View Actions Run.

Copy link

No OpenAPI schema changes detected.

Copy link

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

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/19/2024, 4:30:45 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

Copy link

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

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/22/2024, 11:06:28 AM. View Actions Run.

Copy link

No OpenAPI schema changes detected.

Copy link

No GraphQL schema changes detected.

Copy link

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

Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

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

lgtm

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/23/2024, 2:21:25 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

Copy link

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

@andrewazores andrewazores merged commit 05060e5 into cryostatio:main Apr 23, 2024
8 checks passed
@andrewazores andrewazores deleted the agent-client branch April 23, 2024 18: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.

2 participants