Skip to content

listener: envoy.reloadable_features.listener_reuse_port_default_enabled deprecation#19977

Merged
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
giantcroc:listener
Feb 23, 2022
Merged

listener: envoy.reloadable_features.listener_reuse_port_default_enabled deprecation#19977
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
giantcroc:listener

Conversation

@giantcroc
Copy link
Copy Markdown

@giantcroc giantcroc commented Feb 16, 2022

Signed-off-by: changran changran.wang@intel.com

Commit Message: remove envoy.reloadable_features.listener_reuse_port_default_enabled
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0
Copy link
Copy Markdown
Member

/cc @alyssawilk

@giantcroc giantcroc changed the title listener: remove listener_reuse_port_default_enabled listener: envoy.reloadable_features.listener_reuse_port_default_enabled deprecation Feb 16, 2022
@KBaichoo
Copy link
Copy Markdown
Contributor

/assign @mattklein123

It seems unclear if we want to remove this now as this might not yet be resolved?

As part of this change, the use of reuse_port for TCP listeners on both macOS and Windows has been disabled due to suboptimal behavior. See the field documentation for more information.

from https://www.envoyproxy.io/docs/envoy/latest/version_history/v1.20.0

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

I think it's OK to remove the runtime flag and just leave the default as disabled for OSX and Windows. Please merge main to fix CI.

/wait

Signed-off-by: changran <changran.wang@intel.com>
@giantcroc
Copy link
Copy Markdown
Author

/wait

@giantcroc
Copy link
Copy Markdown
Author

I think it's OK to remove the runtime flag and just leave the default as disabled for OSX and Windows. Please merge main to fix CI.

/wait

OK, thanks. I will fix it later.

@@ -530,7 +530,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
: ReusePortDefault::False;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A question, does it mean the user never can upgrade to the true default value by hot restart? So that means we should keep those code forever?

Copy link
Copy Markdown
Member

@soulxu soulxu Feb 22, 2022

Choose a reason for hiding this comment

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

@mattklein123 this question probably needs your help. I thought we should remove all the upgrade code. But from the code and comments, I understand the hot restart can't change the default value. That means we only can remove those codes when all the users have a cold restart, but that can't be forced. Then probably this code never can be deleted?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't have a lot of precedent for this. I think technically we could likely remove the code when the last version that had the old default falls out of support. You could leave a comment/TODO about this and we can circle back later? cc @envoyproxy/envoy-maintainers

@daixiang0
Copy link
Copy Markdown
Member

Please run ci/run_envoy_docker.sh 'ci/check_and_fix_format.sh' to fix CI.

Signed-off-by: changran <changran.wang@intel.com>
@mattklein123
Copy link
Copy Markdown
Member

LGTM pending @soulxu comments, thank you.

/wait

Signed-off-by: changran <changran.wang@intel.com>
@soulxu
Copy link
Copy Markdown
Member

soulxu commented Feb 23, 2022

LGTM, thanks for the update!

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit ea44d46 into envoyproxy:main Feb 23, 2022
mattklein123 added a commit that referenced this pull request Apr 26, 2022
RUNTIME_GUARD(envoy_reloadable_features_listener_reuse_port_default_enabled)

Was already removed in #19977
but somehow it's back.

Fixes #20987

Signed-off-by: Matt Klein <mklein@lyft.com>
alyssawilk pushed a commit that referenced this pull request Apr 26, 2022
RUNTIME_GUARD(envoy_reloadable_features_listener_reuse_port_default_enabled)

Was already removed in #19977
but somehow it's back.

Fixes #20987

Signed-off-by: Matt Klein <mklein@lyft.com>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
RUNTIME_GUARD(envoy_reloadable_features_listener_reuse_port_default_enabled)

Was already removed in envoyproxy#19977
but somehow it's back.

Fixes envoyproxy#20987

Signed-off-by: Matt Klein <mklein@lyft.com>
oschaaf pushed a commit to maistra/envoy that referenced this pull request Oct 26, 2022
RUNTIME_GUARD(envoy_reloadable_features_listener_reuse_port_default_enabled)

Was already removed in envoyproxy/envoy#19977
but somehow it's back.

Fixes envoyproxy/envoy#20987

Signed-off-by: Matt Klein <mklein@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants