Not preserve the ownership of the files copied in the autoinstrumentation #2695
Conversation
…tion. Closes open-telemetry#2655 Signed-off-by: Israel Blancas <[email protected]>
Co-authored-by: Mikołaj Świątek <[email protected]>
|
@iblancasa would it be possible to build e2e tests to verify this? |
We should consider running instrumentation e2e tests with images built from the repo imo at least in some circumstances. Not clear how annoying it would be. |
I'm not sure what to add to the current E2E tests to check this. Any ideas? |
I don't think there's an easy way to do it right now. Have you tried building these images locally and running E2E tests with them? |
This approach should be OK with the current images, right? We're not changing them. |
|
I thought for some reason we were changing the images themselves in this PR, sorry for the confusion. Existing E2E tests should probably be fine here, though I'm not 100% confident we have the necessary coverage for all instrumentations. |
What do you think I can add to increase the coverage? |
|
@swiatekm-sumo @iblancasa This #2702 PR fixes our instrumentation tests and checks if all the containers in the pods are started and not in Error or CrashLoopBackoff state. If any of the containers in the instrumented application pod fails, we collect all the container logs using catch: I have fixed the tests in the e2e-instrumentation test suite and checking the e2e-multi-instrumentation. By the way I ran into this issue in the e2e-multi-instrumenation for Java and Nodejs, checking for Java if I need to raise a bug. |
|
Can we merge this? |
| @@ -865,7 +865,7 @@ func TestInjectDotNet(t *testing.T) { | |||
| { | |||
| Name: dotnetInitContainerName, | |||
| Image: "img:1", | |||
| Command: []string{"cp", "-a", "/autoinstrumentation/.", dotnetInstrMountPath}, | |||
| Command: []string{"cp", "-r", "/autoinstrumentation/.", dotnetInstrMountPath}, | |||
There was a problem hiding this comment.
is cp -r available on all of the distributions we use? I tested it on my mac and it failed... i guess if our e2e tests are passing thats reason enough for me to believe it does work, but have you tested this locally on a cluster?
There was a problem hiding this comment.
I think we use busybox everywhere, so it should work.
There was a problem hiding this comment.
yeah i think just some extra confirmation that this worked on a local kind cluster would be good
There was a problem hiding this comment.
The current E2E are testing this if I'm not wrong @IshwarKanse
There was a problem hiding this comment.
So the current e2e tests are not checking the status of the containers in the pod. I have fixed that with the PR #2702 (pending merge )with the changes, we check all the containers in the instrumented app pod are running and the test will fail if any of the container fails.
|
@iblancasa having the same issues. |
Happy to heard it works! |
…tion (open-telemetry#2695) * Not preserve the ownership of the files copied in the autoinstrumentation. Closes open-telemetry#2655 Signed-off-by: Israel Blancas <[email protected]> * Update .chloggen/fix-2655.yaml Co-authored-by: Mikołaj Świątek <[email protected]> --------- Signed-off-by: Israel Blancas <[email protected]> Co-authored-by: Mikołaj Świątek <[email protected]>
Closes #2655.
Using
-ais a problem when you try to instrument pods that are not run as root.