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

xds: change the DumpResources API to return proto message containing the resource dump #7240

Merged
merged 2 commits into from
May 22, 2024

Conversation

easwars
Copy link
Contributor

@easwars easwars commented May 17, 2024

Addresses #6898

This PR does not cover all changes required to support A71 in CSDS. It changes the DumpResources API on the XDSclient interface to return the actual proto message for the CSDS response. The eventual goal is as follows:

  • remove the DumpResources method from the XDSClient interface
  • unexport the DumpResources method on the clientImpl type which is the implementation of the XDSClient interface
  • have a package level function xdsclient.DumpResources() with returns the CSDS response proto
    • this function would go through the list of available xDS clients, and invoke their dumpResources() method and collate all the responses and return one
  • CSDS service implementation would call the above package level function

I'm currently blocked on the last two steps because our current state of things with the global singleton and global xDS clients for testing seems to be mess. I need to clean that up quite a bit before I can get to the last two steps here.

#a71-xds-fallback

RELEASE NOTES: none

@easwars easwars requested a review from dfawley May 17, 2024 22:27
@easwars easwars added the Type: Feature New features or improvements in behavior label May 17, 2024
@easwars easwars added this to the 1.65 Release milestone May 17, 2024
protocmp.IgnoreFields((*v3statuspb.ClientConfig_GenericXdsConfig)(nil), "last_updated"),
protocmp.IgnoreFields((*v3adminpb.UpdateFailureState)(nil), "last_update_attempt", "details"),
// cmpopts.EquateEmpty(),
cmp.Comparer(func(a, b time.Time) bool { return true }),
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just ignore the fields instead? Or is there no other way to ignore by type of field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we can get rid of all the other cmp opts, other than the ones to ignore fields.

@dfawley dfawley assigned easwars and unassigned dfawley May 21, 2024
@easwars easwars merged commit a75dfa6 into grpc:master May 22, 2024
12 checks passed
@easwars easwars deleted the xds_fallback_csds_changes branch May 22, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants