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: store server config for LRS server in xdsresource.ClusterUpdate #7191

Merged
merged 2 commits into from
May 8, 2024

Conversation

easwars
Copy link
Contributor

@easwars easwars commented May 6, 2024

Summary of changes:

  • Add server config to the xdsresource.DecodeOptions struct. This field will contain the configuration of the xDS server from where the resource was received.
    • The authority struct inside the xdsclient sets up this field when it receives a Cluster resource and passes it on to the data model layer for parsing and decoding.
  • Store the server config of the LRS server in the xdsresource.ClusterUpdate struct, instead of an enum representing whether LRS is ON.
    • The data model layer populates this field if it sees LRS being enabled in the received Cluster resource.
  • Change CDS LB policy to simply copy the LRS server config from the ClusterUpdate struct to the child policy config.
  • Update tests.
  • Remove a TODO which has already been taken care of.

#a71-xds-fallback

Fixes #6896

RELEASE NOTES: none

@easwars easwars requested a review from dfawley May 6, 2024 17:44
@easwars easwars added the Type: Feature New features or improvements in behavior label May 6, 2024
@easwars easwars added this to the 1.64 Release milestone May 6, 2024
@easwars easwars force-pushed the a47_leftovers_server_config branch from 1abd1cd to 75dd0cf Compare May 6, 2024 18:18
// authority.
dm.LoadReportingServer = bootstrapConfig.XDSServer
}
LoadReportingServer: cluster.LRSServerConfig,
Copy link
Member

Choose a reason for hiding this comment

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

IIRC we're still missing something because we aren't supporting LRS for aggregate clusters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #7192, we currently don't support LRS for non-EDS clusters, and according to Mark, that is a bug. And with regards to aggregate clusters, Mark mentioned that the expected behavior is specified in A75, but Go hasn't implemented A74 and A75 yet.

So, in this PR, I didn't want to add LRS support for non-EDS clusters. I will do that as part of #7192.

@dfawley dfawley assigned easwars and unassigned dfawley May 7, 2024
Copy link
Contributor Author

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

// authority.
dm.LoadReportingServer = bootstrapConfig.XDSServer
}
LoadReportingServer: cluster.LRSServerConfig,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #7192, we currently don't support LRS for non-EDS clusters, and according to Mark, that is a bug. And with regards to aggregate clusters, Mark mentioned that the expected behavior is specified in A75, but Go hasn't implemented A74 and A75 yet.

So, in this PR, I didn't want to add LRS support for non-EDS clusters. I will do that as part of #7192.

@easwars easwars merged commit 5d24ee2 into grpc:master May 8, 2024
12 checks passed
@easwars easwars deleted the a47_leftovers_server_config branch May 8, 2024 16:35
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.

xdsclient: store server config for LRS server in xdsresource.ClusterUpdate
2 participants