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

Change from free form options to a typed model #72

Closed
objectiser opened this issue Oct 18, 2018 · 3 comments
Closed

Change from free form options to a typed model #72

objectiser opened this issue Oct 18, 2018 · 3 comments

Comments

@objectiser
Copy link
Contributor

As discussed in #71 (comment), the current CRD uses a free format options field to allow the user to pass parameters through to the executables.

Although this means that the user could take advantage of new options without requiring a new release of the operator, it also means that incorrect options can be specified and passed to the executables as parameters - resulting in the executable throwing an error.

Having a properly typed model in the CRD means that:

  • we can ensure the user has not inadvertently specified an option that will cause the executable to fail to start
  • options that only support specific values can be validated and an informative message presented to the user
  • the structure of the options can be organised in a more meaningful way - e.g. cassandra create schema info contained under other information related to cassandra.
@jpkrohling
Copy link
Contributor

This is the current set of options for 1.7:
https://gist.github.com/jpkrohling/a7d24b1c1e3f76eb004915464785f531

One thing to think about as well is regarding the organization of such options. Should they be split by component, like, "CollectorOptions" and "QueryOptions"? This would make it possible to reuse these objects for the JaegerQuerySpec, JaegerAllInOneSpec, JaegerCollectorSpec and JaegerAgentSpec like the current JaegerStorageSpec is used and would require a schema like the following:

apiVersion: io.jaegertracing/v1alpha1
kind: Jaeger
metadata:
  name: typed-options
spec:
  strategy: production
  collector:
    queue-size: 100
  agent:
    discovery:
      min-peers: 10
  storage:
    type: cassandra
    cassandra:
      servers: cassandra
      keyspace: jaeger_v1_datacenter3
      schema:
        datacenter: "datacenter3"
        mode: "test"

Then there's a question on what to do with options that are used for different components, in different ways, like the health check port (health-check-http-port). This can be specified at the all-in-one binary, collector and query, but doesn't really belong to any of them. Should this be at the "top level", like the collector?

apiVersion: io.jaegertracing/v1alpha1
kind: Jaeger
metadata:
  name: typed-options
spec:
  strategy: production
  collector:
    queue-size: 100
  health-check-port: 14269 # applies this value to all components if strategy is production

@objectiser
Copy link
Contributor Author

objectiser commented Oct 18, 2018

Yes I think we should use this opportunity to have options in specific categories related to their function.

I think we should deal with other cases, that are not clear cut, on a case by case basis.

We should also carefully consider whether all options should be exposed, or whether it is something that the operator could take responsibility for. I don't have anything specific in mind - but the health check could be a good example. We should discuss whether it really needs to be exposed to the user.

@jpkrohling jpkrohling added needs-triage New issues, in need of classification and removed needs-triage New issues, in need of classification labels Dec 16, 2019
@pavolloffay
Copy link
Member

Closing, there are no plans to change the options to a strongly typed variant. The validation can be done in the validating webhook.

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

3 participants