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(podman): use labels to specify JMX port and optionally host #1514

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jun 1, 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: #1513
Depends on #1511
Related to #1503
Related to #1394

Description of the change:

This updates the PodmanPlatformClient to discover target JMX URLs as follows:

  1. Query for containers with the io.cryostat.discovery label, then:
  2. Check for the io.cryostat.jmxUrl label. If it is present, use that. Else continue:
  3. Check for the io.cryostat.jmxPort label. If it is present, use this to construct the JMX URL.
  4. Check for the io.cryostat.jmxHost label. If it is not present, default to retrieving the container's hostname from the Podman API. Use this with the jmxPort above to construct the JMX URL.

Since there are now two main routes (jmxUrl, which replaces the previous connectUrl, or jmxHost+jmxPort), and the Podman HTTP API does not support querying for containers that have any of a list of labels applied, there is a new required label: io.cryostat.discovery. This label may have any value. This is the label that Cryostat now queries for against the Podman HTTP API to discover containers.

Motivation for the change:

As described in #1513, this allows for Podman Discovery to be more flexible and support cases where the container hostname is not known statically when the target application container is started.

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. Check that everything still appears in the various discovery API endpoints and Topology view.
  3. Change some applications' --labels in smoketest.sh or run.sh according to the description above and ensure they are still picked up after a restart.

@github-actions github-actions bot added dependent needs-triage Needs thorough attention from code reviewers labels Jun 1, 2023
@andrewazores andrewazores marked this pull request as draft June 1, 2023 20:30
@andrewazores andrewazores added feat New feature or request and removed needs-triage Needs thorough attention from code reviewers labels Jun 1, 2023
@mergify mergify bot added the safe-to-test label Jun 1, 2023
@andrewazores andrewazores force-pushed the podman-label-discovery branch from fb25cf2 to 8e835ad Compare June 2, 2023 14:02
@github-actions github-actions bot removed the dependent label Jun 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

This PR/issue depends on:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1514-e28b24b406741766ab11b5adf1a09f30a9408f95 sh smoketest.sh

@andrewazores andrewazores force-pushed the podman-label-discovery branch from e28b24b to a6da5a7 Compare June 2, 2023 15:07
@andrewazores andrewazores marked this pull request as ready for review June 2, 2023 15:07
@andrewazores andrewazores requested a review from tthvo June 2, 2023 15:07
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1514-a6da5a798378332d0ac75470f6710c87aab9255e 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 good! I just have a question about libpod uri above.

tthvo
tthvo previously approved these changes Jun 2, 2023
@andrewazores
Copy link
Member Author

Oh, weird. Not sure why my commits aren't signed. Let me try again.

@andrewazores andrewazores force-pushed the podman-label-discovery branch 2 times, most recently from 3b2dc22 to 1bd08b4 Compare June 2, 2023 21:55
@andrewazores andrewazores force-pushed the podman-label-discovery branch from 1bd08b4 to 28a1513 Compare June 2, 2023 21:55
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1514-28a15135ab304a15b6534e66bb16298610d79796 sh smoketest.sh

@andrewazores andrewazores merged commit 28c0051 into cryostatio:main Jun 2, 2023
@andrewazores andrewazores deleted the podman-label-discovery branch June 2, 2023 22:24
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.

[Request] Podman Discovery should be able to use container/pod Hostname automatically
2 participants