Mount volumes from agent spec #1102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1102 +/- ##
=======================================
Coverage 88.00% 88.00%
=======================================
Files 86 86
Lines 5254 5254
=======================================
Hits 4624 4624
Misses 466 466
Partials 164 164
Continue to review full report at Codecov.
|
… for sidecar Signed-off-by: Saad Hussain <[email protected]>
Saad-Hussain1
force-pushed
the
sidecar-volumeMounts
branch
from
528bb3c to
c0fdc91
Compare
Signed-off-by: Saad Hussain <[email protected]>
| // We don't want to mount all Jaeger internal volumes into user's deployments | ||
| volumesAndMountsSpec := &v1.JaegerCommonSpec{} | ||
| volumesAndMountsSpec := &jaeger.Spec.Agent.JaegerCommonSpec | ||
| otelConf, err := jaeger.Spec.Agent.Config.GetMap() |
There was a problem hiding this comment.
This would require users to always specify the volume under the agent node in order to be consumed here, right? Meaning, if I specified a volume and mount with the TLS at the root of the CR, it will be applied to every component except to the deployments where the sidecar has been injected.
I like this approach, perhaps @objectiser and/or @pavolloffay would also want to give their opinion.
There was a problem hiding this comment.
Yes, only the volumes under spec.agent would be added to the deployment, not those under just spec.
This was done to satisfy the comment in the code:
// We don't want to mount all Jaeger internal volumes into user's deployments
So all the common spec volumes aren't added to the deployment still, but if someone specifically does want a volume & volumeMount added to the sidecar's deployment & sidecar container respectively, they can specify it under the agent node.
| @@ -158,6 +158,36 @@ func TestInjectSidecarWithEnvVarsOverridePropagation(t *testing.T) { | |||
| assert.Contains(t, dep.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{Name: envVarServiceName, Value: "testapp.default"}) | |||
| } | |||
|
|
|||
| func TestInjectSidecarWithVolumeMounts(t *testing.T) { | |||
| // prepare | |||
There was a problem hiding this comment.
Could you please add an explicit test for the condition I mentioned in the previous comment? Add a volume/mount at the root of the spec and make sure that it does not get mounted.
…s deployment Signed-off-by: Saad Hussain <[email protected]>
|
LGTM. If you think this is ready to be merged, change this from a Draft PR to a regular PR :) |
|
@jpkrohling Changed to regular PR |
|
Thank you for your contribution, @Saad-Hussain1! |
Context: #1097
This PR uses any
volumesorvolumeMountsunder the agent node in a Jaeger instance when creating an agent as a sidecar. This still doesn't use the common spec for volumes/mounts, as it seems, "We don't want to mount all Jaeger internal volumes into user's deployments".However, if someone were to specify a volume/mount under the agent node in the Jaeger instance, they would be mounted to the sidecar, since this would be very intentional.
This seems necessary if one needs to supply files to the agent (e.g. for TLS between the agent and collector with the arg
--reporter.grpc.tls.ca).