Skip to content

[xDS Proto] Update code to use Envoy protos instead of copy#29221

Closed
lidizheng wants to merge 5 commits intogrpc:masterfrom
lidizheng:xp-code
Closed

[xDS Proto] Update code to use Envoy protos instead of copy#29221
lidizheng wants to merge 5 commits intogrpc:masterfrom
lidizheng:xp-code

Conversation

@lidizheng
Copy link
Copy Markdown
Contributor

@lidizheng lidizheng commented Mar 25, 2022 •
edited
Loading

Third step of #25272.

This PR replaces the usage of existing copied protos to the ground truth Envoy protos, including:

  • C++: CSDS usages, client_lb end2end tests, xds end2end tests
  • Python: grpcio-csds usages and tests

Todo

@lidizheng lidizheng added the release notes: no Indicates if PR should not be in release notes label Mar 25, 2022
@lidizheng lidizheng changed the title [xDS Proto] Update code to use Envoy protos instead of copy @lidizheng [xDS Proto] Update code to use Envoy protos instead of copy Mar 25, 2022
@lidizheng lidizheng mentioned this pull request Mar 25, 2022
Closed
@lidizheng lidizheng force-pushed the xp-code branch 2 times, most recently from 2adb11c to 49ccc80 Compare April 14, 2022 23:45
markdroth
markdroth reviewed Apr 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

test/cpp/end2end/xds/BUILD
"//src/proto/grpc/testing/xds/v3:lrs_proto",
"//src/proto/grpc/testing/xds/v3:route_proto",
"//src/proto/grpc/testing/xds/v3:router_proto",
"//src/proto/grpc/lookup/v1:rls_config_proto",
Copy link
Copy Markdown
Member

@markdroth markdroth Apr 15, 2022

Choose a reason for hiding this comment

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

Can we also use the rls protos from the grpc/grpc-proto repo instead of having our own local copy?

Feel free to do this in a separate PR.

Copy link
Copy Markdown
Contributor Author

@lidizheng lidizheng Apr 15, 2022

Choose a reason for hiding this comment

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

Jan and I had some discussion about using the grpc/grpc-proto protos. The benefits are: 1. (as mentioned) remove the copy; 2. remove the hacks we did to override the proto package path (e.g., health.proto supposed to be located at grpc/health/v1/health.proto, but we put it under src/protos).

That's probably a bigger issue. It's on my todo list.

@lidizheng lidizheng marked this pull request as ready for review April 15, 2022 18:48
@lidizheng lidizheng requested review from gnossen and markdroth April 15, 2022 18:48
@lidizheng
Copy link
Copy Markdown
Contributor Author

lidizheng commented Apr 15, 2022

@gnossen PTAL at the changes under src/python and the requirement files.
@markdroth PTALA.

@lidizheng
Copy link
Copy Markdown
Contributor Author

lidizheng commented Apr 15, 2022

The presubmit checks found:

[ 33%] Running gRPC C++ protocol buffer compiler for xds/type/matcher/v3/string.proto
validate/validate.proto: File not found.
xds/type/matcher/v3/regex.proto:5:1: Import "validate/validate.proto" was not found or had errors.
xds/type/matcher/v3/string.proto:5:1: Import "xds/type/matcher/v3/regex.proto" was not found or had errors.
xds/type/matcher/v3/string.proto:7:1: Import "validate/validate.proto" was not found or had errors.
xds/type/matcher/v3/string.proto:46:5: "RegexMatcher" is not defined.
make[3]: *** [CMakeFiles/xds_interop_client.dir/build.make:2212: gens/xds/type/matcher/v3/string.grpc.pb.cc] Error 1
make[2]: *** [CMakeFiles/Makefile2:17881: CMakeFiles/xds_interop_client.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....

This could mean a proto dependency issue. This PR might require more work.

https://source.cloud.google.com/results/invocations/92cfcb69-00ad-4057-acce-0f751688278d/targets/github%2Fgrpc%2Ftoplevel_run_tests_invocations%2Frun_tests_c%2B%2B_linux_dbg_native_buildonly/tests

@lidizheng
Copy link
Copy Markdown
Contributor Author

lidizheng commented Apr 28, 2022

Closing this for #29474, which fixed the new conflicts. The new PR touched more parts of our codebase, I will split it into smaller ones to make review easier.

@lidizheng lidizheng closed this Apr 28, 2022
@lidizheng lidizheng mentioned this pull request Apr 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnossen gnossen Awaiting requested review from gnossen

@markdroth markdroth Awaiting requested review from markdroth

Assignees

@gnossen gnossen

@markdroth markdroth

Labels

bloat/none lang/c++ per-call-memory/neutral perf-change/none release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lidizheng @markdroth @gnossen