Skip to content

xds: xdsClient caches transient error for new watchers #12262

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

Merged
shivaspeaks merged 5 commits into from
Aug 19, 2025
Merged

xds: xdsClient caches transient error for new watchers #12262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
27fe7a4
xds: xdsClient caches transient error for new watchers
shivaspeaks Aug 7, 2025
033a4b1
address comments
shivaspeaks Aug 8, 2025
7f3a44c
unit test
shivaspeaks Aug 19, 2025
3fe9f54
fix unit test
shivaspeaks Aug 19, 2025
4ffdc2e
ughhh
shivaspeaks Aug 19, 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
10 changes: 10 additions & 0 deletions xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,8 @@ private final class ResourceSubscriber<T extends ResourceUpdate> {
private ResourceMetadata metadata;
@Nullable
private String errorDescription;
@Nullable
private Status lastError;
Copy link
Member

@ejona86 ejona86 Aug 8, 2025

Choose a reason for hiding this comment

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

FYI: Change data to StatusOr<T> data would have kept this a bit more clear. You still have data vs absent, but at least you don't have yet another thing to deal with.

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 8, 2025

Choose a reason for hiding this comment

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

I'm concerned that it's a more invasive refactoring change since it would require touching every method that uses the data field. I can try to do it in a follow up PR and keep this PR's scope limited to bug fix?

I'll agree that we are moving to more usage of StatusOr<T> in our codebase everywhere. We should consider here as well for consistency.

Copy link
Member

@ejona86 ejona86 Aug 8, 2025

Choose a reason for hiding this comment

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

There's only a few other places that access data. It does look like it'd make all of them more complicated. Most are of the form data != null, and would likely need to become data != null && data.hasValue().

Copy link
Contributor

@kannanjgithub kannanjgithub Aug 18, 2025

Choose a reason for hiding this comment

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

Unit tests?

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 18, 2025

Choose a reason for hiding this comment

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

Unit tests are needed for this? I don't know how I replicate this behaviour. The logic sounds well, we should probably let it merge and see?

Copy link
Contributor

@kannanjgithub kannanjgithub Aug 19, 2025

Choose a reason for hiding this comment

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

Unit tests prevent breakages in the future from new code changes.
There are many tests that test onData and onError and testing watcher calls.

Copy link
Member

@ejona86 ejona86 Aug 19, 2025

Choose a reason for hiding this comment

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

A unit test shouldn't be too hard. We find a test that is triggering onError(), and then after getting the onError() add a second subscription.

So as "the first one I noticed" there's ldsResourceUpdated_withXdstpResourceName_withUnknownAuthority().

grpc-java/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java

Lines 680 to 695 in 6462ef9

@Test
public void ldsResourceUpdated_withXdstpResourceName_withUnknownAuthority() {
String ldsResourceName =
"xdstp://unknown.example.com/envoy.config.listener.v3.Listener/listener1";
xdsClient.watchXdsResource(XdsListenerResource.getInstance(), ldsResourceName,
ldsResourceWatcher);
verify(ldsResourceWatcher).onError(errorCaptor.capture());
Status error = errorCaptor.getValue();
assertThat(error.getCode()).isEqualTo(Code.INVALID_ARGUMENT);
assertThat(error.getDescription()).isEqualTo(
"Wrong configuration: xds server does not exist for resource " + ldsResourceName);
assertThat(resourceDiscoveryCalls.poll()).isNull();
xdsClient.cancelXdsResourceWatch(XdsListenerResource.getInstance(), ldsResourceName,
ldsResourceWatcher);
assertThat(resourceDiscoveryCalls.poll()).isNull();
}

You could create a ldsResourceWatcher2 and subscribe to the same resource after the verify(ldsResourceWatcher).onError(errorCaptor.capture());, and expect verify(ldsResourceWatcher2).onError(any());. You can also delete unimportant parts of the test, like checking the status description. You might want to verify that the second watcher gets the same Status, although that could even be optional.

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not confident with this unit test since we are not checking the instance to be same. It seems to be getting caught in errorDescription != null(code link) for both the watchers and not checking the savedError.

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This PR introduces lastError, so there's no other test right now for setting lastError.
I tried moving check for savedError before errorDescription but looks like lastError is not getting set.

Copy link
Contributor

@kannanjgithub kannanjgithub Aug 19, 2025

Choose a reason for hiding this comment

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

Your PR introduces lastError but the existing control paths in other tests do go there and set it. Just set breakpoint and run all tests in that test class and you will see.

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 19, 2025

Choose a reason for hiding this comment

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

Looks like none of the tests are stopping at that break point from GrpcXdsClientImplTestBase class.

Copy link
Contributor

@kannanjgithub kannanjgithub Aug 19, 2025

Choose a reason for hiding this comment

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

Screenshot 2025-08-19 6 54 21 PM

Copy link
Member Author

@shivaspeaks shivaspeaks Aug 19, 2025

Choose a reason for hiding this comment

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

Oh I was keeping the break point inside onError where savedError was getting defined. My bad. I made a new commit. PTAL.


ResourceSubscriber(XdsResourceType<T> type, String resource) {
syncContext.throwIfNotInThisSynchronizationContext();
Expand Down Expand Up @@ -712,11 +714,16 @@ void addWatcher(ResourceWatcher<T> watcher, Executor watcherExecutor) {
watchers.put(watcher, watcherExecutor);
T savedData = data;
boolean savedAbsent = absent;
Status savedError = lastError;
watcherExecutor.execute(() -> {
if (errorDescription != null) {
watcher.onError(Status.INVALID_ARGUMENT.withDescription(errorDescription));
return;
}
if (savedError != null) {
watcher.onError(savedError);
return;
}
if (savedData != null) {
notifyWatcher(watcher, savedData);
} else if (savedAbsent) {
Expand Down Expand Up @@ -808,6 +815,7 @@ void onData(ParsedResource<T> parsedResource, String version, long updateTime,
this.metadata = ResourceMetadata
.newResourceMetadataAcked(parsedResource.getRawResource(), version, updateTime);
absent = false;
lastError = null;
if (resourceDeletionIgnored) {
logger.log(XdsLogLevel.FORCE_INFO, "xds server {0}: server returned new version "
+ "of resource for which we previously ignored a deletion: type {1} name {2}",
Expand Down Expand Up @@ -857,6 +865,7 @@ void onAbsent(@Nullable ProcessingTracker processingTracker, ServerInfo serverIn
if (!absent) {
data = null;
absent = true;
lastError = null;
metadata = serverInfo.resourceTimerIsTransientError()
? ResourceMetadata.newResourceMetadataTimeout()
: ResourceMetadata.newResourceMetadataDoesNotExist();
Expand Down Expand Up @@ -894,6 +903,7 @@ void onError(Status error, @Nullable ProcessingTracker tracker) {
Status errorAugmented = Status.fromCode(error.getCode())
.withDescription(description + "nodeID: " + bootstrapInfo.node().getId())
.withCause(error.getCause());
this.lastError = errorAugmented;

for (ResourceWatcher<T> watcher : watchers.keySet()) {
if (tracker != null) {
Expand Down
20 changes: 20 additions & 0 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 @@ -285,6 +285,8 @@ public long currentTimeNanos() {
@Mock
private ResourceWatcher<LdsUpdate> ldsResourceWatcher;
@Mock
private ResourceWatcher<LdsUpdate> ldsResourceWatcher2;
@Mock
private ResourceWatcher<RdsUpdate> rdsResourceWatcher;
@Mock
private ResourceWatcher<CdsUpdate> cdsResourceWatcher;
Expand Down Expand Up @@ -694,6 +696,24 @@ public void ldsResourceUpdated_withXdstpResourceName_withUnknownAuthority() {
assertThat(resourceDiscoveryCalls.poll()).isNull();
}

@Test
public void ldsResource_onError_cachedForNewWatcher() {
xdsClient.watchXdsResource(XdsListenerResource.getInstance(), LDS_RESOURCE,
ldsResourceWatcher);
DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
call.sendCompleted();
verify(ldsResourceWatcher).onError(errorCaptor.capture());
Status initialError = errorCaptor.getValue();
xdsClient.watchXdsResource(XdsListenerResource.getInstance(), LDS_RESOURCE,
ldsResourceWatcher2);
ArgumentCaptor<Status> secondErrorCaptor = ArgumentCaptor.forClass(Status.class);
verify(ldsResourceWatcher2).onError(secondErrorCaptor.capture());
Status cachedError = secondErrorCaptor.getValue();

assertThat(cachedError).isEqualTo(initialError);
assertThat(resourceDiscoveryCalls.poll()).isNull();
}

@Test
public void ldsResponseErrorHandling_allResourcesFailedUnpack() {
DiscoveryRpcCall call = startResourceWatcher(XdsListenerResource.getInstance(), LDS_RESOURCE,
Expand Down