Inject sidecar in properly annotated pods #58
Conversation
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 99.26% 99.29% +0.02%
==========================================
Files 16 17 +1
Lines 684 706 +22
==========================================
+ Hits 679 701 +22
Misses 5 5
Continue to review full report at Codecov.
|
|
Does it auto inject to all deployments in cluster or in namespace? |
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @jpkrohling, @objectiser, and @pavolloffay)
README.adoc, line 172 at r1 (raw file):
== Auto injection of Jaeger Agent sidecars The operator can also inject Jaeger Agent sidecars in `Deployment` workloads, provided that the deployment has the annotation `inject-jaeger-agent` with a suitable value. The values can be either `"true"` (as string), or the Jaeger instance name, as returned by `kubectl get jaegers`. When `"true"` is used, there should be exactly *one* Jaeger instance for the same namespace as the deployment, otherwise, the operator can't figure out automatically which Jaeger instance to use.
I think the Jaeger instance name needs to support namespace scoping - it is very likely that the jaeger instance will be deployed into a different namespace - e.g. in the case of istio, where Jaeger is deployed to the istio-system namespace. This makes me wonder whether the "true" option should be supported? unless it is possible for the operator running in one namespace to process deployments in another, and point their agent to the namespace hosting the Jaeger instance?
| // this has two reasons: | ||
| // 1) as it is, it would cause a circular dependency, so, we'd have to extract that constant to somewhere else | ||
| // 2) this specific string is part of the "public API" of the operator: we should not change | ||
| // it at will. So, we leave this configured just like any other application would |
There was a problem hiding this comment.
I always prefer using a constant. Even we don't change it it's easier from the maintenance point of view.
There was a problem hiding this comment.
A local constant, instead of the same constant as the one used in the inject package? Could be, in that case, I'll consider it for #59.
There was a problem hiding this comment.
I mean a single constant unique for the project if possible
There was a problem hiding this comment.
Then no, I'll keep it as string following the reasoning on the comment
|
btw. I find that "=true" option useful for simple deployments. Or we can default to the default name used by the operator. |
I actually thought about it when implementing, but I got some practical questions that I didn't want to figure out right now. I recorded #60 with the questions and I'd add support for inter-namespace injection in a follow up PR (as this here is already complex enough, IMO). |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
There was a problem hiding this comment.
Reviewed 14 of 15 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @pavolloffay and @jpkrohling)
|
Just pointing out that I would like to have it in Statefulsets! |
|
@cfontes that's great to hear! Could you please open an issue with the request? Bonus points if you could describe your use case. |
* Inject sidecar in properly annotated pods Signed-off-by: Juraci Paixão Kröhling <[email protected]>
This PR adds support for auto-injection of Jaeger agents into target
Deploymentobjects. Technically, this can be done for all objects containingPodobjects, likeStatefulSetorDaemonSet, but this PR limits the first implementation toDeployment, to get it tested first. If there's demand, it's relatively easy to add support for other constructs.Signed-off-by: Juraci Paixão Kröhling [email protected]