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.

Sign up for GitHub

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

Jump to bottom

Dualstack interop testing enablement #11231

Merged
merged 28 commits into from
Jun 27, 2024
Merged

Dualstack interop testing enablement #11231

merged 28 commits into from
Jun 27, 2024

Conversation

larry-safran
Copy link
Contributor

@larry-safran larry-safran commented May 23, 2024 •
edited
Loading

Fix flag string for enabling dual stack and give server ability to parse argument address_type

@larry-safran larry-safran requested review from ejona86 and removed request for sergiitk June 5, 2024 00:44
ejona86
ejona86 reviewed Jun 5, 2024
View reviewed changes
interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java Outdated
@@ -268,7 +267,7 @@ void setUp() {
tester.setUp();
}

private synchronized void tearDown() {
synchronized void tearDown() {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this file changed? I don't see any usages of these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes reverted

interop-testing/src/main/java/io/grpc/testing/integration/Util.java Outdated
InetAddress[] addresses = InetAddress.getAllByName(InetAddress.getLocalHost().getHostName());
for (InetAddress address : addresses) {
if (address.getAddress().length == 4) {
return new java.net.InetSocketAddress(address.getHostAddress(), port);
Copy link
Member

Choose a reason for hiding this comment

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

Pass address, like you did for IPv6. This will do a DNS lookup and can leave you with an IPV6 address.

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

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceServer.java Show resolved Hide resolved
interop-testing/src/main/java/io/grpc/testing/integration/XdsTestServer.java Show resolved Hide resolved
interop-testing/src/test/java/io/grpc/testing/integration/StressTestClientTest.java Outdated Show resolved Hide resolved
@larry-safran larry-safran mentioned this pull request Jun 6, 2024
Closed
@larry-safran
Copy link
Contributor Author

@ejona86 ping

ejona86
ejona86 reviewed Jun 26, 2024
View reviewed changes
interop-testing/src/main/java/io/grpc/testing/integration/XdsTestServer.java Outdated Show resolved Hide resolved
interop-testing/src/main/java/io/grpc/testing/integration/XdsTestServer.java Show resolved Hide resolved
xds/src/main/java/io/grpc/xds/XdsEndpointResource.java Outdated
@@ -101,7 +100,8 @@ protected EdsUpdate doParse(Args args, Message unpackedMessage) throws ResourceI
}

private static boolean isEnabledXdsDualStack() {
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
return GrpcUtil.getFlag(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
Copy link
Member

Choose a reason for hiding this comment

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

We will need to be careful here to not flip the flag before it is passing interop. We can flip the happy eyeballs flag without interop passing, but we shouldn't flip this one.

interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated Show resolved Hide resolved
interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated Show resolved Hide resolved
interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated
public final GrpcCleanupRule cleanupRule = new GrpcCleanupRule();

@Rule
public final Timeout globalTimeout = Timeout.seconds(10);
Copy link
Member

Choose a reason for hiding this comment

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

This will leak the server. Why not just use a timeout on the unary RPC?

interop-testing/src/main/java/io/grpc/testing/integration/Util.java Outdated Show resolved Hide resolved
interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated Show resolved Hide resolved
interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated
}
}

private static ManagedChannel createChannel(int port, String target) {
Copy link
Member

Choose a reason for hiding this comment

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

throws Exception to remove the unhelpful catch+rethrow?

interop-testing/src/test/java/io/grpc/testing/integration/DualStackClientTest.java Outdated Show resolved Hide resolved
copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Jun 26, 2024
@larry-safran
Add --address_type to interop and xds test descriptions (#36830)
3caa004 
This documents the changes the the test server classes in grpc/grpc-java#11231

Closes #36830

COPYBARA_INTEGRATE_REVIEW=#36830 from larry-safran:dualstack_test_doc 986e9dd
PiperOrigin-RevId: 647054757
@larry-safran
Respond to reviewer comments including splitting enabling Happy Eyeba…
084e375 
…lls from flag guarding xds dual stack processing.
sergiitk
sergiitk requested changes Jun 27, 2024
View reviewed changes
Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

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

Before merging, let's confirm PSM Interop passes with the changes to the server.

interop-testing/src/main/java/io/grpc/testing/integration/StressTestClient.java Outdated
Comment on lines 311 to 313
metricsServer.shutdownNow();
if (metricsServer != null) {
metricsServer.shutdownNow();
}
Copy link
Member

Choose a reason for hiding this comment

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

Since this is unrelated to the change, could we please move it to a separate PR? To avoid any confusion, and in case we need to roll back this PR.

interop-testing/src/test/java/io/grpc/testing/integration/XdsTestServerTest.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java
@@ -33,15 +33,15 @@
* down the address list and sticks to the first that works.
*/
public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider {
public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
"GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS";
public static final String GRPC_PF_USE_HAPPY_EYEBALLS = "GRPC_PF_USE_HAPPY_EYEBALLS";
Copy link
Member

Choose a reason for hiding this comment

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

@ejona86 - does it make sense to separate PSM Interop server changes from the dualstack enablement env vars? I'd prefer that, but if not possible/desirable, I'm good with this too.

@sergiitk
Copy link
Member

sergiitk commented Jun 27, 2024 •
edited
Loading

ejona86
ejona86 reviewed Jun 27, 2024
View reviewed changes
xds/src/main/java/io/grpc/xds/XdsEndpointResource.java Outdated
"grpc.experimental.xdsDualstackEndpoints";

public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
"GRPC_EXPERIMENTAL_HAPPY_EYEBALLS";
Copy link
Member

Choose a reason for hiding this comment

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

HAPPY_EYEBALLS? This should be "GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", per the gRFC, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@larry-safran larry-safran merged commit 7a53fa8 into grpc:master Jun 27, 2024
15 checks passed
@larry-safran larry-safran deleted the dualstack_server_flags branch June 27, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejona86 ejona86 ejona86 approved these changes

@sergiitk sergiitk sergiitk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@larry-safran @sergiitk @ejona86