udp: no SO_REUSEPORT if not explicitly configured by danzh2010 · Pull Request #9495 · envoyproxy/envoy

danzh2010 · 2019-12-26T21:57:19Z

Only set SO_REUSEPORT socket option if config says so. If there are multiple UDP listeners and config doesn't explicitly set reuse_port to true, log a warning.

This change fixes a bazel test issue with SO_REUSEPORT. With that socket option enabled, bazel test with --run_per_test > 1 can be flaky because all the tests are created in different threads and they may pick up the same port. So the packets will be sent cross individual runs of tests. This makes some QUIC integration test flaky. I saw different runs of a test created listeners listening on the same port in the log.

Risk Level: low, not in use
Testing: fixing existing flaky tests
Part of #8794

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-12-26T21:59:55Z

/assign @htuch

mattklein123

Thanks for looking into this. Just to make sure I understand: when specifying port 0 with SO_REUSEPORT you can wind up w/ the same port? Is that right?

/wait

mattklein123 · 2019-12-26T22:24:16Z

source/server/listener_impl.cc

+  if ((socket_type == Network::Address::SocketType::Datagram && concurrency > 1) ||
+      config.reuse_port()) {


Won't this break restart cases in which the user still expects to be able to rebind? I think we probably need to make the option actually work for UDP also such that it can be disabled in config for the the tests?

How does restart case work if port is 0 in config? I would suspect in the old way, we can't guarantee rebinding to the same port either. Does restarting actually depends on SO_REUSEPORT?
If that's the case, for UDP we can make it work by prioritizing reuse_port in config. If that field is set to false, RELEASE_ASSERT failure if concurrency > 1, if concurrency == 1 not set SO_REUSEPORT. In this way, a config with concurrency > 1 and reuse_port not specified or set to false will crash at loading config.

I'm not worried about the port 0 case, just the case in which someone wants to start a new proxy with SO_REUSEPORT and then shut down the old proxy. I'm pretty sure there are people doing this, and I don't think we should break that case for UDP and concurrency 1?

How did user achieve that? SO_REUSEPORT only allows port re-bind within same process.

I could be wrong, but I'm pretty sure that's not true. I think it has to be part of the same process group, same owner, or something like that.

I changed the logic to only set SO_REUSEPORT if reuse_port == true in config.

danzh2010 · 2019-12-26T22:47:36Z

Thanks for looking into this. Just to make sure I understand: when specifying port 0 with SO_REUSEPORT you can wind up w/ the same port? Is that right?

Yes.

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2019-12-27T18:21:46Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9495 was synchronize by danzh2010.

see: more, trace.

danzh2010 · 2019-12-27T19:47:06Z

/assign @htuch for doc change

repokitteh-read-only · 2019-12-27T19:47:10Z

neither of for, doc, change can be assigned to this issue.

🐱

Caused by: a #9495 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-12-27T19:54:46Z

/assign @htuch
for doc change

htuch · 2019-12-29T04:15:09Z

/lgtm api

mattklein123

Thanks, LGTM with small comment. This is also going to need a master merge and format fix for the shadow API stuff.

/wait

mattklein123 · 2019-12-29T21:36:35Z

source/server/listener_impl.cc

+    ENVOY_LOG(warn, "Listening on UDP without SO_REUSEPORT socket option may result to unstable "
+                    "packet proxying.");


WDYT about the following text:

Listening on UDP without SO_REUSEPORT socket option may result in unstable packet proxying. Consider configuring the reuse_port listener option.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-12-30T20:58:47Z

/retest go_control_plane_mirror

repokitteh-read-only · 2019-12-30T20:58:51Z

🔨 rebuilding ci/circleci: go_control_plane_mirror (failed build)

🐱

Caused by: a #9495 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-12-30T21:46:13Z

/retest coverage

repokitteh-read-only · 2019-12-30T21:46:18Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #9495 (comment) was created by @danzh2010.

see: more, trace.

mattklein123

Awesome, thanks.

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

SO_REUSEPORT socket option can lead bazel test to use the same port across parallel jobs. This causes test flakiness in http_quic_integration_test with --runs_per_test > 1. To mitigate this issue, the integration test should run with enable_reuse_port explicitly false unless it needs concurrency_ > 1. For those tests with concurrency_ > 1, running with --jobs=1 would eliminate the flakiness. Additional Description: We used to disable SO_REUSEPORT for tests whose concurrency_ = 1 in #9495. However this was undo in #17259. Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com>

SO_REUSEPORT socket option can lead bazel test to use the same port across parallel jobs. This causes test flakiness in http_quic_integration_test with --runs_per_test > 1. To mitigate this issue, the integration test should run with enable_reuse_port explicitly false unless it needs concurrency_ > 1. For those tests with concurrency_ > 1, running with --jobs=1 would eliminate the flakiness. Additional Description: We used to disable SO_REUSEPORT for tests whose concurrency_ = 1 in envoyproxy#9495. However this was undo in envoyproxy#17259. Signed-off-by: Dan Zhang <danzh@google.com> Co-authored-by: Dan Zhang <danzh@google.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

no reuseport if concurrency is 1

d504d0f

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the fix_reuseport branch from b3052a3 to d504d0f Compare December 26, 2019 21:58

repokitteh-read-only bot assigned htuch Dec 26, 2019

mattklein123 requested changes Dec 26, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 26, 2019

mattklein123 self-assigned this Dec 26, 2019

htuch removed their assignment Dec 26, 2019

reuse_port apply to UDP

1770aa2

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Dec 27, 2019

danzh2010 changed the title ~~udp: no SO_REUSEPORT if concurrency == 1~~ udp: no SO_REUSEPORT if not explicitly configured Dec 27, 2019

update doc

3145454

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the api label Dec 27, 2019

repokitteh-read-only bot assigned htuch Dec 27, 2019

repokitteh-read-only bot removed the api label Dec 29, 2019

mattklein123 requested changes Dec 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 29, 2019

danzh1989 added 2 commits December 30, 2019 13:08

comment

68c9d1e

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into fix_reuseport

bee7f0f

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added api and removed waiting labels Dec 30, 2019

fix proto format

19c8cbb

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 approved these changes Dec 31, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Dec 31, 2019

mattklein123 merged commit 1e2a529 into envoyproxy:master Dec 31, 2019

prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020

udp: no SO_REUSEPORT if not explicitly configured (envoyproxy#9495)

2879cc4

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

danzh2010 mentioned this pull request Dec 3, 2021

quic: disable SO_REUSEPORT for integration tests #19188

Merged

		if ((socket_type == Network::Address::SocketType::Datagram && concurrency > 1) \|\|
		config.reuse_port()) {

		ENVOY_LOG(warn, "Listening on UDP without SO_REUSEPORT socket option may result to unstable "
		"packet proxying.");

Conversation

danzh2010 commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Dec 26, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Dec 26, 2019

Uh oh!

repokitteh-read-only bot commented Dec 27, 2019

Uh oh!

danzh2010 commented Dec 27, 2019

Uh oh!

repokitteh-read-only bot commented Dec 27, 2019

Uh oh!

danzh2010 commented Dec 27, 2019

Uh oh!

htuch commented Dec 29, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Dec 30, 2019

Uh oh!

repokitteh-read-only bot commented Dec 30, 2019

Uh oh!

danzh2010 commented Dec 30, 2019

Uh oh!

repokitteh-read-only bot commented Dec 30, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danzh2010 commented Dec 26, 2019 •

edited

Loading