Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: add google-c2p dependence to DirectPath #1521

Merged
merged 11 commits into from Oct 24, 2021

Conversation

mohanli-ml
Copy link
Contributor

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 a team as code owners October 14, 2021 23:54
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2021
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Could you add tests?

@mohanli-ml
Copy link
Contributor Author

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

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

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

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

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

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

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

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

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

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 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

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

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

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

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

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

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

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

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

@mohanli-ml
Copy link
Contributor Author

@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
@chanseokoh
Copy link
Contributor

Sure, just merged it.

gcf-merge-on-green bot pushed a commit to googleapis/google-api-go-client that referenced this pull request 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
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