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.

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

xdsclient: correct logic used to suppress empty ADS requests on new streams #7026

Merged
merged 2 commits into from Mar 7, 2024

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
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
14 checks passed
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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