Skip to content

Conversation

dio
Copy link
Member

@dio dio commented Jul 16, 2018

Description:
This patch implements load_assigment field in CDS' Cluster.
This change specifically adds the implementation of the new load_assigment field
for clusters with discovery-type: STATIC, STRICT_DNS and LOGICAL_DNS.

While adding this load_assigment field implementation to Cluster,
this patch also allows specifying optional (active) health check config per specified upstream host.

Risk Level: medium
Testing: unit tests
Docs Changes:

  • This unhides docs for endpoint health check config

Release Notes: N/A

Fixes #439

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

@dio
Copy link
Member Author

dio commented Jul 16, 2018

cc. @danielhochman

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@@ -544,10 +565,10 @@ Management Server Unreachability
--------------------------------

When Envoy instance looses connectivity with the management server, Envoy will latch on to
Copy link
Contributor

Choose a reason for hiding this comment

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

while you're here, do you mind s/looses/loses/


If active health checking is configured for an upstream cluster, a specific additional configuration
for each registered member can be specified by setting the
:ref:`health check config<envoy_api_msg_endpoint.Endpoint.HealthCheckConfig>`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we keep the CamelCase for HealthCheckConfig and Endpoint? or at least make it consistent for each of the linked resources in this paragraph.

If active health checking is configured for an upstream cluster, a specific additional configuration
for each registered member can be specified by setting the
:ref:`health check config<envoy_api_msg_endpoint.Endpoint.HealthCheckConfig>`
in :ref:`endpoint message<envoy_api_msg_endpoint.Endpoint>`
Copy link
Contributor

Choose a reason for hiding this comment

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

slight rewording, add the after in

- endpoint:
health_check_config:
port_value: 8080

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove newline

dio added 2 commits July 19, 2018 04:09
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Jul 18, 2018

Thanks, @danielhochman. Updated in 18e98f4.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Jul 19, 2018

Since this is a re-submitted PR of #3261, probably this requires @mattklein123 @alyssawilk @htuch and @zuercher help to check too.

Copy link
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.

Couple minor things from me, but otherwise looks good!

When Envoy instance looses connectivity with the management server, Envoy will latch on to
the previous configuration while actively retrying in the background to reestablish the
connection with the management server.
When Envoy instance loses connectivity with the management server, Envoy will latch on to
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, it should read "When an Envoy instance loses..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @zuercher! This is addressed in 75a1537.

: Config::Utility::translateClusterHosts(cluster.hosts())) {
const auto& locality_lb_endpoints = load_assignment_.endpoints();
if (locality_lb_endpoints.size() != 1 || locality_lb_endpoints[0].lb_endpoints().size() != 1) {
throw EnvoyException(fmt::format("LOGICAL_DNS clusters must have {}",
Copy link
Member

Choose a reason for hiding this comment

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

nit: just if (cluster.has_load_assignment()) { /* throw exception message1 */ } else { /* throw exception message 2 */ }

(it'll be easier to search for the error string, but I don't feel strongly)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 updated 75a1537.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

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