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

http: making the lua filter a dual filter #38465

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yuval-k
Copy link
Contributor

@yuval-k yuval-k commented Feb 15, 2025

Commit Message:
http: making the lua filter a dual filter
Additional Description: n/a
Risk Level: Low
Testing: Updated integration tests
Docs Changes: n/a
Release Notes: inline
Platform Specific Features: n/a
Part of #10455

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
@wbpcode wbpcode self-assigned this Feb 17, 2025
@wbpcode
Copy link
Member

wbpcode commented Feb 17, 2025

/wait on ci

…nd new functionality.

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
@yuval-k
Copy link
Contributor Author

yuval-k commented Feb 17, 2025

I'm seeing TSAN timing out in the middle of running the integration tests (the new integration test covers more combinations so it takes longer). I'm seeing the timeout set to 180 min in .github/workflows/_check_san.yml; but the test times out after 35 minutes.. do we know where the 35 min comes from?

2025-02-17T14:38:27.7895112Z -- Test timed out at 2025-02-17 14:34:47 UTC --

(I ran TSAN locally and it passes, so pretty sure it's just a time-out issue)

@wbpcode
Copy link
Member

wbpcode commented Feb 18, 2025

/retest

1 similar comment
@wbpcode
Copy link
Member

wbpcode commented Feb 19, 2025

/retest

… class for upstream filters results in 1216 tests

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
@yuval-k
Copy link
Contributor Author

yuval-k commented Feb 19, 2025

timeout still seems to be 30 min, even though I changed the test's size to enormous.
Locally this test takes around 1180 seconds, so I imagine it can cross the 1800 seconds in a slower CI VM.

I'm really not sure where that 30 minute value is coming from.. @wbpcode do you have any idea?

@wbpcode
Copy link
Member

wbpcode commented Feb 20, 2025

timeout still seems to be 30 min, even though I changed the test's size to enormous. Locally this test takes around 1180 seconds, so I imagine it can cross the 1800 seconds in a slower CI VM.

I'm really not sure where that 30 minute value is coming from.. @wbpcode do you have any idea?

Sorry, I also have no much idea. 😞 An eternal should use 3600s as the timeout. I think @phlax may could helps to this CI timeout problem.

@phlax
Copy link
Member

phlax commented Feb 20, 2025

this is the execution log https://mordenite.cluster.engflow.com/actions/executions/ChCgHfMQ0UNCiqLjEC0JFA-PEgdkZWZhdWx0GiUKIBR15GaFEZlQjH9EFo_3Ehks2raEKWcQQPwE5Z0MFeBPEKgD which suggests that the timeout is set to 4800s - so confused

either way - it would be better to avoid setting enormous

im wondering if the test can be made quicker - as 30m+ is a long time for a test

would increasing core count help at all?

@yuval-k
Copy link
Contributor Author

yuval-k commented Feb 20, 2025

would increasing core count help at all?

I believe google-test runs all the test of a single binary serially; so not sure if that would help. That said, if its easy to change, maybe worth a try?

@phlax
Copy link
Member

phlax commented Feb 20, 2025

yeah - given it fails reliably - change in the test rule rbe_pool = "6gig" -> rbe_pool = "2core"

@yuval-k
Copy link
Contributor Author

yuval-k commented Feb 20, 2025

Another option to make the test take less time is to remove http3 testing... not sure if it adds value for lua integration testing. running it locally without http3 takes around 600s which is ~50% percent less than before (1180s)

…a integration test

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants