Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestLabelsDemoApp: Replace isovalent/jobs-app by Opentelemetry demo app #2345

Merged
merged 1 commit into from
May 3, 2024
Merged

TestLabelsDemoApp: Replace isovalent/jobs-app by Opentelemetry demo app #2345

merged 1 commit into from
May 3, 2024

Conversation

Trung-DV
Copy link
Contributor

@Trung-DV Trung-DV commented Apr 19, 2024

Migrate e2e labels test from isovalent/jobs-app to Opentelemetry demo app

  • Update the labels checker
  • remove -p 1 -parallel 1 flags when running tests

Fixes: #1976, #2176

@Trung-DV Trung-DV requested a review from a team as a code owner April 19, 2024 04:46
@Trung-DV Trung-DV requested a review from jrfastab April 19, 2024 04:46
Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 4be43fb
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6630a4ed7ec9c40008a1ceba
😎 Deploy Preview https://deploy-preview-2345--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mtardy mtardy added area/ci Related to CI release-note/ci This PR makes changes to the CI. labels Apr 19, 2024
@mtardy mtardy self-requested a review April 19, 2024 09:18
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks a lot for doing this work, I think the patch works, failures are unrelated. Could you maybe squash your commits into one?

I need to take a deeper look to see if we can make the label a bit more explicit maybe (add more of them if they are any) and test your patch locally in the meantime.

@mtardy mtardy self-assigned this Apr 19, 2024
@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

make the label a bit more explicit maybe (add more of them if they are any)

Those are the only ones set by the default helm chart install, so let's keep this it's plenty enough and it simplifies how they are checked!

@Trung-DV
Copy link
Contributor Author

make the label a bit more explicit maybe (add more of them if they are any)

Those are the only ones set by the default helm chart install, so let's keep this it's plenty enough and it simplifies how they are checked!

As mentioned in #2176 (comment), I replaced labels to be checked by looked into https://github.com/GoogleCloudPlatform/microservices-demo/tree/90b81cca772eb61b8695a638e44b9b2cde70853f/helm-chart/templates and saw only the app label :D

So just let it simple like this, right? @mtardy :D

@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

As mentioned in #2176 (comment), I replaced labels to be checked by looked into https://github.com/GoogleCloudPlatform/microservices-demo/tree/90b81cca772eb61b8695a638e44b9b2cde70853f/helm-chart/templates and saw only the app label :D

So just let it simple like this, right? @mtardy :D

This is perfect like this, thanks a lot!

@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

Again, if you can squash those commits, I'll run the tests and I guess we can merge this!

@mtardy mtardy requested review from mtardy and removed request for jrfastab April 19, 2024 10:52
@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

So we have a failure on e2e tests but I think it's unrelated https://github.com/cilium/tetragon/actions/runs/8752369371/job/24019870334?pr=2345#step:7:1061. Since all the checkers have matched, I don't think it's related to your changes. Let me just investigate a bit before merging this.

@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

By the way, it seems the deployment time and checker time is a ~50% (or a bit more) gain from before, that's great for CI speed.

@Trung-DV
Copy link
Contributor Author

By the way, it seems the deployment time and checker time is a ~50% (or a bit more) gain from before, that's great for CI speed.

Relate to the CI speed, I have a concern why we need -p 1 -parallel 1 flags here https://github.com/cilium/tetragon/blob/main/Makefile#L257?
As I understand, -p 1 means only 1 build command (for the test package) can run at a time, -parallel 1 means only 1 execution of test functions that call t.Parallel running.

An with my experience, the -parallel 1 seems redundant with -p 1. In case we want to make sure the tests do not run in parallel, we can set the -parallel 1 and let the compiler build the test packages in parallel. That could reduce the waiting time when running testing (https://github.com/cilium/tetragon/actions/runs/8752369371/job/24024402667?pr=2345#step:7:10 to https://github.com/cilium/tetragon/actions/runs/8752369371/job/24024402667?pr=2345#step:7:12)

@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

So we have a failure on e2e tests but I think it's unrelated https://github.com/cilium/tetragon/actions/runs/8752369371/job/24019870334?pr=2345#step:7:1061. Since all the checkers have matched, I don't think it's related to your changes. Let me just investigate a bit before merging this.

So in the end, even if the output is very confusing, it was related to this. Unfortunately the microservices-demo does not ship arm64 images for all the microservices and it seems now it builds okay, they just don't want to publish the image for some reason GoogleCloudPlatform/microservices-demo#622.

It worked fine locally on my computer since I use a rosetta share between my OS and the Linux VM I use and didn't realize the arch issue... Unfortunately it does not run as well with just qemu for x86_64 on arm64 :(!

I posted a message here but it seems it's pretty compromised to use this for our tests GoogleCloudPlatform/microservices-demo#622 (comment).

@mtardy
Copy link
Member

mtardy commented Apr 19, 2024

Just as a note, it worked previously because the event checker does not need everything to deploy gracefully to get all the events. Even with some of the services to CrashLoopBackoff it still got events. The thing blocks when trying to use the --wait flag with Helm because it needs the deployment to be fully successful.

So maybe we should find an alternative that really supports arm64, or just simplify this deployment with a more simple nginx deployment with custom labels that we can try to retrieve on the tetragon event instead of "real-life" example.

I know this is frustrating, especially since I was expecting something as big as a Google microservices demo to support arm64 out of the box :/!

mtardy
mtardy previously requested changes Apr 19, 2024
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah again I'm sorry, you did a really good job and helped us a lot on something we have been postponing forever and you just bumped into this not being the ideal solution in the end.

At least now we know it, and we can see what actions should be taken:

  • we can see if Google moves quickly on publishing the arm64 images;
  • or if we should find an alternative real-life deployment;
  • or just use something made-up that is simpler.

Let me chat about that with the rest of the team and see what we can do! Thanks again for your PR :)

@Trung-DV
Copy link
Contributor Author

Thank @mtardy for supporting me :D. While waiting for support from Google microservices demo, let me check that may we reduce the go test time 💪

@mtardy
Copy link
Member

mtardy commented Apr 22, 2024

Hey, @lambdanis suggested we use this demo instead https://github.com/open-telemetry/opentelemetry-demo, apparently, it's a fork of the Google microservice demo and I tested it on arm64, and it works fine! You can find the doc here https://opentelemetry.io/docs/demo/kubernetes-deployment/.

If you still have the motivation to switch the e2e tests to use this one it would be amazing, otherwise, I can update the issue with the new desired deployment and someone can pick this up.

@Trung-DV
Copy link
Contributor Author

Hey, @lambdanis suggested we use this demo instead https://github.com/open-telemetry/opentelemetry-demo, apparently, it's a fork of the Google microservice demo and I tested it on arm64, and it works fine! You can find the doc here https://opentelemetry.io/docs/demo/kubernetes-deployment/.

If you still have the motivation to switch the e2e tests to use this one it would be amazing, otherwise, I can update the issue with the new desired deployment and someone can pick this up.

Thank you for your suggestion. Let me change the Helm charts, that would be a great start for me 💪

@Trung-DV Trung-DV requested a review from willfindlay as a code owner April 30, 2024 07:59
@Trung-DV
Copy link
Contributor Author

Trung-DV commented Apr 30, 2024

Hi @mtardy , I have update the labelsEventChecker to match the otel-demo.
Since we have split e2e-tests to run separately, I suggest removing -p 1 -parallel 1 so that the go's compiler can compile the test package in parallel (which is limited by -p 1).

Help me to review this change :D

@Trung-DV Trung-DV changed the title TestLabelsDemoApp: Replace isovalent/jobs-app by Google onlineboutique TestLabelsDemoApp: Replace isovalent/jobs-app by Opentelemetry demo app Apr 30, 2024
Signed-off-by: Trung-DV <TrungDV.PMB@gmail.com>
@Trung-DV Trung-DV requested a review from mtardy May 1, 2024 11:26
@jrfastab
Copy link
Contributor

jrfastab commented May 3, 2024

LGTM we can follow up with the -p * if we need/want to.

@jrfastab jrfastab dismissed mtardy’s stale review May 3, 2024 19:05

Resolved by moving to otel demo.

@jrfastab jrfastab merged commit a3b867c into cilium:main May 3, 2024
38 checks passed
@mtardy
Copy link
Member

mtardy commented May 5, 2024

Lovely thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Related to CI release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate e2e labels test from isovalent/jobs-app
3 participants