Enable Windows workers #17555

davinci26 · 2021-07-30T21:09:05Z

Commit Message:

Fixing an issue where Envoy workers are not picking up connections on Windows.

The root cause of the issue is in the Windows kernel.

There are two issues that we need to consider:

If we want to listen from a duplicated socket then we need to duplicate it after we call listen on the original socket.
If duplicated sockets try to accept at the same time, then one of the accept calls might block. Even if the sockets are non-blocking.

The best way to work around that issue is to only listen/accept connections from the first worker thread and then use ExactConnectionBalance to dispatch the connection to another worker thread. This is a temporary solution until the underlying issue is fixed on Windows.

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com
Additional Description:
Risk Level: low windows only
Testing: integration tests added
Docs Changes: Added
Release Notes: Added
Platform Specific Features: windows only
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-07-30T21:09:08Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #17555 was opened by davinci26.

see: more, trace.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

…urrency Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

source/server/listener_impl.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

…urrency

ggreenway

/wait

source/server/listener_impl.cc

mattklein123

Thanks this generally makes sense to me modulo small comments. I would definitely have a release note for this, and also a TODO and tracking issue for a better OS level solution?

/wait

source/server/listener_impl.cc

test/integration/integration_test.cc

mattklein123 · 2021-08-04T15:10:04Z

test/integration/integration_test.cc

+  // We set the counters for the two workers to see how many connections each handles.
+  uint64_t w0_ctr = 0;
+  uint64_t w1_ctr = 0;
+  constexpr int loops = 5;


I would be considered about flakes here. Can you make sure to run this test 1000 times? I think with REUSE_PORT on linux and exact balance on Windows this is probably OK, but it might flake on OSX. I'm not sure.

I don't have a mac to test it. I have put this in a loop on Windows to verify the fix and it did not flake.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

…weird whitespace Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

mattklein123

Thanks LGTM modulo small comment typo.

/wait

test/integration/integration_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

addressed

ggreenway · 2021-08-05T05:09:12Z

@davinci26 can you write a commit message for this? I don't think what is in the PR description is accurate for what this is.

Fixing an issue where Envoy workers are not picking up connections on Windows. The root cause of the issue is in the Windows kernel. There are two issues that we need to consider: If we want to listen from a duplicated socket then we need to duplicate it after we call listen on the original socket. If duplicated sockets try to accept at the same time, then one of the accept calls might block. Even if the sockets are non-blocking. The best way to work around that issue is to only listen/accept connections from the first worker thread and then use ExactConnectionBalance to dispatch the connection to another worker thread. This is a temporary solution until the underlying issue is fixed on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

* Enable Windows workers (#17555) Fixing an issue where Envoy workers are not picking up connections on Windows. The root cause of the issue is in the Windows kernel. There are two issues that we need to consider: If we want to listen from a duplicated socket then we need to duplicate it after we call listen on the original socket. If duplicated sockets try to accept at the same time, then one of the accept calls might block. Even if the sockets are non-blocking. The best way to work around that issue is to only listen/accept connections from the first worker thread and then use ExactConnectionBalance to dispatch the connection to another worker thread. This is a temporary solution until the underlying issue is fixed on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * Guard the test to be Windows only, since we can't gurantee it without setting resuse_port on linux Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * trigger ci Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * reorder current.rst Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * revert change Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

* Enable Windows workers (#17555) Fixing an issue where Envoy workers are not picking up connections on Windows. The root cause of the issue is in the Windows kernel. There are two issues that we need to consider: If we want to listen from a duplicated socket then we need to duplicate it after we call listen on the original socket. If duplicated sockets try to accept at the same time, then one of the accept calls might block. Even if the sockets are non-blocking. The best way to work around that issue is to only listen/accept connections from the first worker thread and then use ExactConnectionBalance to dispatch the connection to another worker thread. This is a temporary solution until the underlying issue is fixed on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * fix spelling mistake Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * fix spelling v2 Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * trigger ci Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> * revert changelog changes Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Fixing an issue where Envoy workers are not picking up connections on Windows. The root cause of the issue is in the Windows kernel. There are two issues that we need to consider: If we want to listen from a duplicated socket then we need to duplicate it after we call listen on the original socket. If duplicated sockets try to accept at the same time, then one of the accept calls might block. Even if the sockets are non-blocking. The best way to work around that issue is to only listen/accept connections from the first worker thread and then use ExactConnectionBalance to dispatch the connection to another worker thread. This is a temporary solution until the underlying issue is fixed on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 force-pushed the attemptingToFixConcurrency branch from 476c6b3 to 4c348c9 Compare July 30, 2021 21:11

test dummy duplication in tests

3ce5724

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 force-pushed the attemptingToFixConcurrency branch from d21592f to 3ce5724 Compare August 2, 2021 01:01

Sotiris Nanopoulos added 3 commits August 2, 2021 12:36

change approach to skip listen calls and reorder things

030a002

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into attemptingToFixConc…

823a838

…urrency Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

make changes presentable

eff1acc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 commented Aug 2, 2021

View reviewed changes

source/server/listener_impl.cc Outdated Show resolved Hide resolved

Sotiris Nanopoulos added 2 commits August 2, 2021 17:46

clang tidy + comments

b8dd74a

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into attemptingToFixConc…

2ab6fd1

…urrency

davinci26 changed the title ~~[WIP] Draft CI, Enable Windows workers~~ Enable Windows workers Aug 3, 2021

davinci26 marked this pull request as ready for review August 3, 2021 16:19

davinci26 requested review from mattklein123 and ggreenway August 3, 2021 16:20

davinci26 assigned mattklein123 and ggreenway Aug 3, 2021

ggreenway requested changes Aug 3, 2021

View reviewed changes

source/server/listener_impl.cc Show resolved Hide resolved

source/server/listener_impl.cc Outdated Show resolved Hide resolved

source/server/listener_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 3, 2021

mattklein123 requested changes Aug 4, 2021

View reviewed changes

Addressing PR comments and adding docs

5034e6e

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Aug 4, 2021

Sotiris Nanopoulos added 3 commits August 4, 2021 09:39

format error message to make format happy although the format adds a …

b0b9f09

…weird whitespace Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix whitespace change

aaa6882

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix the warning message

6d9777b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

mattklein123 previously requested changes Aug 4, 2021

View reviewed changes

test/integration/integration_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 4, 2021

fix english in comment

15eb33e

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Aug 4, 2021

ggreenway approved these changes Aug 5, 2021

View reviewed changes

mattklein123 approved these changes Aug 5, 2021

View reviewed changes

mattklein123 merged commit 1260c5c into envoyproxy:main Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Windows workers #17555

Enable Windows workers #17555

davinci26 commented Jul 30, 2021 •

edited

Loading

repokitteh-read-only bot commented Jul 30, 2021

ggreenway left a comment

mattklein123 left a comment

mattklein123 Aug 4, 2021

davinci26 Aug 4, 2021

mattklein123 left a comment

ggreenway commented Aug 5, 2021

Enable Windows workers #17555

Enable Windows workers #17555

Conversation

davinci26 commented Jul 30, 2021 • edited Loading

repokitteh-read-only bot commented Jul 30, 2021

ggreenway left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Aug 4, 2021

Choose a reason for hiding this comment

davinci26 Aug 4, 2021

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

ggreenway commented Aug 5, 2021

davinci26 commented Jul 30, 2021 •

edited

Loading