Skip to content

Remove support for hidden_envoy_deprecated_hosts#16931

Merged
zuercher merged 7 commits intoenvoyproxy:mainfrom
tyxia:host
Jun 16, 2021
Merged

Remove support for hidden_envoy_deprecated_hosts#16931
zuercher merged 7 commits intoenvoyproxy:mainfrom
tyxia:host

Conversation

@tyxia
Copy link
Copy Markdown
Member

@tyxia tyxia commented Jun 11, 2021 •
edited
Loading

  • Remove the code related to hidden_envoy_deprecated_hosts
  • Remove the helper function translateClusterHosts() which is used to translated hosts to load_assignment and becomes redundant now.
  • Modify the test to not add deprecated hosts.

Note to reviewer: I also removed the RELEASE_ASSERT for deprecated_hosts in utility.cc, which could still be useful for asserting the regression. Please let me know if you want to keep them.

Signed-off-by: Tianyu Xia tyxia@google.com

Risk Level: Low
Testing: All tests locally (i.e. bazel test //test/...)

tyxia added 2 commits June 10, 2021 23:06
@tyxia
Remove support related to hidden_envoy_deprecated_hosts
c777bb3 
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Remove support for hidden_envoy_deprecated_hosts
e377d32 
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia tyxia requested a review from mattklein123 as a code owner June 11, 2021 00:55
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 11, 2021

Hi @tyxia, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16931 was opened by tyxia.

see: more, trace.

@daixiang0
Copy link
Copy Markdown
Member

daixiang0 commented Jun 11, 2021

Is it marked as deprecated in docs?

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jun 11, 2021 •
edited
Loading

Is it marked as deprecated in docs?

Hi,
+@htuch

I am not sure if this is documented specifically somewhere. My understanding is that all the fields have hidden_envoy_deprecated are deprecated in xDS V3. Also, the variable naming itself and code indicate they are deprecated: we can see we already have the asserts to make sure this hidden_envoy_deprecated_hosts is not in use. (i.e. Assert in test/config/utility.cc).

Please let me know if you have other questions.

@htuch
Copy link
Copy Markdown
Member

htuch commented Jun 11, 2021

Yes, this has been long deprecated and is no longer available in v3.

htuch
htuch previously approved these changes Jun 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@zuercher
Copy link
Copy Markdown
Member

zuercher commented Jun 11, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 11, 2021

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

🐱

Caused by: a #16931 (comment) was created by @zuercher.

see: more, trace.

@zuercher zuercher self-assigned this Jun 11, 2021
@tyxia
Fix the CI failure that was introduced by last minute exception strin…
1277750 
…g update

Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jun 12, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 12, 2021

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #16931 (comment) was created by @tyxia.

see: more, trace.

tyxia added 2 commits June 12, 2021 03:07
@tyxia
fix format
e5efd10 
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
revert the changes in bazelrc
af71487 
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jun 12, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 12, 2021

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #16931 (comment) was created by @tyxia.

see: more, trace.

@tyxia
fix warning
cf82a50 
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jun 13, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 13, 2021

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

🐱

Caused by: a #16931 (comment) was created by @tyxia.

see: more, trace.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jun 14, 2021 •
edited
Loading

Awesome, thanks!

Thanks for review, Harvey! There was a CI failure introduced by my last minute exception string update. It is fixed in 1277750. Sorry I didn't run it locally and catch it in the first place(thought it was just simple string message update but didn't realized there is a test compare the strings).

The other failure in CI is related to the flakiness of the health_check_integration test itself. And this should be fixed by #16980
Please take another look. Thanks!

htuch
htuch approved these changes Jun 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch 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!

@zuercher zuercher merged commit e737d74 into envoyproxy:main Jun 16, 2021
@tyxia tyxia deleted the host branch June 26, 2021 01:32
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@tyxia
Remove support for hidden_envoy_deprecated_hosts (envoyproxy#16931)
b03c11d 
* Remove the code related to hidden_envoy_deprecated_hosts
* Remove the helper function translateClusterHosts() which is used to translated hosts to load_assignment
  and becomes redundant now.
* Modify the test to not add deprecated hosts.

Risk Level: Low
Testing: All tests locally (i.e. bazel test //test/...)
Release Notes: n/a

Signed-off-by: Tianyu Xia <tyxia@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuercher zuercher zuercher approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

+1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zuercher zuercher

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tyxia @daixiang0 @htuch @zuercher