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.

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

feat(transport): add google-c2p dependence to DirectPath #1260

Merged
merged 9 commits into from Oct 28, 2021

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Oct 19, 2021

DirectPath is migrating to Traffic Director, and we want to setup E2E tests based on TD for Bigtable and Spanner. This PR:

  1. Update grpc-go version to 1.41.0, which has RING_HASH LB policy support;
  2. Add the google-c2p resolver dependence to google-api-go-client;
  3. Add an environment variable GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS to enable using DirectPath over TD;
    Corresponding PR in gax-java: feat: add google-c2p dependence to DirectPath gax-java#1521.

Also note that currently there is a bug in grpc-go, and fix is grpc/grpc-go#4889. I have verified that with this fix Bigtable E2E test over DirectPath Traffic Director can pass. This PR has been merged to grpc-go and will release in grpc-go 1.42.0, which will be the minimal version for supporting google-c2p URI scheme.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2021
@codyoss
Copy link
Member

codyoss commented Oct 21, 2021

@mohanli-ml This library can no longer update the gRPC dependency as we stay compatible with 1.11. We are hoping to up our supported version early next year. But for now if there are new features needed in gRPC we need to try to get them backported or wait.

@codyoss
Copy link
Member

codyoss commented Oct 21, 2021

cc @dfawley This is the feature I had flagged in earlier discussions.

@dfawley
Copy link

dfawley commented Oct 21, 2021

Is it possible for customers that want directpath to declare grpc v1.41 in their go.mods?

@dfawley
Copy link

dfawley commented Oct 25, 2021

Monday morning ping. My question above was for @mohanli-ml, to see if it's feasible to have customers do this themselves (use grpc-go v1.41+), instead of doing it globally.

@mohanli-ml
Copy link
Contributor Author

Thanks Doug, I have the same questions too. @codyoss

I just tried to move the xds registry _ "google.golang.org/grpc/xds/googledirectpath" to bigtable-go, and E2E test could pass.

@codyoss
Copy link
Member

codyoss commented Oct 26, 2021

@mohanli-ml There is no way for us to optionally declare a dependency at a higher version is some situations and lower in others. Build tags don't help in this type of situation either.

@dfawley
Copy link

dfawley commented Oct 26, 2021

The question is about two things:

  1. Are new (>=v1.41) grpc-go symbols required to be referenced from client libraries to make this work?
  2. Is it acceptable to customers to have them add grpc-go @v1.41 to their go.mod files? (EDIT: for directpath to be enabled)

If the answers are "no, yes", then we don't need to pin v1.41 here; users can do it instead. Otherwise, we have to figure something out.

@mohanli-ml
Copy link
Contributor Author

mohanli-ml commented Oct 26, 2021

  1. Are new (>=v1.41) grpc-go symbols required to be referenced from client libraries to make this work?

Yes. To make this work we need grpc/grpc-go#4889, so I think the minimal grpc-go version is 1.42.

  1. Is it acceptable to customers to have them add grpc-go @v1.41 to their go.mod files? (EDIT: for directpath to be enabled)

I think the answer is No, as explained by Cody: "If I took a dep bump to 41 in any client library today that means no customer could deploy to cloud functions /w Go and a modern version of the client library. We need to try not to move passed the types of environments that our serverless teams support in GA today."

@dfawley
Copy link

dfawley commented Oct 26, 2021

Yes. To make this work we need grpc/grpc-go#4889, so I think the minimal grpc-go version is 1.42.

This PR doesn't seem to create any new exported symbols, so I believe this statement is false. We would simply tell users "if you want to use directpath, please make sure you are using grpc-go v1.42+", rather than enforcing that requirement here.

I think the answer is No, as explained by Cody: "If I took a dep bump to 41 in any client library today that means no customer could deploy to cloud functions /w Go and a modern version of the client library. We need to try not to move passed the types of environments that our serverless teams support in GA today."

This statement by Cody is about changing the default requirement here. Or are you implying it's required for us to support directpath for cloud functions users before CF rolls out a less-ancient version of Go to their portfolio? Note the cloud functions does have "preview" support for 1.16, so they could run in that environment. If directpath is also "preview" at this point, then that seems not unreasonable to me.

@mohanli-ml
Copy link
Contributor Author

Thanks Doug! After investigation I think we should be able to enable XDS support in google-api-go-client without enforcing grpc version 1.42+.

@codyoss I have updated the PR, PTAL. Basically changes are:

  1. Remove the grpc version requirements;
  2. Add a grpc version guard before using google-c2p:/// URI scheme;
  3. Move the XDS register to a new file dial_enablexds.go, which is guarded by the tag enablexds.

I believe in this way CBT probers and E2E tests over Traffic Director can be unblocked. For example, we can do bigtable tests like this:

git clone https://github.com/googleapis/google-cloud-go.git
cd google-cloud-go/bigtable
go get -u google.golang.org/grpc
go test -tags enablexds ...

go.sum Show resolved Hide resolved
transport/grpc/dial.go Show resolved Hide resolved
@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Oct 28, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit aa0f0be into googleapis:master Oct 28, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 28, 2021
@tamird
Copy link

tamird commented Nov 3, 2021

The change to transport/grpc/dial_enablexds.go in this PR causes a dependency on google.golang.org/grpc/xds and through it a huge ball of dependencies. Upgrading from v0.59.0 to v0.60.0 pulls in nearly 300k LOC of dependencies. See https://gist.githubusercontent.com/tamird/56aff923e2b061546dc59bc7fec855cf/raw/4b84d173f9153d47fab555d35fa423b8e7edbf14/gistfile1.txt.

@mohanli-ml
Copy link
Contributor Author

Yes, adding xds support will explode dependencies and this size of google-api-go-client library, unfortunately. We have the same issue in other languages (for example, java googleapis/gax-java#1521). I thought the dependencies are protected by the build tag, but looks like even if these codes are not compiled, they are still pulled?

@tamird
Copy link

tamird commented Nov 3, 2021

Yes, the Go module system can't know which build tags will be used, so it pulls all possible combinations.

@tamird
Copy link

tamird commented Nov 3, 2021

Filed #1283.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants