Skip to content

Conversation

@dfawley
Copy link
Member

@dfawley dfawley commented Mar 6, 2024

Fixes #7013

RELEASE NOTES:

  • xds: fix an issue that would cause the client to send an empty list of resources for LDS/CDS upon reconnecting with the management server

@dfawley dfawley added this to the 1.63 Release milestone Mar 6, 2024
@dfawley dfawley requested a review from arvindbr8 March 6, 2024 20:56
@codecov
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #7026 (19f938a) into master (f7c5e6a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7026      +/-   ##
==========================================
+ Coverage   82.48%   82.52%   +0.04%     
==========================================
  Files         296      296              
  Lines       31468    31463       -5     
==========================================
+ Hits        25955    25964       +9     
+ Misses       4457     4447      -10     
+ Partials     1056     1052       -4     
Files Coverage Δ
xds/internal/xdsclient/transport/transport.go 96.49% <100.00%> (+4.96%) ⬆️

... and 16 files with indirect coverage changes

@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Mar 6, 2024
@atollena
Copy link
Collaborator

atollena commented Mar 7, 2024

I'm not familiar with this code, but I'm wondering whether beside never sending empty subscriptions on new ADS streams, as done in this PR, why the xDS resolver itself is not closed (and hence the management server connection, too), when all channels using it are idle or shutdown.

@dfawley
Copy link
Member Author

dfawley commented Mar 7, 2024

I'm not familiar with this code, but I'm wondering whether beside never sending empty subscriptions on new ADS streams, as done in this PR, why the xDS resolver itself is not closed (and hence the management server connection, too), when all channels using it are idle or shutdown.

I think that would be a separate issue. The xdsClient is a global singleton shared between all channels. In theory when the last reference to it goes away, it should shut down, according to:

singletonClient.clientImpl.close()

Are you seeing evidence that the xdsClient and/or its connection to the management server are not being closed when the last consumer goes away?

This change is needed regardless of that to prevent empty resources from being requested for any type.

@dfawley dfawley merged commit 55341d7 into grpc:master Mar 7, 2024
@dfawley dfawley deleted the xdsemptyinitreq branch March 7, 2024 17:15
@atollena
Copy link
Collaborator

atollena commented Mar 7, 2024

Are you seeing evidence that the xdsClient and/or its connection to the management server are not being closed when the last consumer goes away?

I thought so, but let me check again. I'll open another issue if needed.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xds: ADS stream failure triggers wildcard subscriptions on new stream

3 participants