Skip to content

Add load_assignment field in Cluster#3504

Merged
htuch merged 4 commits intoenvoyproxy:masterfrom
dio:cluster-api-load-assignment
Jun 6, 2018
Merged

Add load_assignment field in Cluster#3504
htuch merged 4 commits intoenvoyproxy:masterfrom
dio:cluster-api-load-assignment

Conversation

@dio
Copy link
Copy Markdown
Member

@dio dio commented May 30, 2018

Add load_assignment field in Cluster

This patch introduces load_assigment field in CDS' Cluster. This is an API change only.
This is part of effort on breaking #3261 into multiple PRs.

Risk Level:

  • Low, since it is hidden.

Testing:

  • Build api and envoy-static without error

Docs Changes:

  • Add load_assignment in Cluster of cds.proto.

Signed-off-by: Dhi Aurrahman dio@rockybars.com

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
// Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
//
// [#not-implemented-hide:]
ClusterLoadAssignment load_assignment = 33;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the discussion at https://github.com/envoyproxy/envoy/pull/3261/files#r188243657 still applies.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@zuercher thanks, will update it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated in 0001298.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
zuercher
zuercher previously approved these changes Jun 5, 2018
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

LGTM. I added #3553 to track the other part of the comments re: custom resolvers.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, small comment nit.

//
// Setting this allows CDS static/DNS assignments to contain embedded EDS equivalent
// :ref:`endpoint assignments<envoy_api_msg_ClusterLoadAssignment>`.
// Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dio can you make a note to deprecate the hosts field and add to deprecated.md when you do the PR implementing this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, 2ab4f13. Thanks for reminding me.

//
// .. attention::
//
// Setting this allows CDS static/DNS assignments to contain embedded EDS equivalent
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"CDS static/DNS" is a bit confusing to read. Can you rephrase? Do you mean "Setting this allows non-EDS cluster types to contain..." ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated f5783e9

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@htuch htuch merged commit 79bce5f into envoyproxy:master Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants