Add support for Cassandra create-schema job #71

jpkrohling · 2018-10-17T17:17:24Z

This PR adds support for the Cassandra create-schema job, backed by the create-schema script.

With this PR, the following template can be used:

apiVersion: io.jaegertracing/v1alpha1
kind: Jaeger
metadata:
  name: with-cassandra
spec:
  strategy: all-in-one
  storage:
    type: cassandra
    options:
      cassandra:
        servers: cassandra
        keyspace: jaeger_v1_datacenter3
    cassandra-create-schema:
      datacenter: "datacenter3"
      mode: "test"

I'm not 100% happy with the "datanceter" option duplicating part of the "keyspace" property from the options object, but that's how they are also today. Perhaps we can improve the create-schema script to not require a datacenter/mode, accepting only a "keyspace".

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

jpkrohling · 2018-10-17T17:17:30Z

This change is

jpkrohling · 2018-10-17T17:17:50Z

Missing:

New e2e test
Documentation

jpkrohling · 2018-10-17T17:18:13Z

@objectiser , @pavolloffay would one of you like to review this?

codecov · 2018-10-17T17:20:52Z

Codecov Report

Merging #71 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage    99.3%   99.37%   +0.07%     
==========================================
  Files          17       19       +2     
  Lines         718      801      +83     
==========================================
+ Hits          713      796      +83     
  Misses          5        5

Impacted Files	Coverage Δ
pkg/controller/controller.go	`100% <ø> (ø)`	⬆️
pkg/apis/io/v1alpha1/options.go	`100% <100%> (ø)`	⬆️
pkg/storage/dependency.go	`100% <100%> (ø)`
pkg/controller/production.go	`100% <100%> (ø)`	⬆️
pkg/controller/all-in-one.go	`100% <100%> (ø)`	⬆️
pkg/storage/cassandra_dependencies.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f520023...090da1a. Read the comment docs.

jaegerci-bot · 2018-10-18T07:58:19Z

@jpkrohling As a general comment, I think this has thrown up an issue for me with the use of options in the CR as a way to pass through a free format set of parameters.

Ideally I think the schema creation config should be a part of the overall cassandra config, e.g.

apiVersion: io.jaegertracing/v1alpha1
kind: Jaeger
metadata:
  name: with-cassandra
spec:
  strategy: all-in-one
  storage:
    type: cassandra
    cassandra:
      servers: cassandra
      keyspace: jaeger_v1_datacenter3
      schema:
        create: true
        datacenter: "datacenter3"
        mode: "test"

and I assume having the cassandra config statically typed would identify errors in the CR, rather than waiting for the Jaeger instance to be deployed and throw errors due to the passed parameters being incorrect.

pavolloffay

Where are settings for TTL, replication and keyspace?

pavolloffay · 2018-10-18T08:16:53Z

pkg/apis/io/v1alpha1/zz_generated.deepcopy.go

+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JaegerCassandraCreateSchemaSpec.
+func (in *JaegerCassandraCreateSchemaSpec) DeepCopy() *JaegerCassandraCreateSchemaSpec {
+	if in == nil {


in CopyInto this check is missing. Is it a convenience? They both could define the same behavior and preconditions.

This is generated code

jpkrohling · 2018-10-18T08:23:13Z

Where are settings for TTL, replication and keyspace?

They are missing on this first PR, but I'll probably add them before removing the WIP flag.

jpkrohling · 2018-10-18T09:35:03Z

@pavolloffay, @objectiser I just added a new commit with tests. I believe this is ready to be reviewed.

objectiser · 2018-10-18T10:54:56Z

README.adoc

+
+=== Cassandra
+
+When the storage type is set to Cassandra, the operator will automatically create a batch job that creates the required schema for Jaeger to run. This batch job will block the Jaeger installation, so that it starts only after the schema is successfuly created. The creation of this batch job can be disabled by setting the `enabled` property to `false`:


Just wondering if it would be safer to default to false? If someone is using an existing jaeger cassandra storage - what is the impact of the job running, is it a noop?

The current scripts are noop, as they have an "if not exists" clause:

https://github.com/jaegertracing/jaeger/blob/7d9d212b6d947742055fb4e38180152da687acf5/plugin/storage/cassandra/schema/v001.cql.tmpl#L25

But it might be a good idea to add a flag to the binary, to get this enabled/disabled by default, as we discussed today in the meeting. Something for a follow up PR, to be discussed with the ES parts?

objectiser · 2018-10-18T10:57:02Z

test/e2e/wait_util.go

@@ -87,3 +87,29 @@ func WaitForIngress(t *testing.T, kubeclient kubernetes.Interface, namespace, na
 	t.Logf("Ingress available\n")
 	return nil
 }
+
+// WaitForJob checks to see if a given job has the completed successfuly


nit: remove 'the'

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pavolloffay reviewed Oct 18, 2018

View reviewed changes

objectiser mentioned this pull request Oct 18, 2018

Change from free form options to a typed model #72

Closed

jpkrohling force-pushed the Cassandra-Batch-Job branch from 65fcb75 to 94d9bbf Compare October 18, 2018 09:30

jpkrohling changed the title ~~WIP - Add support for Cassandra create-schema job~~ Add support for Cassandra create-schema job Oct 18, 2018

objectiser reviewed Oct 18, 2018

View reviewed changes

jpkrohling added 3 commits October 18, 2018 16:08

Add support for Cassandra create-schema job

9eedaf2

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Added e2e tests

12f5a4c

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Fix typo on godoc for WaitForJob

090da1a

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling force-pushed the Cassandra-Batch-Job branch from 94d9bbf to 090da1a Compare October 18, 2018 14:09

pavolloffay approved these changes Oct 19, 2018

View reviewed changes

jpkrohling merged commit 92fce64 into jaegertracing:master Oct 19, 2018

jpkrohling mentioned this pull request Nov 6, 2018

Run the Cassandra create-schema batch job #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Cassandra create-schema job #71

Add support for Cassandra create-schema job #71

jpkrohling commented Oct 17, 2018

jpkrohling commented Oct 17, 2018

jpkrohling commented Oct 17, 2018 •

edited

Loading

jpkrohling commented Oct 17, 2018

codecov bot commented Oct 17, 2018 •

edited

Loading

jaegerci-bot commented Oct 18, 2018

pavolloffay left a comment

pavolloffay Oct 18, 2018

jpkrohling Oct 18, 2018

jpkrohling commented Oct 18, 2018

jpkrohling commented Oct 18, 2018

objectiser Oct 18, 2018

jpkrohling Oct 18, 2018

objectiser Oct 18, 2018


		=== Cassandra

		When the storage type is set to Cassandra, the operator will automatically create a batch job that creates the required schema for Jaeger to run. This batch job will block the Jaeger installation, so that it starts only after the schema is successfuly created. The creation of this batch job can be disabled by setting the `enabled` property to `false`:

Add support for Cassandra create-schema job #71

Add support for Cassandra create-schema job #71

Conversation

jpkrohling commented Oct 17, 2018

jpkrohling commented Oct 17, 2018

jpkrohling commented Oct 17, 2018 • edited Loading

jpkrohling commented Oct 17, 2018

codecov bot commented Oct 17, 2018 • edited Loading

Codecov Report

jaegerci-bot commented Oct 18, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

pavolloffay Oct 18, 2018

Choose a reason for hiding this comment

jpkrohling Oct 18, 2018

Choose a reason for hiding this comment

jpkrohling commented Oct 18, 2018

jpkrohling commented Oct 18, 2018

objectiser Oct 18, 2018

Choose a reason for hiding this comment

jpkrohling Oct 18, 2018

Choose a reason for hiding this comment

objectiser Oct 18, 2018

Choose a reason for hiding this comment

jpkrohling commented Oct 17, 2018 •

edited

Loading

codecov bot commented Oct 17, 2018 •

edited

Loading