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

xds: Preserve nonce when unsubscribing type #11796

Merged
merged 2 commits into from
Jan 3, 2025
Merged

xds: Preserve nonce when unsubscribing type #11796

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6405a3
xds: Preserve nonce when unsubscribing type
ejona86 Dec 28, 2024
e562d93
Combine except for nonces
ejona86 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,11 @@ void adjustResourceSubscription(XdsResourceType<?> resourceType) {
resourceStore.startMissingResourceTimers(resources, resourceType);

if (resources.isEmpty()) {
// The resource type no longer has subscribing resources; clean up references to it
// The resource type no longer has subscribing resources; clean up references to it.
// Except for nonces. If the resource type becomes used again the control plane can ignore
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
// requests for old/missing nonces. Old type's nonces are dropped when the ADS stream is
// restarted.
versions.remove(resourceType);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should retain the version also. The version doesn't change when we change the set of resources that we're subscribed to, and unsubscribing from all resources is just a special case of changing the set of resources that we're subscribed to.

Copy link
Member Author

@ejona86 ejona86 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We had discussed this offline and Mark prefers keeping the version but doesn't see it as mandatory. We (grpc-java) want to drop our resource data structures, and that means the version is also lost. The version serves no purpose when there are no resources saved by the client, so it can't make any difference which way it is done.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it probably won't cause any problems to drop it. But I do worry about potential edge cases we might hit in the future.

I guess let's kick the can down the road here and see if we have problems.

adsStream.respNonces.remove(resourceType);
}
}

Expand Down Expand Up @@ -313,7 +315,10 @@ private class AdsStream implements XdsTransportFactory.EventHandler<DiscoveryRes
// Nonce in each response is echoed back in the following ACK/NACK request. It is
// used for management server to identify which response the client is ACKing/NACking.
// To avoid confusion, client-initiated requests will always use the nonce in
// most recently received responses of each resource type.
// most recently received responses of each resource type. Nonces are never deleted from the
// map; nonces are only discarded once the stream closes because xds_protocol says "the
// management server should not send a DiscoveryResponse for any DiscoveryRequest that has a
// stale nonce."
private final Map<XdsResourceType<?>, String> respNonces = new HashMap<>();
private final StreamingCall<DiscoveryRequest, DiscoveryResponse> call;
private final MethodDescriptor<DiscoveryRequest, DiscoveryResponse> methodDescriptor =
Expand Down
5 changes: 2 additions & 3 deletions xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2899,10 +2899,9 @@ public void edsCleanupNonceAfterUnsubscription() {
verifySubscribedResourcesMetadataSizes(0, 0, 0, 0);
call.verifyRequest(EDS, Arrays.asList(), VERSION_1, "0000", NODE);

// When re-subscribing, the version and nonce were properly forgotten, so the request is the
// same as the initial request
// When re-subscribing, the version was forgotten but not the nonce
xdsClient.watchXdsResource(XdsEndpointResource.getInstance(), "A.1", edsResourceWatcher);
call.verifyRequest(EDS, "A.1", "", "", NODE, Mockito.timeout(2000).times(2));
call.verifyRequest(EDS, "A.1", "", "0000", NODE, Mockito.timeout(2000));
}

@Test
Expand Down
Loading