Drop fuzz_coverage target from CI#39249
Closed
krinkinmu wants to merge 1 commit intoenvoyproxy:mainfrom
Closed
Conversation
I'm trying to switch back to static linking for coverage tests in Envoy CI to work around a bug in Clang/LLVM (see llvm/llvm-project#32849). However the change that switches to static linking fails `fuzz_coverage` with what seems like some kind of resource constraint (e.g., linker gets killed, I suspect, as a result of OOM, since static linking requires more resources). @yanavalsov suggested that fuzz_coverage target might not be the best way to measure fuzzing coverage (see envoyproxy#39030 (comment)) and given that I think we should at least consider disabling it until we can make it better. That's what this PR does. NOTE: @phlax also mention that he did some work to migrate coverage to EngFlow builds in the past and there might be PR that we could finish that will migrate fuzz_coverage to EngFlow. There is a chance that it will address the issue - I'm mentioning it here to give a complete overview of the possible alternatives here. Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Contributor
Author
|
@phlax mentioned that I should really test this PR is staging first, so I'm closing it for now (I cannot move it to back to draft) and will re-open after I can do more testing. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Commit Message:
I'm trying to switch back to static linking for coverage tests in Envoy CI to work around a bug in Clang/LLVM (see
llvm/llvm-project#32849).
However the change that switches to static linking fails
fuzz_coveragewith what seems like some kind of resource constraint (e.g., linker gets killed, I suspect, as a result of OOM, since static linking requires more resources).@yanavalsov suggested that fuzz_coverage target might not be the best way to measure fuzzing coverage (see
#39030 (comment)) and given that I think we should at least consider disabling it until we can make it better. That's what this PR does.
NOTE: @phlax also mention that he did some work to migrate coverage to EngFlow builds in the past and there might be PR that we could finish that will migrate fuzz_coverage to EngFlow. There is a chance that it will address the issue - I'm mentioning it here to give a complete overview of the possible alternatives here.
Additional Description:
You can find some related discussion in #39030 (specifically #39030 (comment), #39030 (comment), #39030 (comment), #39030 (comment) and #39030 (comment)).
@phlax suggested in slack that @adisuissa might be the person more familiar with the coverage targets in Envoy CI and should be involved.
Risk Level: low (in the sense that disabling this target on CI should not break anything, but let's discuss if there are some strategic risks with disabling this test target).
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
+cc @phlax @yanavlasov @adisuissa