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(discovery): Podman platform #1394

Merged
merged 22 commits into from
Mar 14, 2023
Merged

Conversation

andrewazores
Copy link
Member

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

Related to cryostatio/cryostat#492
Related to cryostatio/cryostat#421

Description of the change:

I revisited an old, old branch (June 2021 - unfortunately the original commit dates are lost now that this has been rebased and signed) where I was experimenting with talking to the Podman HTTP API from within Cryostat. At the time the Discovery tree querying work was under way, but there was not yet any notion of a persistent database for this data, and no developed system for discovery plugins other than the runtime detection of the single type platform strategy to pick up and use. At that time the original goal of getting a Podman HTTP API discovery query working was just to enable that discovery tree structure development for a contributor who was unable to run crc or other heftier systems that can provide a more interesting and non-flat deployment structure as compared to JDP's flat list.
Today we have much more sophistication around discovery, persistence, and modularity, and so I got the Podman HTTP API communication working and wrapped it up into a built-in discovery plugin.

Motivation for the change:

This may be helpful because it is an additional example of a discovery plugin implementation, but it also removes dependency on JDP which may relieve some work from upstream JMC-core refactoring. JDP is probably only used in toy deployments of Cryostat like the upstream development smoketest.sh, so dropping it is unlikely to cause anyone any real issues so long as we have a reasonable development cycle replacement. Querying the Podman API periodically seems to do just as well for this purpose, and as an added bonus also is a little closer to other container runtimes in that we could configure multiple pods along with multiple containers within the pods and emulate something more like an OpenShift deployment (more complex topology).

In this PR the JDP platform client is still present, still named Default, and still has the lowest priority of the built-in plugins. The system will still choose the highest priority available built-in client to run, alongside Custom Targets and the plugin API, so in smoketest.sh/run.sh the Podman setup will be active rather than JDP. In integration tests the Podman socket is not active and mounted into the container, so the system continues to fall back on the old JDP discovery built-in.

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. List targets, view topology if you have that PR checked out, etc.

@andrewazores andrewazores added the feat New feature or request label Mar 9, 2023
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Mar 9, 2023
@mergify mergify bot added the safe-to-test label Mar 9, 2023
@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Mar 9, 2023
@andrewazores
Copy link
Member Author

Screenshot_2023-03-09_18-57-25

@andrewazores
Copy link
Member Author

andrewazores commented Mar 10, 2023

This + cryostatio/cryostat-web#891 =

image

Of course, the two quarkus-test applications are also within the pod, but they are self-publishing as discovery plugins rather than being configured for discovery by podman container label.

@andrewazores andrewazores requested review from maxcao13 and tthvo March 10, 2023 00:30
@andrewazores andrewazores marked this pull request as ready for review March 10, 2023 00:30
@andrewazores andrewazores requested a review from ebaron as a code owner March 10, 2023 00:30
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-0769036a15b56f049b987b01c835e351798c3809 sh smoketest.sh

@@ -82,7 +82,10 @@ public class HttpServer extends AbstractVerticle {
.setPort(netConf.getInternalWebServerPort())
.addWebSocketSubProtocol("*")
.setCompressionSupported(true)
.setLogActivity(true)));
.setLogActivity(true)
.setTcpFastOpen(true)
Copy link
Member

@maxcao13 maxcao13 Mar 10, 2023

Choose a reason for hiding this comment

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

Just curious what is reason for enabling these tcp features now vs. before?

@maxcao13
Copy link
Member

image

Running the smoketest with the podman socket open results in both the Podman Realm and the JDP realm visible. Does this make sense?

@maxcao13
Copy link
Member

maxcao13 commented Mar 10, 2023

Hmm... weird but it seems like jvmIds are different for two equivalent JVMs in the two realms.
image
image

@andrewazores
Copy link
Member Author

image

Running the smoketest with the podman socket open results in both the Podman Realm and the JDP realm visible. Does this make sense?

Ah right, I forgot to mention. Since the built-in discovery only loads one of these plugins at runtime (plus custom targets, always), it's the Podman realm that is up to date. The JDP realm you're seeing is old data in the database from the last time you ran it and JDP was loaded.

I'm not sure what I want to do about that. Maybe the builtins that are not activated should have their realms deleted automatically?

@maxcao13
Copy link
Member

maxcao13 commented Mar 10, 2023

Oh, that makes a lot of sense then. Then the jvmID thing is not an issue.

Realms being deleted automatically if not loaded makes sense. We probably will never activate/deactive any built-in discovery platforms over the lifecycle after startup is complete so any of that data that isn't loaded will always be stale in a future run.

@andrewazores
Copy link
Member Author

andrewazores commented Mar 10, 2023

I'm not sure what I want to do about that. Maybe the builtins that are not activated should have their realms deleted automatically?

External plugins that go through the API have the ping-pruning mechanism that will result in their realms being deleted if the plugins becomes unreachable too, so they do potentially suffer the same "problem". And just in general, if a target disappears, that context is already lost for any archived recordings that came from it, which is why we have separate archived recording metadata that we store. So deleting the builtin plugins realms' doesn't seem too much different.

On a technical level it will take some refactoring, but I don't think it should be too hard to do.

@andrewazores
Copy link
Member Author

Okay, last set of commits:

  1. Allows multiple platform strategies to be selected. The highest priority available strategy continues to provide the sole AuthManager implementation. All selected strategies provide PlatformClients, which are what the internal implementation calls a built-in discovery plugin still. Custom Targets is special cased and always enabled. JDP happens to have its availability method hardcoded to returning true. The Podman client checks for the existence of the Podman API Unix socket. These may all be available and publishing to the discovery database at once, along with any external discovery plugins (like -agents). The previous variables for selecting a specific auth manager, a specific platform strategy, and for disabling built-ins (except custom targets) are all still there and work the same way.
  2. Deregisters built-in plugins if they are unavailable on startup. For example, CRYOSTAT_PLATFORM=io.cryostat.platform.internal.PodmanPlatformStrategy sh smoketest.sh will cause the internal JDP built-in plugin to be deregistered and its discovery realm deleted from the tree. A subsequent rerun with no CRYOSTAT_PLATFORM setting will allow JDP to be selected alongside Podman again and it will re-register again as a new built-in discovery plugin.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-f22338525b586e3dbd25d88bab066bd56ee76e07 sh smoketest.sh

@andrewazores andrewazores force-pushed the podman-api branch 2 times, most recently from 2d3a028 to 5dc644a Compare March 13, 2023 15:30
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-5dc644aa0c47fa00f04b80b53c576093d7d93e5b sh smoketest.sh

@andrewazores
Copy link
Member Author

Maybe CRYOSTAT_PLATFORM should support a comma-separated list instead of just a single platform now, too.

@maxcao13
Copy link
Member

maxcao13 commented Mar 13, 2023

Running smoketest just like this: sh smoketest.sh runs both the podman client discovery platform and the JDP discovery platform.

But running it with the env var CRYOSTAT_PLATFORM=io.cryostat.platform.internal.PodmanPlatformStrategy sh smoketest.sh removes the stale JDP built-in targets from the database and runs the podman discovery correctly on its own.

@andrewazores
Copy link
Member Author

Running smoketest just like this: sh smoketest.sh runs both the podman client discovery platform and the JDP discovery platform.

But running it with the env var CRYOSTAT_PLATFORM=io.cryostat.platform.internal.PodmanPlatformStrategy sh smoketest.sh removes the stale JDP built-in targets from the database and runs the podman discovery correctly on its own.

Yep, that sounds like it's working as expected.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-e365bf4510bf54617aea1a722e6208b3ad75aa47 sh smoketest.sh

maxcao13
maxcao13 previously approved these changes Mar 13, 2023
Copy link
Member

@maxcao13 maxcao13 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!

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-dfae9a139d156928527eb75c6633f6490319bfef sh smoketest.sh

ebaron
ebaron previously approved these changes Mar 14, 2023
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Dependency addition looks fine

Signed-off-by: Andrew Azores <aazores@redhat.com>
Signed-off-by: Andrew Azores <aazores@redhat.com>
Signed-off-by: Andrew Azores <aazores@redhat.com>
allow multiple available strategies to be selected. the highest priority available platform provides the authmanager if none explicitly specified. set of selected strategies can be mapped to set of available platform clients, allowing multiple enabled built-in plugins at once

Signed-off-by: Andrew Azores <aazores@redhat.com>
Signed-off-by: Andrew Azores <aazores@redhat.com>
Signed-off-by: Andrew Azores <aazores@redhat.com>
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1394-ffca3e1107cbd7a8333262cd22d7d063720d0006 sh smoketest.sh

@andrewazores andrewazores merged commit d675fef into cryostatio:main Mar 14, 2023
@andrewazores andrewazores deleted the podman-api branch March 14, 2023 19:38
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.

3 participants