Added automatic provisioning of Kafka when its operator is available #713
Conversation
jpkrohling
force-pushed
the
698-AutoProvision-Kafka
branch
2 times, most recently
from
f72a3f5 to
ca924b3
Compare
jpkrohling
force-pushed
the
698-AutoProvision-Kafka
branch
from
ca924b3 to
a86c117
Compare
| image: jaegertracing/jaeger-ingester:latest # TLS configuration is supported starting from 1.15 | ||
| options: | ||
| ingester: | ||
| deadlockInterval: 0 |
There was a problem hiding this comment.
Is this temporary - although hasn't the new default been merged and therefore in latest?
There was a problem hiding this comment.
Yes, tracked here: #717 . Once we have a released version, this is ready to be removed.
| @@ -0,0 +1,56 @@ | |||
| package v1beta1 | |||
There was a problem hiding this comment.
Is it not possible to import this from the strimzi project?
There was a problem hiding this comment.
No, they are a Java operator and do not generate Go artifacts.
| "strimzi.io/cluster": jaeger.Name, | ||
|
|
||
| // workaround for https://github.com/strimzi/strimzi-kafka-operator/issues/2107 | ||
| "app.kubernetes.io/managed---by": "jaeger-operator", |
There was a problem hiding this comment.
Just wondering if there will be migration issues when it is fixed? i.e. dealing with existing deployed instances with this modified key?
There was a problem hiding this comment.
I'm hoping they will have a new master release by the time we have a 1.15. An alternative is to require Strimzi 0.15.0 and provide instructions on how to use Strimzi from their master while 0.15.0 isn't out.
| jaeger.Annotations = map[string]string{} | ||
| } | ||
| // mark that we auto provisioned a kafka for this instance | ||
| jaeger.Annotations[v1.AnnotationProvisionedKafkaKey] = v1.AnnotationProvisionedKafkaValue |
There was a problem hiding this comment.
I see the purpose here - but if required, then need to change the key so that it is kafka specific. This also implies that the newStreamingStrategy is called multiple times? is it per reconciliation cycle?
There was a problem hiding this comment.
But then why create new kafka and kafkauser if already provisioned? Sorry possibly lack of understanding of the mechanism.
There was a problem hiding this comment.
Perhaps the comment I left for the previous item clarified this already, but if not, let me know.
pkg/strategy/streaming_test.go
Outdated
| { | ||
| Name: "volume-a", | ||
| MountPath: "/user/path", | ||
| }, { |
There was a problem hiding this comment.
Just out of curiosity, what is the reason for replacing two entries?
There was a problem hiding this comment.
The volume mounts are quite specific, with the contents being provided by the secret that is created by Strimzi based on the KafkUser resource. The names were quite generic, so, I prefixed the volumes/volume mounts with "kafkauser-". I opened a doc issue, to document that users should avoid creating a volume/volume mount prefixed with "kafkauser-", otherwise it might get overridden by the operator.
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling
force-pushed
the
698-AutoProvision-Kafka
branch
from
aa0dc6a to
7b38638
Compare
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
|
I think this PR is now ready for another review round. I believe all items were addressed and I did a full manual test using the new example file. |
|
@jpkrohling Should an e2e test be added as part of this PR, similar to the |
|
It's part of #714, I understand that @kevinearls will work on it soon. |
Signed-off-by: Juraci Paixão Kröhling [email protected]