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.

Sign up for GitHub

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

Jump to bottom

Listener: the API change for multiple addresses support #21391

Merged
merged 9 commits into from
Jun 9, 2022
Merged

Listener: the API change for multiple addresses support #21391

merged 9 commits into from
Jun 9, 2022

Conversation

soulxu
Copy link
Member

@soulxu soulxu commented May 20, 2022

Commit Message: Listener: the API change for multiple addresses support
Additional Description:

Both listener and admin API are changed to support multiple addresses.

Risk Level: low
Testing: n/a
Docs Changes: API doc
Release Notes: n/a
Platform Specific Features: n/a
Part of Fixes #11184

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #21391 was opened by soulxu.

see: more, trace.

@soulxu
Copy link
Member Author

soulxu commented May 20, 2022

This the API change separated from #19367

soulxu added 2 commits May 20, 2022 06:49
@soulxu
fix typo
33097ed 
Signed-off-by: He Jie Xu <[email protected]>
@soulxu
bump the next free field
1f96eb8 
Signed-off-by: He Jie Xu <[email protected]>
@soulxu soulxu mentioned this pull request May 26, 2022
Closed
@mattklein123 mattklein123 self-assigned this May 26, 2022
mattklein123
mattklein123 requested changes May 26, 2022
View reviewed changes
Copy link
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 a few comments.

/wait

@soulxu
address comments
0595758 
Signed-off-by: He Jie Xu <[email protected]>
@soulxu
Copy link
Member Author

soulxu commented May 27, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21391 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Member Author

soulxu commented May 27, 2022

seems CI issue also. not waste resources to retest again.

@soulxu
Copy link
Member Author

soulxu commented Jun 2, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21391 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Member Author

soulxu commented Jun 2, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21391 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Member Author

soulxu commented Jun 2, 2022

emm...why this keep failure, is that my problem? is it just a timeout https://dev.azure.com/cncf/envoy/_build/results?buildId=109367&view=logs&j=9847b632-4506-56d9-5b24-dbc653a96189&t=77343030-d1d4-546e-4c1c-b92bbd14ed15&l=76

@soulxu
Copy link
Member Author

soulxu commented Jun 6, 2022

The CI passed after merge with main

mattklein123
mattklein123 requested changes Jun 6, 2022
View reviewed changes
Copy link
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.

LGTM with small comments, thanks.

/wait

@soulxu
Address comments
7f34049 
Signed-off-by: He Jie Xu <[email protected]>
mattklein123
mattklein123 requested changes Jun 7, 2022
View reviewed changes
Copy link
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 LGTM with a small comment.

/wait

ggreenway
ggreenway reviewed Jun 7, 2022
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Before finalizing this, I just want to confirm that we do NOT think we need any other data with each address. Some examples of things that might be useful, but possibly none of these are needed:

  • different socket options
  • different stat prefix (if no stat-prefix is specified, we currently use the address as the stat-prefix; if a stat-prefix is specified then there's no way to get different stats for the different addresses)
  • anything related to internal listeners (cc @lambdai)
  • different value of freebind

Adding a message to repeat here gives us the ability to add more settings in the future if we need to, so it seems more future-proof.

Another way of thinking about it: the result of this is that additional listening sockets are setup. Will we ever need to set configuration to change any of the settings that are on a socket, to be different between the different addresses on the same envoy listener?

@mattklein123
Copy link
Member

Adding a message to repeat here gives us the ability to add more settings in the future if we need to, so it seems more future-proof.

Yeah I like this idea. Good point. Let's do it.

@soulxu
Copy link
Member Author

soulxu commented Jun 8, 2022 •
edited
Loading

  • different stat prefix (if no stat-prefix is specified, we currently use the address as the stat-prefix; if a stat-prefix is specified then there's no way to get different stats for the different addresses)

At least ensure that we already consider the pre-address stat prefix, but it is too complex for now. Thanks for the good point! let me update the PR

@soulxu
address comments
7a2bf3b 
Signed-off-by: He Jie Xu <[email protected]>
@soulxu
Copy link
Member Author

soulxu commented Jun 8, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21391 (comment) was created by @soulxu.

see: more, trace.

mattklein123
mattklein123 previously approved these changes Jun 8, 2022
View reviewed changes
Copy link
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.

LGTM, thanks. Will defer to @ggreenway for approve/merge.

@repokitteh-read-only repokitteh-read-only bot removed the api label Jun 8, 2022
ggreenway
ggreenway requested changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

/wait

@soulxu
remove repeated
542c094 
Signed-off-by: He Jie Xu <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added api and removed waiting labels Jun 8, 2022
@soulxu
Copy link
Member Author

soulxu commented Jun 9, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #21391 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Member Author

soulxu commented Jun 9, 2022 •
edited
Loading

@mattklein123 sorry, I lost your approval since I updated the PR :)

@repokitteh-read-only repokitteh-read-only bot removed the api label Jun 9, 2022
@mattklein123 mattklein123 merged commit 10533dd into envoyproxy:main Jun 9, 2022
tyxia pushed a commit to tyxia/envoy that referenced this pull request Jun 14, 2022
@soulxu @tyxia
Listener: the API change for multiple addresses support (envoyproxy#2…
8ad1e39 
…1391)

Signed-off-by: He Jie Xu <[email protected]>
Signed-off-by: Tianyu Xia <[email protected]>
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
@soulxu @Amila-Rukshan
Listener: the API change for multiple addresses support (envoyproxy#2…
e9ec37a 
…1391)

Signed-off-by: He Jie Xu <[email protected]>
Signed-off-by: Amila Senadheera <[email protected]>
@zhlsunshine zhlsunshine mentioned this pull request Oct 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@ggreenway ggreenway ggreenway approved these changes

Assignees

@ggreenway ggreenway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple addresses per listener
4 participants
@soulxu @mattklein123 @ggreenway @adisuissa