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

Cryostat Integration #1831

Open
ryanemerson opened this issue Jul 6, 2023 · 5 comments
Open

Cryostat Integration #1831

ryanemerson opened this issue Jul 6, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@ryanemerson
Copy link
Contributor

The Operator should provide seamless integration with the Cryostat project, so that Infinispan cluster performance can be monitored at runtime via JFR. This provides an alternative to the traditional JDK tooling such as jmap or jcmd that are not shipped with the ubi8/openjdk-17-runtime image.

User should be able to enable Cryostat integration during the initial Infinispan CR creation. It will not be possible to enable/disable cryostat on an existing Infinispan CR as this requires changes to the Infinispan startup args and therefore a cluster redeployment is necessary.

Proposed API:

spec:
  cryostat:
    enabled: true
@ryanemerson ryanemerson added the enhancement New feature or request label Jul 6, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 6, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 7, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 10, 2023
domiborges added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 14, 2023
@Crumby
Copy link
Collaborator

Crumby commented Jul 14, 2023

User should be able to enable Cryostat integration during the initial Infinispan CR creation. It will not be possible to enable/disable cryostat on an existing Infinispan CR as this requires changes to the Infinispan startup args and therefore a cluster redeployment is necessary.

@ryanemerson We already have several fields that require cluster restart, is there any other reason to prohibit this behavior with Cryostat? I can imagine that I'd like to enable the JMX once my cluster is up but I don't want to redeploy whole cluster as that is more time consuming then changing the field value. On that topic, is it even necessary to have it configurable? What are the disadvantages of having it force enabled?

@ryanemerson
Copy link
Contributor Author

We already have several fields that require cluster restart, is there any other reason to prohibit this behavior with Cryostat? I can imagine that I'd like to enable the JMX once my cluster is up but I don't want to redeploy whole cluster as that is more time consuming then changing the field value.

That's true, but I don't think this is a good design as the user is not presented with any warning that data will be dropped on CR change. When a lot of these features were added to the Infinispan CR we didn't have a webhook support, therefore it was not possible to reject CR updates and effectively make fields immutable. Now that we have such capabilities, I think it makes sense to leverage them where appropriate.

Immutable spec fields are present in many of the native Kubernetes resources, StatefulSets etc, so I don't think this is deviating from best practices.

On that topic, is it even necessary to have it configurable? What are the disadvantages of having it force enabled?

Do you mean having Cryostat always enabled? The disadvantages are two-fold:

  1. JMX would always be enabled and exposed on the admin service
  2. A Cryostat CR creates a new pod to host the deployment, so additional resource usage

ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 17, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 17, 2023
domiborges added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 17, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 17, 2023
@Crumby
Copy link
Collaborator

Crumby commented Jul 17, 2023

Immutable spec fields are present in many of the native Kubernetes resources, StatefulSets etc, so I don't think this is deviating from best practices.

Fair enough

JMX would always be enabled and exposed on the admin service

Why is having a JMX always enabled and exposed a bad thing? The endpoint should be secured anyway?

A Cryostat CR creates a new pod to host the deployment, so additional resource usage

Is the design counting with Operator creating new Cryostat instance for each cluster? If that's the case I don't think it's a good idea. It would be better if the user would make the final link himself in his own managed Cryostat instance from multiple reasons:

  • Cryostat is able to handle multiple endpoints, meaning that instance per cluster takes more resources
  • We are taking over responsibility for configuring Cryostat, which will lead to additional fields we'll have to manage and pass to Cryostat CR for users (memory, cpu and others that may show as important in the future)
  • We'll need to test integration with various Cryostat Operator versions for Cryostat CRs compatibility

Generally I'd imagine the solution would just enable JMX so Cryostat can connect to the cluster but it would be up to the user to make the final link. The integration surface will be smaller while allowing for more configuration options by user.

@ryanemerson
Copy link
Contributor Author

ryanemerson commented Jul 17, 2023

Why is having a JMX always enabled and exposed a bad thing? The endpoint should be secured anyway?

It's true that the endpoint is always secured, however by always exposing this we're increasing the attack service. @tristantarrant Any thoughts?

  • Cryostat is able to handle multiple endpoints, meaning that instance per cluster takes more resources

That's true, however JFR recordings are mainly a tool for profiling/debugging performance so I don't envisage this being enabled for the majority of clusters and certainly not in production (unless an issue can't be reproduced in the dev environment).

  • We are taking over responsibility for configuring Cryostat, which will lead to additional fields we'll have to manage and pass to Cryostat CR for users (memory, cpu and others that may show as important in the future)

I'm torn on this one. Initially I thought the same, however the process of configuring credentials in the Cryostat instance is not very user friendly. Typical workflow that's currently automated by my PR:

  1. Create Cryostat CR in cluster namespace
  2. Obtain operator-credentials from generated service
  3. Manually enter credentials via UI

So here the tradeoff is whether we want users to have to become familiar with Cryostat CRs etc and assume less responsibility for the integration, or do most of the "plumbing" for the user and allow them to focus on JFR via the UI.

  • We'll need to test integration with various Cryostat Operator versions for Cryostat CRs compatibility

Given that the CRD should remain backwards compatible, I don't think this is too much of an issue. Calls to the Cryostat REST API would need to be checked, but this should be pretty stable given that it's a public API and the release cadence of Cryostat is relatively slow.

You make a valid point about configuring additional fields, however the defaults seem reasonable for CPU etc. The way I have implemented the Cryostat CR, it's possible for the user to update these fields at runtime on the CR instance and they won't be overridden.

ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 21, 2023
ryanemerson added a commit to ryanemerson/infinispan-operator that referenced this issue Jul 21, 2023
@ryanemerson
Copy link
Contributor Author

After further discussion, we have determined that the best way forward is to split this into three issues:

  1. Allow JMX to be exposed via the Infinispan CR Allow JMX endpoint to be exposed #1835
  2. Document how to integration Cryostat with 1 Document how to utilise Cryostat CR #1836
  3. Provision Cryostat CR and automatically store operator credentials.

I'll leave this issue open to tackle 3.

domiborges added a commit to domiborges/infinispan-operator that referenced this issue Sep 4, 2023
domiborges added a commit to domiborges/infinispan-operator that referenced this issue Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants