-
Notifications
You must be signed in to change notification settings - Fork 5.1k
upstream: implement Cluster's load_assignment field #3864
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
Conversation
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 |
There was a problem hiding this comment.
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>` |
There was a problem hiding this comment.
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>` |
There was a problem hiding this comment.
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove newline
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Thanks, @danielhochman. Updated in 18e98f4. |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Since this is a re-submitted PR of #3261, probably this requires @mattklein123 @alyssawilk @htuch and @zuercher help to check too. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: 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 {}", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
Description:
This patch implements
load_assigment
field in CDS'Cluster
.This change specifically adds the implementation of the new
load_assigment
fieldfor clusters with discovery-type:
STATIC
,STRICT_DNS
andLOGICAL_DNS
.While adding this
load_assigment
field implementation toCluster
,this patch also allows specifying optional (active) health check config per specified upstream host.
Risk Level: medium
Testing: unit tests
Docs Changes:
Release Notes: N/A
Fixes #439
Signed-off-by: Dhi Aurrahman dio@rockybars.com