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): port kubeApi discovery to v3 #325

Merged
merged 30 commits into from
Apr 24, 2024

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Mar 11, 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


Fixes: #238

Description of the change:

Port KubeDiscoveryAPI from v2 to v3.

Motivation for the change:

See #238

How to manually test:

Build the application and push to quay.io:

IMAGE_NAMESPACE=quay.io/thvo
./mvnw clean package -DskipTests=true && \
podman tag quay.io/cryostat/cryostat:3.0.0-snapshot ${IMAGE_NAMESPACE}/cryostat:3.0.0-snapshot && \
podman push ${IMAGE_NAMESPACE}/cryostat:3.0.0-snapshot

Install the application with Helm Chart (cryostatio/cryostat-helm#128):

helm install cryostat3 ./charts/cryostat/ \
  --set core.discovery.kubernetes.enabled=true \
  --set core.discovery.kubernetes.namespaces="{default,myns,.}" \
  --set core.discovery.kubernetes.portNames="{jmx-jfr,my-port}" \
  --set core.discovery.kubernetes.portNumbers="{9091,9097}" \
  --set core.image.repository=${IMAGE_NAMESPACE}/cryostat

Deploy/Scale/Teardown sample applications:

cd cryostat-operator && make sample_app # or make undeploy_sample_app

@tthvo tthvo added feat New feature or request safe-to-test labels Mar 11, 2024
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Mar 11, 2024
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Mar 11, 2024
@andrewazores
Copy link
Member

In 2.4, the NAMESPACES env var was expected to be simply a comma-separated list of namespaces that Cryostat should try to set up an informer for. If the env var was not set then it would default to an empty list - no discovery.

I think in 3.0 we want to preserve that same behaviour, however with one addition: a special character that signifies "your own namespace", as in the namespace that the Cryostat Pod is running within. I suggest . for this special character. This can be included in the namespace list, so it would be possible for the user to set the variable to a,b,.. Cryostat should try to look up what its own namespace is, map . to that value, then deduplicate the namespace list (in case . is equal to a or b). This way configuration is a little bit simpler. I think we can also default the namespace list to the . singleton rather than defaulting to empty.

@tthvo
Copy link
Member Author

tthvo commented Apr 4, 2024

In 2.4, the NAMESPACES env var was expected to be simply a comma-separated list of namespaces that Cryostat should try to set up an informer for. If the env var was not set then it would default to an empty list - no discovery.

I think in 3.0 we want to preserve that same behaviour, however with one addition: a special character that signifies "your own namespace", as in the namespace that the Cryostat Pod is running within. I suggest . for this special character. This can be included in the namespace list, so it would be possible for the user to set the variable to a,b,.. Cryostat should try to look up what its own namespace is, map . to that value, then deduplicate the namespace list (in case . is equal to a or b). This way configuration is a little bit simpler. I think we can also default the namespace list to the . singleton rather than defaulting to empty.

Oh thank you! This behaviour with . makes sense to me. I suppose we would want the default namespace list to empty and require it to be explicitly set in a similar way to other configurations (e.g. cryostat.discovery.k8s.port-names)?

@andrewazores
Copy link
Member

Sure, I think for the individual Cryostat container level it makes sense to default to empty lists there. We can apply default values in the Helm chart, in Operator CRs, and maybe also in Operator default webhooks.

@tthvo tthvo force-pushed the k8s-discovery branch 5 times, most recently from 19daf77 to bd4ad8e Compare April 15, 2024 08:07
@tthvo tthvo force-pushed the k8s-discovery branch 4 times, most recently from 4d97568 to e3686da Compare April 22, 2024 10:01
@tthvo tthvo marked this pull request as ready for review April 22, 2024 11:08
@tthvo tthvo requested a review from a team as a code owner April 22, 2024 11:08
@tthvo tthvo requested a review from a team April 22, 2024 11:08
@tthvo
Copy link
Member Author

tthvo commented Apr 24, 2024

/build_test

Copy link

Workflow started at 4/24/2024, 2:05:13 PM. View Actions Run.

Copy link

No OpenAPI schema changes detected.

Copy link

No GraphQL schema changes detected.

Copy link

CI build and push: At least one test failed ❌ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8821181398

@tthvo
Copy link
Member Author

tthvo commented Apr 24, 2024

/build_test

Copy link

Workflow started at 4/24/2024, 2:25:51 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/8821461039

@tthvo
Copy link
Member Author

tthvo commented Apr 24, 2024

Phew tests failure and spotbugs errors are fixed now^^ I did manual tests with container, jdp and custom discovery as this PR also affects them. They are all fine.

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.

[Story] KubernetesApi Discovery Mechanism
2 participants