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.

Sign up for GitHub

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

Jump to bottom

Clang tidy deflaking / experiment in ci #574

Merged
merged 28 commits into from
Feb 19, 2021
Merged

Clang tidy deflaking / experiment in ci #574

merged 28 commits into from
Feb 19, 2021

Conversation

oschaaf
Copy link
Member

@oschaaf oschaaf commented Nov 16, 2020 •
edited
Loading

At some point clang-tidy runs broke in CI, and this couldn't be reproduced
locally. The after a while local runs would exit with a failure seemingly mid-
run. This PR addresses both these things.

  • Update the CircleCI SHA of the envoy build docker image.
  • Sync the script we use to run Envoy's build docker image.
  • Make clang-tidy runs use libstdc++ to avoid standard library
    headers from producing errors.
  • Sync run_clang_tidy.sh
  • Update MAINTAINERS.md to mention we should keep more things in sync.

Fixes #546

Signed-off-by: Otto van der Schaaf [email protected]

oschaaf added 17 commits November 16, 2020 19:22
@oschaaf
Clang tidy deflaking / experiment in ci
d74bdb2 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Update Envoy to 588d9344b31e6544869547c4bcd359b3b0f1d4cf
e227746 
- Fixes to accommodate upstream connection pool changes.
- Fixes to accommodate upstream cluster related changes.

Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Mirror some changes in Envoy just to see what happens.
282ab3a 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Regenerate README.md for the test server
cb2a3e2 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge remote-tracking branch 'upstream/master' into envoy-dep-update-48
3c98ef3 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge remote-tracking branch 'upstream/master' into envoy-dep-update-48
cc199ae 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Update Envoy to 73c2d57aa1dfd4dc91b76aec20723b114a1d17ee
69d2759 
WIP. Connection prefetching needs amendment / Envoy seems to support
overlapping functionality now. Our version broke, but perhaps we can
piggy back and make it non-experimental.

Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge remote-tracking branch 'upstream/master' into envoy-dep-update-48
898771b 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge remote-tracking branch 'upstream/master' into envoy-dep-update-48
7b492c8 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Back out change to source/server/README.md
de03d42 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge branch 'envoy-dep-update-48' into envoy-dep-update-49
8d49c09 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge remote-tracking branch 'upstream/master' into envoy-dep-update-49
b9d5e0a 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Bump Envoy to 8188e232a9e0b15111d30f4724cbc7bf77d3964a
22fb2c6 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Fix prefetch/connection re-use
ec71f33 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge branch 'envoy-dep-update-49' into clang-tidy-exp
4603088 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Regen README.md
937d26a 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Merge branch 'envoy-dep-update-49' into clang-tidy-exp
1a09ceb 
Signed-off-by: Otto van der Schaaf <[email protected]>
Base automatically changed from master to main January 16, 2021 22:00
oschaaf and others added 5 commits January 26, 2021 22:54
@oschaaf
Merge remote-tracking branch 'upstream/main' into clang-tidy-exp
5f3462a 
Signed-off-by: Otto van der Schaaf <[email protected]>
Merge remote-tracking branch 'upstream/main' into clang-tidy-exp
5fe580b 
Signed-off-by: Otto van der Schaaf <[email protected]>
clang-tidy: address complaints
3df8c4c 
Fixes envoyproxy#629

Signed-off-by: Otto van der Schaaf <[email protected]>
attempt to sync with envoy
c3bf404 
Signed-off-by: Otto van der Schaaf <[email protected]>
test
58b6d16 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Copy link
Member Author

oschaaf commented Feb 14, 2021

I'm a bit at a loss here, it's almost as if the clang tidy replacement binary is crashing or some such.

Otto van der Schaaf added 3 commits February 14, 2021 22:05
test
b86a6ce 
Signed-off-by: Otto van der Schaaf <[email protected]>
test
ba20a40 
Signed-off-by: Otto van der Schaaf <[email protected]>
Couple of fixes
8551a3c 
- Update the docker image sha for the circleci configuration
- Update run_envoy_docker.sh
- Update MAINTAINERS.md to mention that we should keep these
  things in sync when updating the Envoy dependency.

Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf oschaaf marked this pull request as ready for review February 14, 2021 23:24
@oschaaf oschaaf added the waiting-for-review A PR waiting for a review. label Feb 14, 2021
@mum4k mum4k requested a review from qqustc February 15, 2021 04:24
@mum4k
Copy link
Collaborator

mum4k commented Feb 15, 2021

@qqustc please review and assign back to me once done.

@oschaaf
Copy link
Member Author

oschaaf commented Feb 15, 2021

Unfortunately the clang-tidy check now fails after merging master here.
But I suspect one important part of this change may only get effective after we merge: the update of the circleci configuration.

qqustc
qqustc previously approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@qqustc qqustc left a comment

Choose a reason for hiding this comment

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

LGTM, reassign back to Jakub for review

mum4k
mum4k requested changes Feb 17, 2021
View reviewed changes
Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

@oschaaf how do we want to proceed with regards to the failing clang-tidy on this PR?

.circleci/config.yml Outdated
@@ -1,6 +1,6 @@
references:
envoy-build-image: &envoy-build-image # November 10th, 2020
envoyproxy/envoy-build-ubuntu:19a268cfe3d12625380e7c61d2467c8779b58b56
envoy-build-image: &envoy-build-image # Februari 14th, 2021
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) Typo: February

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in e122081. Let's stick to English :-)

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Feb 17, 2021
Fix typo
e122081 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Copy link
Member Author

oschaaf commented Feb 17, 2021

@oschaaf how do we want to proceed with regards to the failing clang-tidy on this PR?

I found out that it's possible to ssh into the CI env, I'm trying to figure out what's up. I'm stumped that clang_tidy passed after updating the docker image sha in the circleci config, and subsequently started failing again after merging main into this branch.

clang-tidy: Reduce parallelism.
358643a 
Signed-off-by: Otto van der Schaaf <[email protected]>
@oschaaf
Copy link
Member Author

oschaaf commented Feb 17, 2021

Allright, the PR resolves a few problems, but we it looks like we would sometimes silently get OOM killed. Aargh. In any case, let's respin CI to give the integration tests a chance to resolve.
/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: integration_test_coverage (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #574 (comment) was created by @oschaaf.

see: more, trace.

@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Feb 17, 2021
@mum4k mum4k merged commit ff0271e into envoyproxy:main Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mum4k mum4k mum4k approved these changes

@qqustc qqustc qqustc left review comments

Assignees

@mum4k mum4k

Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clang-tidy flake
3 participants
@oschaaf @mum4k @qqustc