Add resource requirements to sidecar agent #401
Conversation
Signed-off-by: Gary Brown <gary@brownuk.com>
|
It doesn't answer the issue raised in #70 related to how to define resource requirements if using both daemonset and sidecar agents, but addresses the more immediate issue of not being able to define resource limits for sidecar agents, when only that approach is being used. So currently the resource limits defined against the agent will be used for both daemonset and sidecar agents. |
Codecov Report
@@ Coverage Diff @@
## master #401 +/- ##
==========================================
+ Coverage 89.71% 89.72% +<.01%
==========================================
Files 64 64
Lines 3092 3094 +2
==========================================
+ Hits 2774 2776 +2
Misses 216 216
Partials 102 102
Continue to review full report at Codecov.
|
|
Shouldn't we have more fine-grained control over agent sidecar resources? There is already a PR which sets the resources based on annotations #169. I think this could go in as default global setting and be able to override it via annotations. |
| @@ -104,6 +104,8 @@ func container(jaeger *v1.Jaeger) corev1.Container { | |||
| jgCompactTrft := util.GetPort("--processor.jaeger-compact.server-host-port=", args, 6831) | |||
| jgBinaryTrft := util.GetPort("--processor.jaeger-binary.server-host-port=", args, 6832) | |||
|
|
|||
| commonSpec := util.Merge([]v1.JaegerCommonSpec{jaeger.Spec.Agent.JaegerCommonSpec, jaeger.Spec.JaegerCommonSpec}) | |||
There was a problem hiding this comment.
What does this do? Why isn't it enough to use just Spec.Agent.Resources?
There was a problem hiding this comment.
It allows common resource requirements to be defined in jaeger.Spec.Resources and only agent specific requirements defined in jaeger.Spec.Agent.Resources. Its a pattern used for all parts of the common spec (e.g. volumnes, mounts, annotations, etc).
There was a problem hiding this comment.
So if no agent resources are specified would it use ones from jaeger.Spec.Resources?
There was a problem hiding this comment.
Yes. It has been done from a consistency perspective - but if is not reasonable, then we just need to make sure it is documented clearly that for agent as a sidecar (or agent in general), the parent resources are not inherited.
There was a problem hiding this comment.
I would maybe prefer applying only resources on the agent spec as sidecar can populate to a lot of pods but I don't have strong objections.
|
@pavolloffay I've removed the |
|
@pavolloffay I'll keep like this for now, just for consistency. Most people will probably just set the requirements on the Agent itself - so if we decide to change later it probably won't have a big impact. @jpkrohling When you get a chance, would be good to get your thoughts on this. |
Related to #169
Adds resource requirements to the agent sidecar.
Signed-off-by: Gary Brown gary@brownuk.com