unwinder/native: Accomodate larger program due to rate limits #2059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The newly added rate limits increase the code size of the native unwinder. By default they are disabled, so the abstract paths that the verifier has to analyse are reduced, compared to when they are enabled. As in Go, structs with fields that aren't explicitely named have a default value, in the case of booleans, false, this feature was disabled in unit and integration tests, working both locally and CI. In the Agent binary we have this flag enabled by default. This worked in local testing because the kernel I run is new enough to allow for larger programs (6.4.6-200.fc38.x86_64) but both the e2e tests in CI and in our own Demo deployment, which run older kernels, the program fails to load with `Argument list too long`. Enabled rate limiting by default in tests shows the issue before this commit. To fix it, the the iterations per program were reduced. We might want to revisit this in the future. Test Plan ========= With rate-limits enabled both integration and unit tests pass now. Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
Sylfrena
approved these changes
Sep 25, 2023
|
Because of #2047 and #2047, we need to take a look at the Agent logs in the e2e tests before merging (https://github.com/parca-dev/parca-agent/actions/runs/6295970858/job/17090368809?pr=2059). edit: Let's roll this out to Demo before cutting a new release. Also heads up @Sylfrena as this might break in arm64. Very excited for the future kernel tests for arm64 to prevent these regressions! |
javierhonduco
deleted the
accomodate-larger-program-due-to-ratelimits
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The newly added rate limits increase the code size of the native unwinder. By default they are disabled, so the abstract paths that the verifier has to analyse are reduced, compared to when they are enabled.
As in Go, structs with fields that aren't explicitely named have a default value, in the case of booleans, false, this feature was disabled in unit and integration tests, working both locally and CI.
In the Agent binary we have this flag enabled by default. This worked in local testing because the kernel I run is new enough to allow for larger programs (6.4.6-200.fc38.x86_64) but both the e2e tests in CI and in our own Demo deployment, which run older kernels, the program fails to load with
Argument list too long.Enabled rate limiting by default in tests shows the issue before this commit. To fix it, the the iterations per program were reduced. We might want to revisit this in the future.
Test Plan
With rate-limits enabled both integration and unit tests pass now.