Skip to content

health check: add optional alternative health check port #597

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.

Sign up for GitHub

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

Merged
mattklein123 merged 7 commits into from
Apr 3, 2018
Merged

health check: add optional alternative health check port #597

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d46994
Add optional alternative health check port
dio Apr 1, 2018
6023047
Review comment
dio Apr 2, 2018
88c37cd
Add health check config
dio Apr 3, 2018
b64c384
Re-index
dio Apr 3, 2018
71c5ac5
Re-index
dio Apr 3, 2018
0da7365
Rename
dio Apr 3, 2018
75662e3
Document non-zero
dio Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions envoy/api/v2/endpoint/endpoint.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,28 @@ option (gogoproto.equal_all) = true;

// Upstream host identifier.
message Endpoint {
Copy link
Member

@htuch htuch Apr 2, 2018

Choose a reason for hiding this comment

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

Do we really need each endpoint to have its own alternative health check port, or can this just be specified at cluster level?

Copy link
Member Author

@dio dio Apr 2, 2018

Choose a reason for hiding this comment

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

Some related discussions:

From above threads, I perceived that providing an alternative health check port per endpoint is preferred.

Copy link
Member

@mattklein123 mattklein123 Apr 2, 2018

Choose a reason for hiding this comment

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

I think per-host is likely required due to potentially dynamic ports on the service side. In the cluster general situation it's more verbose, but I don't think we are optimizing for lack of verbosity here so I think it's OK?

Copy link
Member

@htuch htuch Apr 2, 2018

Choose a reason for hiding this comment

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

SGTM

// The upstream host address.
core.Address address = 1;

// [#not-implemented-hide:] The optional health check configuration.
message HealthCheckConfig {
// Optional alternative health check port value.
//
// By default the health check address port of an upstream host is the same
// as the host's serving address port. This provides an alternative health
// check port. Setting this with a non-zero value allows an upstream host
// to have different health check address port.
uint32 port_value = 1;
}

// [#not-implemented-hide:] The optional health check configuration is used as
// configuration for the health checker to contact the health checked host.
//
// .. attention::
//
// This takes into effect only for upstream clusters with
// :ref:`active health checking <arch_overview_health_checking>` enabled.
HealthCheckConfig health_check_config = 2;
}

// An Endpoint that Envoy can route traffic to.
Expand Down