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: add google-c2p dependence to DirectPath #1521

Merged
merged 11 commits into from Oct 24, 2021

Conversation

mohanli-ml
Copy link
Contributor

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

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

  1. Add the google-c2p resolver dependence to gax-java;
  2. Add an environment variable GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS to enable using DirectPath over TD;

@mohanli-ml mohanli-ml requested review from as code owners Oct 14, 2021
@google-cla google-cla bot added the cla: yes label Oct 14, 2021
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Could you add tests?

@mohanli-ml
Copy link
Contributor Author

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

I am a bit confused about how to write unit test here, as I need to first set the environment variable. After some searching I find it seems not easy to do that in Java. Do you have suggestions here? Or do you think if it is OK that we ignore the unit tests? I have done integration tests before submitting the PR, so the functionalities have been tested.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Oct 18, 2021

I am a bit confused about how to write unit test here, as I need to first set the environment variable.

Typically this is done through "dependency injection." In fact, the current could have been retrieving directly system environment variables with Boolean.get(), but it's getting the variables through the envProvider instance. What you can do is to pass a custom EnvironmentProvider. For that to work, I see you will need to add a package-private (shouldn't be public) setEnvironmentProvider() to the Builder class, and call the setter while building an instance in the corresponding ...Test.java. You will see examples of creating an instance through the builder.

I'll take care of the merge conflict in build.gradle.

@@ -18,7 +18,8 @@ dependencies {
libraries['maven.io_grpc_grpc_auth'],
libraries['maven.io_grpc_grpc_netty_shaded'],
libraries['maven.io_grpc_grpc_protobuf'],
libraries['maven.io_grpc_grpc_stub'])
libraries['maven.io_grpc_grpc_stub'],
libraries['maven.io_grpc_grpc_xds'])
Copy link
Contributor

@dapengzhang0 dapengzhang0 Oct 18, 2021

Choose a reason for hiding this comment

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

cc @ejona86 will adding grpc-xds explode dependencies and the size of gax-grpc library?

Copy link

@ejona86 ejona86 Oct 18, 2021

Choose a reason for hiding this comment

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

Yes, but that is the design. With added risk, it would be possible to delay adding this as a dependency until users are expected to actually use it. But this would eventually need to happen.

With some effort, the C2P-enabled services could add this dependency instead of gax. So only those services using it would pay the price.

Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

Maybe we should not add this per my comment above?

Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

Per my new comment, I think this should be a runtime classpath. Note that you added this to implementation on line 15. Assuming that it's a runtime dependency, you need to do

runtimeOnly libraries['maven.io_grpc_grpc_xds']

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 21, 2021

Choose a reason for hiding this comment

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

done

builder = ComputeEngineChannelBuilder.forTarget("google-c2p:///" + serviceAddress);
} else {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled.
Copy link

@ejona86 ejona86 Oct 18, 2021

Choose a reason for hiding this comment

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

Seems keepalive should be for google-c2p as well.

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 18, 2021

Choose a reason for hiding this comment

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

done

@@ -18,7 +18,8 @@ dependencies {
libraries['maven.io_grpc_grpc_auth'],
libraries['maven.io_grpc_grpc_netty_shaded'],
libraries['maven.io_grpc_grpc_protobuf'],
libraries['maven.io_grpc_grpc_stub'])
libraries['maven.io_grpc_grpc_stub'],
libraries['maven.io_grpc_grpc_xds'])
Copy link

@ejona86 ejona86 Oct 18, 2021

Choose a reason for hiding this comment

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

Yes, but that is the design. With added risk, it would be possible to delay adding this as a dependency until users are expected to actually use it. But this would eventually need to happen.

With some effort, the C2P-enabled services could add this dependency instead of gax. So only those services using it would pay the price.

@mohanli-ml
Copy link
Contributor Author

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

Typically this is done through "dependency injection." In fact, the current could have been retrieving directly system environment variables with Boolean.get(), but it's getting the variables through the envProvider instance. What you can do is to pass a custom EnvironmentProvider. For that to work, I see you will need to add a package-private (shouldn't be public) setEnvironmentProvider() to the Builder class, and call the setter while building an instance in the corresponding ...Test.java. You will see examples of creating an instance through the builder.

Thanks for the explanation! I have understood your method and been able to create a unit test, where I when DirectPath XDS is enabled, a URI scheme google-c2p:/// will be added and port number will be removed. However, as I do not have a way to retrieve and check the endpoint passed to a gRPC channel builder, I could not use this method https://github.com/googleapis/gax-java/blob/main/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java#L272 (@dapengzhang0
for help here). Instead, I added a new field activeEndpoint to represent the actual endpoint that is used to build a gRPC channel. PTAL.

Also notice that in my manual test it failed because ava.lang.IllegalArgumentException: cannot find a NameResolver for google-c2p:///localhost, which I believe is because current gax-java does not have the XDS dependency.

@@ -40,6 +40,7 @@ ext {
'maven.io_grpc_grpc_protobuf': "io.grpc:grpc-protobuf:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_netty_shaded': "io.grpc:grpc-netty-shaded:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_alts': "io.grpc:grpc-alts:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_xds': "io.grpc:grpc-xds:${libraries['version.io_grpc']}",
Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

I just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 20, 2021

Choose a reason for hiding this comment

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

I just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

Can you give me an example about how to add the dependency to dependencies.properties? And should I also remove change in both gax-grpc/build.gradle?

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

Compilation can succeed because as you said, no new class is added. However, this dependency is actually registering google-c2p resolver to the build binary, otherwise gRPC can not recognize the google-c2p:/// URI scheme and we will get error like Java.lang.IllegalArgumentException: cannot find a NameResolver for google-c2p:///localhost. So we should keep the dependency.

Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

I see. What you are saying sounds like the dependency is required to exist in the runtime classpath of the user consuming the gax-java library.

Since it's not required for compiling the project, let's have this as a Gradle-only dependency. I'm pretty new to this repo, so I don't know whether the Bazel build will also need this for something. However, gax-java libraries are released from Gradle, at least they will be okay. If there comes an issue on the Bazel side later, we'll take care of it later.

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 20, 2021

Choose a reason for hiding this comment

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

Sure, thanks!

@@ -18,7 +18,8 @@ dependencies {
libraries['maven.io_grpc_grpc_auth'],
libraries['maven.io_grpc_grpc_netty_shaded'],
libraries['maven.io_grpc_grpc_protobuf'],
libraries['maven.io_grpc_grpc_stub'])
libraries['maven.io_grpc_grpc_stub'],
libraries['maven.io_grpc_grpc_xds'])
Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

Maybe we should not add this per my comment above?

@@ -390,6 +407,29 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
provider.getTransportChannel().shutdownNow();
}

@Test
public void testWithDirectPathXds() throws IOException {
Copy link
Contributor

@chanseokoh chanseokoh Oct 20, 2021

Choose a reason for hiding this comment

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

Thanks for trying. I saw this test is failing, and I think I know why. In order to test the section of the interest, we should satisfy the following constraints:

    if (isDirectPathEnabled(serviceAddress)
        && isNonDefaultServiceAccountAllowed()
        && isOnComputeEngine()) {

The problem is that, isOnComputeEngine() is not controllable. Certainly, this class is not very testable. For making this testable, we should further refactor the class so that the information about whether it's on GCE is given from another provider (like an env provider).

But at this point, I don't want to make your life too difficult. Let's forget adding this and the setEnvProvider/activeEndpoint stuff to maintain simple code. Sorry, I appreciate your time you spent to explore this!

Copy link
Contributor Author

@mohanli-ml mohanli-ml Oct 20, 2021

Choose a reason for hiding this comment

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

Ah, thanks! I will remove the unit test then.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Oct 20, 2021

I pushed some changes, so be sure to pull before working on this.

@mohanli-ml
Copy link
Contributor Author

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

@chanseokoh Hey Chanseok, can you also help to merge this PR? Sorry I do not have the access. Thanks!

@chanseokoh chanseokoh merged commit d4222e7 into googleapis:main Oct 24, 2021
5 checks passed
@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Oct 24, 2021

Sure, just merged it.

gcf-merge-on-green bot pushed a commit to googleapis/google-api-go-client that referenced this issue Oct 28, 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: googleapis/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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants