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

Bump Envoy to v1.20.1 #4075

Merged
merged 7 commits into from
Dec 1, 2021
Merged

Conversation

sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Oct 5, 2021

See release notes:
https://www.envoyproxy.io/docs/envoy/v1.20.0/version_history/current

Also bumps go-control-plane to latest

Deprecation of existing headermatch usage will need to be handled in the release after this is
merged. The "string match" header matching method is preferred but was
added in 1.20.1 and it was not present in 1.19.x, we will need to
release a version with Envoy 1.20.x compatibility and the subsequent
release will need to move to the "string match" method (rather than the
individual regex/exact/etc. methods.

@sunjayBhatia sunjayBhatia requested a review from a team as a code owner October 5, 2021 20:21
@sunjayBhatia sunjayBhatia requested review from tsaarni and stevesloka and removed request for a team October 5, 2021 20:21
@sunjayBhatia
Copy link
Member Author

Not sure if we want to merge for 1.19 or not cc @projectcontour/maintainers

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #4075 (63d5d5d) into main (39a31e6) will increase coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4075      +/-   ##
==========================================
+ Coverage   76.41%   76.52%   +0.10%     
==========================================
  Files         111      111              
  Lines        9883     9869      -14     
==========================================
  Hits         7552     7552              
+ Misses       2164     2150      -14     
  Partials      167      167              
Impacted Files Coverage Δ
internal/xds/v3/snapshotter.go 0.00% <0.00%> (ø)
internal/xdscache/snapshot.go 11.62% <0.00%> (+2.19%) ⬆️

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

👍 I don't see any reason not to ship it in Contour 1.19.

@skriss
Copy link
Member

skriss commented Oct 5, 2021

Should update the compat matrix and versions.yaml as well

@sunjayBhatia
Copy link
Member Author

looks like a few e2e tests not passing

submitted #4080 so we get a proper failure rather than a panic, but looks like some requests never succeeding in tests

@sunjayBhatia sunjayBhatia added the do not merge Don't merge this PR until this label is removed. label Oct 6, 2021
@sunjayBhatia
Copy link
Member Author

added do not merge since this is a bit of a curious one, tests seem to be failing in CI but not locally "on my machine"

@stevesloka
Copy link
Member

I tried running on my machine and seems like some tests are timing out. 🤔

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Oct 6, 2021

I tried running on my machine and seems like some tests are timing out. 🤔

yeah latest CI runs ~140s for the httpproxy suite, my local machine ~975 and CI ~882, some issues here for sure

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Oct 6, 2021

I tried running on my machine and seems like some tests are timing out. 🤔

yeah latest CI runs ~140s, my local machine ~975 and CI ~882, some issues here for sure

just switching back to the older Envoy, I get ~140s locally for the httpproxy suite, drastic change from the 1.20.0 image

@sunjayBhatia sunjayBhatia added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 6, 2021
@sunjayBhatia
Copy link
Member Author

I tried running on my machine and seems like some tests are timing out. 🤔

yeah latest CI runs ~140s, my local machine ~975 and CI ~882, some issues here for sure

just switching back to the older Envoy, I get ~140s locally for the httpproxy suite, drastic change from the 1.20.0 image

seemingly a lot of TLS handshake timeouts and stalling the tests when we're making requests, might also adjust the test suite to use an http client that has some more strict timeouts but its pretty black and white switching back to Envoy 1.19.1 with no changes to the tests, these issues go away (the other code changes in the PR are kept in, just changing the Envoy image)

@youngnick youngnick added this to the 1.19.0 milestone Oct 7, 2021
@youngnick youngnick modified the milestones: 1.19.0, 1.20.0 Oct 11, 2021
@sunjayBhatia
Copy link
Member Author

With a git bisect, was able to find this commit as the Envoy culprit: envoyproxy/envoy@ba474ac

@sunjayBhatia sunjayBhatia added release-note/small A small change that needs one line of explanation in the release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 13, 2021
@sunjayBhatia
Copy link
Member Author

adding some logging to our test suite, you can see the holdup is in waiting for requests to succeed after we program an HTTPProxy etc.

there aren't connection refused errors in the slow tests but rather TLS handshake timeouts, I assume as listeners/filter chains/worker threads are spun up and torn down with the new logic that was introduced; it appears connections are still accepted by Envoy but the new slowness is maybe a mixture of our tests initiating a connection before worker threads have properly been initialized and these connections being held open waiting for a response that will never come

our tests dont currently have a request timeout/they use the default go http client which has no HTTP request timeout and only has the default TLS handshake timeout from the default TLS transport so adding a timeout there should help

@sunjayBhatia
Copy link
Member Author

See envoyproxy/envoy#18616

@sunjayBhatia
Copy link
Member Author

testing with image docker.io/envoyproxy/envoy-dev:6f0ecc5ae16f3c39937dd13a3bc0327177a0e754 (and removed http client timeouts in tests) which has the fix from envoyproxy/envoy#18686

@sunjayBhatia
Copy link
Member Author

looks like that passed ^

We'll have to wait for the linked change to be backported to Envoy 1.20.x

@youngnick youngnick added blocked Blocked waiting on a dependency blocked/needs-envoy Categorizes the issue or PR as blocked because it needs changes in Envoy. and removed blocked Blocked waiting on a dependency labels Nov 2, 2021
@wrowe
Copy link

wrowe commented Nov 15, 2021

@sunjayBhatia could you fix the PR title? Thx!

@sunjayBhatia sunjayBhatia changed the title Bump Envoy to v1.20.0 Bump Envoy to v1.20.1 Nov 15, 2021
@github-actions
Copy link

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 30, 2021
@sunjayBhatia sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 30, 2021
See release notes:
https://www.envoyproxy.io/docs/envoy/v1.20.0/version_history/current

Also bumps go-control-plane to latest

Deprecation of existing headermatch usage will need to be handled in the release after this is
merged. The "string match" header matching method is preferred but was
added in 1.20.0 and it was not present in 1.19.x, we will need to
release a version with Envoy 1.20.0 compatibility and the subsequent
release will need to move to the "string match" method (rather than the
individual regex/exact/etc. methods.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia removed blocked/needs-envoy Categorizes the issue or PR as blocked because it needs changes in Envoy. do not merge Don't merge this PR until this label is removed. labels Nov 30, 2021
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

🎉

@sunjayBhatia sunjayBhatia merged commit af86d8c into projectcontour:main Dec 1, 2021
@sunjayBhatia sunjayBhatia deleted the bump-envoy-1.20 branch December 1, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants