Skip to content

CI: Move asan workflow to Engflow's remote execution#1745

Merged
goaway merged 46 commits into
envoyproxy:mainfrom
lfpino:luis-asan2
Sep 2, 2021
Merged

CI: Move asan workflow to Engflow's remote execution#1745
goaway merged 46 commits into
envoyproxy:mainfrom
lfpino:luis-asan2

Conversation

@lfpino

@lfpino lfpino commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

Description:
CI: Move asan workflow to Engflow's remote execution and cut build times by 5x

Before (~2hr):
Screenshot from 2021-08-27 18-03-41

After (~20min):
Screenshot from 2021-08-31 00-38-38

Due to google/sanitizers#916 we add an extra capability (SYS_PTRACE) to the remote execution docker container, we do this by adding a new Clang ASAN toolchain. Otherwise, dangling threads can fail the test during teardown (example with a previous run: https://github.com/envoyproxy/envoy-mobile/runs/3443649963).

Risk Level: Low
Testing: See asan workflow
Docs Changes: N/A
Release Notes: N/A

lfpino and others added 30 commits August 24, 2021 17:26
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
@lfpino lfpino marked this pull request as ready for review August 30, 2021 22:47
@lfpino

lfpino commented Aug 30, 2021

Copy link
Copy Markdown
Contributor Author

/cc @goaway

@goaway goaway self-assigned this Aug 30, 2021
@goaway goaway self-requested a review August 30, 2021 23:16

@goaway goaway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @lfpino!

@goaway

goaway commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

@lfpino I just noticed that leak sanitization has been turned off. As much as I really would like to merge this, we've relied on leak sanitization to catch issues in the past, and they have the potential to be quite disastrous on mobile devices. Is it possible to update our tests so that we can leave this enabled?

@goaway goaway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing review status until we can discuss leak detection further.

@lfpino

lfpino commented Aug 31, 2021

Copy link
Copy Markdown
Contributor Author

I understand and thanks for taking a look. Let me see if I can find an alternative and I'll get back to you.

lfpino and others added 4 commits August 31, 2021 14:40
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
@lfpino

lfpino commented Aug 31, 2021

Copy link
Copy Markdown
Contributor Author

I found a different workaround that doesn't require deactivating leak sanitization. Please take a look!

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
@lfpino

lfpino commented Aug 31, 2021

Copy link
Copy Markdown
Contributor Author

Fully cached asan builds are down to 3 minutes!

@goaway goaway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @lfpino!

@goaway goaway merged commit a7a0bd1 into envoyproxy:main Sep 2, 2021
carloseltuerto pushed a commit to carloseltuerto/envoy-mobile that referenced this pull request Sep 7, 2021
Description: Due to google/sanitizers#916 we add an extra capability (SYS_PTRACE) to the remote execution docker container, we do this by adding a new Clang ASAN toolchain. Otherwise, dangling threads can fail the test during teardown (example with a previous run: https://github.com/envoyproxy/envoy-mobile/runs/3443649963). 

Risk Level: Low
Testing: See asan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
goaway pushed a commit that referenced this pull request Sep 10, 2021
Description: Move tsan workflow to Engflow's remote execution

Same as #1745 but using a couple of tsan flags instead of asan. Using a separate config for modularity.

Risk Level: Low
Testing: See tsan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
Description: This change is a no-op that will be used for switching more CI workflows to Engflow's remote execution.

See envoyproxy/envoy-mobile#1745 for preliminary results.

Risk Level: Low
Testing: Manually tested
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
Description: Move tsan workflow to Engflow's remote execution

Same as envoyproxy/envoy-mobile#1745 but using a couple of tsan flags instead of asan. Using a separate config for modularity.

Risk Level: Low
Testing: See tsan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Description: This change is a no-op that will be used for switching more CI workflows to Engflow's remote execution.

See envoyproxy/envoy-mobile#1745 for preliminary results.

Risk Level: Low
Testing: Manually tested
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Description: Move tsan workflow to Engflow's remote execution

Same as envoyproxy/envoy-mobile#1745 but using a couple of tsan flags instead of asan. Using a separate config for modularity.

Risk Level: Low
Testing: See tsan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
Signed-off-by: JP Simard <jp@jpsim.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.

2 participants