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

[Backport 1.18] Enable Windows workers (#17555) #17814

Merged
merged 7 commits into from
Aug 31, 2021

Conversation

davinci26
Copy link
Member

Commit Message:

Backport 1260c5c to 1.18

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

Risk Level: Low
Testing: Manual testing and tests
Docs Changes: Added
Release Notes: Added a bug fix note
Platform Specific Features: N/A

Sotiris Nanopoulos added 2 commits August 23, 2021 14:23
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>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17814 (comment) was created by @davinci26.

see: more, trace.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Member Author

cc @wrowe

@davinci26 davinci26 added backport/review Request to backport to stable releases area/windows labels Aug 24, 2021
@dmitri-d
Copy link
Contributor

This lgtm, but with a fat warning that I don't know much about Windows idiosyncrasies.

@wrowe
Copy link
Contributor

wrowe commented Aug 24, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17814 (comment) was created by @wrowe.

see: more, trace.

@wrowe
Copy link
Contributor

wrowe commented Aug 25, 2021

Need to merge main here, I believe compile_time_options phase is fixed on main.

@mathetake
Copy link
Member

#17868 needs to land beforehand.

@wrowe
Copy link
Contributor

wrowe commented Aug 30, 2021

@mathetake was correct, need to merge to branch one more time here @davinci26 and I think it will pass CI to be merged.

Sotiris Nanopoulos added 2 commits August 30, 2021 17:18
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM, it would be nice to have several working examples to compare between 1.18/1.19/main evolution of the rest of the code. TYVM for the backport submission.

@wrowe wrowe merged commit ce71ae0 into envoyproxy:release/v1.18 Aug 31, 2021
@alyssawilk alyssawilk removed the backport/review Request to backport to stable releases label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants