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

Cache CRD #224

Closed
rigazilla opened this issue Nov 26, 2019 · 10 comments
Closed

Cache CRD #224

rigazilla opened this issue Nov 26, 2019 · 10 comments
Assignees
Milestone

Comments

@rigazilla
Copy link
Collaborator

Implement a new Custom Resource Definition Cache that looks like this:

apiVersion: infinispan.org/v2alpha1
    kind: Cache
metadata:
    name: cache-name
spec:
    clustername: mycluster
    cacheTemplate: my-template
    cacheXML: |
  <distributed-cache name="mycache">
     <encoding>
        <key media-type="application/x-protostream"/>
        <value media-type="application/x-protostream"/>
     </encoding>
  </distributed-cache>
status:
    connectionInfo: info needed by the user for connecting. Structure is TBD

With the following behaviour:
if an Infinispan cluster named mycluster is not present the operator must create it with a configuration that fits the cache needs.

The operator must create the cache mycache and return in status.connectionInfo the correct info on how to use it.

@Crumby
Copy link
Collaborator

Crumby commented Nov 27, 2019

@rigazilla What is the relation between cacheTemplate and cacheXML? Are they mutually exclusive or will the cacheXML creates also cacheTemplate for later reference?

If an Infinispan cluster named mycluster is not present the operator must create it with a configuration that fits the cache needs.

Do you have some examples that would require to instantiate differently configured Infinispan clusters? It would be good to have some reasonable default specified as part of the issue too.

Btw. I like the way you solved getting connection info to the user.

@rigazilla
Copy link
Collaborator Author

@rigazilla What is the relation between cacheTemplate and cacheXML? Are they mutually exclusive or will the cacheXML creates also cacheTemplate for later reference?

I was thinking to force them to be exclusive or one to override the other, but your option is good too if the template doesn't exist on the cluster (or the cluster is new)

If an Infinispan cluster named mycluster is not present the operator must create it with a configuration that fits the cache needs.

Do you have some examples that would require to instantiate differently configured Infinispan clusters? It would be good to have some reasonable default specified as part of the issue too.

I'm referring to proper cache options like the 'number of owners' or 'maximum size'...
I would like to see security aspect applied later by composition: like TLS terminated on the service for encryption... no idea for authentication atm.

Btw. I like the way you solved getting connection info to the user.

thank you :)

@rigazilla
Copy link
Collaborator Author

Hi @slaskawi, this is the issue for the cache CRD, please share here anything

@slaskawi
Copy link

slaskawi commented Dec 6, 2019

I think there might be a better way than submitting XML.

If I'm not mistaken, we can define a cache as JSON representation. In our code this will result in CRD type object and Go code type interface. Since every YAML can be marshalled to JSON, we can use this property and submit a JSON representation to Infinispan REST interface.

I think it's a clever way of pushing the responsibility for JSON representation to a user (and Infinispan Servier).

Let me experiment a little bit with it.

@galderz
Copy link
Member

galderz commented Dec 10, 2019

I agree with @slaskawi, it'd be better to have something other than XML. However, a few weeks back I was trying out sending JSON as configuration, which @gustavonalle had pointed me to, but we need to improve that experience because it didn't feel easy to figure out what's the right JSON for a given XML. I was in a bit of a rush so I went with XML for my examples (HTTP REST calls), which are just as fine.

@galderz
Copy link
Member

galderz commented Dec 10, 2019

I agree with @Crumby, it'd be good for template and xml to work together. So if XML is not present, create a cache out of an existing template, otherwise create the cache with the XML and create a cache too.

One more thing, as also hinted by @Crumby, there probably needs to be a link between our current Infinispan kind and this new Cache kind. I expect that'd be the cluster name which would match the name of an Infinispan?

@rigazilla
Copy link
Collaborator Author

One more thing, as also hinted by @Crumby, there probably needs to be a link between our current Infinispan kind and this new Cache kind. I expect that'd be the cluster name which would match the name of an Infinispan?

How about using labels: we can use selectors with them and we don't need to define any new ad-hoc field.

@slaskawi
Copy link

slaskawi commented Jan 5, 2020

However, a few weeks back I was trying out sending JSON as configuration, which @gustavonalle had pointed me to, but we need to improve that experience because it didn't feel easy to figure out what's the right JSON for a given XML

@galderz I think, this is more a documentation issue, right? A user should know how the JSON Cache configuration representation should look like.

I agree with @Crumby, it'd be good for template and xml to work together. So if XML is not present, create a cache out of an existing template, otherwise create the cache with the XML and create a cache too.

@galderz I think it's possible. But do you really want to support 2 exchangeable formats out of the box? To me, this increases maintenance burden and gives nothing back.

How about using labels: we can use selectors with them and we don't need to define any new ad-hoc field.

@rigazilla Yes, in general I prefer labels approach, similar to what we did for Keycloak: https://github.com/keycloak/keycloak-operator/blob/master/deploy/crds/keycloak.org_keycloakrealms_crd.yaml#L34

@rigazilla
Copy link
Collaborator Author

I agree with @Crumby, it'd be good for template and xml to work together. So if XML is not present, create a cache out of an existing template, otherwise create the cache with the XML and create a cache too.

@galderz I think it's possible. But do you really want to support 2 exchangeable formats out of the box? To me, this increases maintenance burden and gives nothing back.

If we could implement the policy above with JSON, I think it's ok to have just one format as first step.

@galderz
Copy link
Member

galderz commented Jan 10, 2020

@galderz I think, this is more a documentation issue, right? A user should know how the JSON Cache configuration representation should look like.

Not only documentation IMO. Sure, documentation should explain what a JSON Cache configuration looks like. But aside from that, many users might have XML configuration today and they'd want the JSON version for that, e.g. create cache calls via curl are much less verbose with JSON vs XML. There should be a way to provide XML and spit out JSON that works.

Taking this further, I'd skip JSON and allow YAML configuration to be included directly? Since the CRD is yaml, you can take a subset of it and be it the YAML configuration? YAML is even less verbose.

@galderz I think it's possible. But do you really want to support 2 exchangeable formats out of the box? To me, this increases maintenance burden and gives nothing back.

Good point, if I had to choose one I'd go for the option of just embedding the YAML (instead of XML) of the configuration for the cache. The template option could come later if really needed. In fact, for templates to work, the user would have had to add them in advance to an existing cluster, so maybe that could an independent CR, e.g. CacheTemplate CR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants