Review CRDs for next release

[pebrc]

Let's review our current CRDs and remove any redundant attributes that are there for historic reasons only.

One candidate that comes to mind is the featureFlags attribute.

• Go through the current CRDs and identify attributes to remove
• discuss where unsure
• remove

[charith-elastic]

See https://gist.github.com/charith-elastic/21840956cb553ff31942b2166eed4cce for reference documentation generated from the CRDs.

Quick links:

• Elasticsearch
• Kibana
• APM

[charith-elastic]

Proposed Changes

• All specs
  • Should version be renamed to stackVersion?.
  • FeatureFlags is not referenced by any operator code.
  • Should nodeCount be renamed to podCount or podTemplate to nodeTemplate for consistency? My feeling is that it is better to use the appropriate Kubernetes terms such as "pod" to reduce the cognitive load.
• Elasticsearch Spec
  • Should rename name to nodeName in NodeSpec to be consistent with nodeCount (or rename nodeCount to count)
  • Should UpdateStrategy move under NodeSpec? (less flexibility but simpler to reason about)
• Kibana Spec
  • Should elasticsearch and elasticsearchRef be a union so that a connection to Elasticsearch should be defined as either elasticsearch.ref or elasticsearch.backend? (Some users already find this confusing)
• APM Spec
  • Again, should elasticsearch and elasticsearchRef be a union?

[david-kow]

Nice findings, some feedback:

FeatureFlags is not referenced by any operator code.

Is this because we are prepared to use it if there is a need?

Should rename name to nodeName in NodeSpec to be consistent with nodeCount (or rename nodeCount to count)

I think the naming is pretty accurate - name is the name of the node set, nodeCount is the number of nodes in the set. I think going with nodeName would be inaccurate, and going with count could be confusing (count of what? sets? nodes? volume claim templates?). Btw, #1488 was filled to discuss whether name 'nodes' is not confusing on its own - I think having nodeSet.count, nodeSet.name would be more clear.

[sebgl]

Should version be renamed to stackVersion

I think I like version? We are in the Elasticsearch (or other stack product) CRD, so it seems implicit that version is the Elasticsearch version?

FeatureFlags is not referenced by any operator code.

+1 to remove it.

Should nodeCount be renamed to podCount or podTemplate to nodeTemplate for consistency? My feeling is that it is better to use the appropriate Kubernetes terms such as "pod" to reduce the cognitive load.

That's a tough question we debated many times 😄 We always have mixed feelings around using Elasticsearch vocabulary vs. Kubernetes vocabulary. Since we are in the context of the Elasticsearch CRD, I tend to think we are in the Elasticsearch domain, so we should tend to use Elasticsearch wording. However things like podTemplate clearly specify a Kubernetes thing. nodeTemplate would be weird to use here. The way I see our current CRD is: "these are my Elasticsearch nodes (nodes:), here I want 3 of those nodes (nodeCount:), and I want them to be deployed in this particular Kubernetes way (podTemplate:). I think I would probably like count instead of nodeCount, but I don't have strong feelings. Naming is hard! We probably need other people thoughts. A related close issue for the last time we talked about renaming nodeCount.

Should rename name to nodeName in NodeSpec to be consistent with nodeCount (or rename nodeCount to count)

-1 on that one, this would be even more confusing, since that name isn't used as the Elasticsearch node name, but as a basis for this name. The ES node name ends up being the pod name, eg. <cluster-name>-<nodeSpec-name>-<ordinal>. But as you point out, the current situation is already confusing probably, and my comment is also true for the current CRD. Maybe #1488 would help.

Should UpdateStrategy move under NodeSpec? (less flexibility but simpler to reason about)

I don't think so, it's a setting that applies across all nodeSpecs.

Should elasticsearch and elasticsearchRef be a union

Can we rely on something landing beta in Kubernetes 1.15? But we can probably implement the same mechanism ourself. I don't have strong feelings. ref looks like a strange field name?

[charith-elastic]

Isn't UpdateStrategy related to how node groups are defined in NodeSpec? Providing a list of label selectors to the UpdateStrategy provides maximum flexibility but I suspect that in most cases it could just as well be the list of node group names as well. At the cost of being a little bit more verbose, specifying the update strategy at the point where the node groups is defined seems easier to reason about.

For specifying the connection to Elasticsearch, it doesn't have to be a formal union type but something similar to how different types of volumes are defined in the pod spec might be more idiomatic. Maybe introducing a field named backend that would accept either elasticsearch or elasticsearchRef as values?

backend: 
  elasticsearch:
     url: https://es.local

or

backend:
  elasticsearchRef:
     name: "es"

[pebrc]

Should elasticsearch and elasticsearchRef be a union
...
Maybe introducing a field named backend that would accept either elasticsearch or elasticsearchRef as values?

I think our previous discussions assumed the following invariants:

• if a user wants to hand-craft a backend configuration they should use the config attribute and just use standard config parameters as documented in the Kibana docs
• if a users wants to use the operators convenience association mechanism it should use elasticsearchRef

That leaves us with the backend thingy. Can we just remove the BackendElasticsearch attribute? It looks like a left-over from the association refactoring:

  elasticsearch: 
        certificateAuthorities: 
          secretName: clusterInternalCaCertSecretName # could go into kibana.yml via config
        url: https://main-es-http:9200/ # same kibana.yml via config attribute
        auth: 
          secret: 
               name: 'main-es-elastic-user',  # do we need to make this configurable or can this just be a convention/label based discovery?
               key: 'elastic'  

[charith-elastic]

Can we just remove the BackendElasticsearch attribute?

Presumably the BackendElasticsearch object still exists because auth and certificateAuthorities fields are Kubernetes-specific values that cannot be represented as standard Kibana configuration.

[pebrc]

Presumably the BackendElasticsearch object still exists because auth and certificateAuthorities fields are Kubernetes-specific values that cannot be represented as standard Kibana configuration.

Yes, the question is whether we want to move to implicit discovery (via naming convention or labels) for the CA secret and the user secret to be able to get rid of this section.

[charith-elastic]

To summarize the conversation so far:

• All specs
  • Should version be renamed to stackVersion?. 👎
  • FeatureFlags is not referenced by any operator code. 👍
  • Should nodeCount be renamed to podCount or podTemplate to nodeTemplate for consistency? 🤷‍♂️
• Elasticsearch Spec
  • Should rename name to nodeName in NodeSpec to be consistent with nodeCount (or rename nodeCount to count) 👎
  • Should UpdateStrategy move under NodeSpec? 👎
• Kibana and APM Specs
  • Should elasticsearch and elasticsearchRef be a union? 🔜 replace BackendElasticsearch

I will leave this issue open for a couple of more days and if there are no further comments, raise issues for the following actions:

• Remove FeatureFlags from CRDs
• Discuss possible ways of removing BackendElasticsearch

[pebrc]

See #1488 for the discussion around nodes and nodeCount

[charith-elastic]

Summary of progress so far:

• Remove Elasticsearch Remove Elasticsearch field from CRDs #1687
• Remove FeatureFlags Remove FeatureFlags from CRDs #1658
• Remove GroupingDefinition Remove Grouping definition #1694
• Rename nodes to nodeSets Rename nodes to nodeSets, nodeCount to count #1843
• Default resource limits Set Elasticsearch Pod default memory limit to 2Gi  #1810, Set APM Server default memory requests and limits to 512MiB #1815, Set Kibana default memory requests and limits to 1Gi  #1818
• Remove setVMMaxMapCount: Remove setVmMaxMapCount from Elasticsearch CRD #1839

[charith-elastic]

I think we have covered all the major changes we wanted to go into v1.0.0-beta1 so this issue can be closed. Please re-open if I have missed something.