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

Specify health-check for each service #97

Closed
hbagdi opened this issue Feb 15, 2020 · 23 comments
Closed

Specify health-check for each service #97

hbagdi opened this issue Feb 15, 2020 · 23 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/user-story Categorizes an issue as capturing a user story lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@hbagdi
Copy link
Contributor

hbagdi commented Feb 15, 2020

What would you like to be added:

As a Service owner, when I'm exposing my service to other users/services outside the k8s cluster (or even inside), I want to define active health-checking behavior for my service.

Why is this needed:

In case an instances of a service goes unhealthy, the proxy can skip sending requests to that specific instance and instead route traffic to other instances. This is also useful during rolling upgrades where a health-check endpoint can stop responding and stop accepting connections before the pod is replaced.

/kind feature
/kind user-story

@hbagdi hbagdi added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 15, 2020
@k8s-ci-robot k8s-ci-robot added the kind/user-story Categorizes an issue as capturing a user story label Feb 15, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented Feb 15, 2020

The new API we design should take care of two things:

  1. There can be multiple routes pointing to the same service. There should be a way of de-duplicating this data or having a priority around which configuration to use in such a case.
  2. User should be asked to explicitly define health-check behavior via this API and we should refrain to re-use readiness/liveliness probes.

This was also discussed during Ingress v1beta -> v1 transition but was punted because of the scope of the change required.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented May 15, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 12, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented Sep 12, 2020

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 12, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 12, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented Dec 14, 2020

/remove-lifecycle stale
/lifecycle frozen

@hbagdi
Copy link
Contributor Author

hbagdi commented Jan 12, 2021

/remove-lifecycle stale
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 12, 2021
@robscott robscott added this to the v0.3.0 milestone Mar 24, 2021
@stevesloka
Copy link
Contributor

Would it make sense to apply these to BackendPolicies? This would fit in with allowing these to be specified once since the backendRef could be used in multiple routes. But since these are namespaced, it might not fit all use cases.

Also, backendPolicy has a tls option to specify certs for the backend making this a good place to add higher level of customizations that other users might not require if just using serviceName in a HTTPRoute (for example) making this a good spot for this addition.

We also need to think about the protocol of the service to know if we need say a path for HTTP types whereas that would be invalid for a TCP type. We could look at the type of object this is referenced from possibly to understand what types might be valid, meaning if this is referenced from a TCPRoute then path param is not required.

This idea would add a healthCheck to the backendRefs array with the following values:

  • path: Path in the upstream service to validate against (optional)
  • intervalSeconds: Number of seconds wait between health checks (optional)
  • timeoutSeconds: Number of seconds to wait for a response before giving up (optional)
  • unhealthyThresholdCount: Number of times the health checks need to fail to become unhealthy (optional)
  • healthyThresholdCount: Number of times the health checks need to succeed to become healthy (optional)

Note: These are in seconds, could switch to something more generic and allow users to specify milliseconds, etc.

Example yaml:

kind: BackendPolicy
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
  name: policy1
spec: 
  backendRefs:
  - group: core
    kind: Service
    name: svc1
    port: 80
  healthCheck:
    path: /healthz
    intervalSeconds: 5
    timeoutSeconds: 2
    unhealthyThresholdCount: 3
    healthyThresholdCount: 5

@howardjohn
Copy link
Contributor

One concern with backend policies is that its not explicitly when we should check it. This could be solvable by a comment in the spec, but we need to consider it.

For example, if I have 10 Gateways, which ones should health check policy1? is it all of them, any of them with routes to policy1, etc. If all gateways have routes to it, we will now get 10x the health check load.

The reason I care is we will have lots of gateways most likely, so want to make sure the spec is clear here

@stevesloka
Copy link
Contributor

Yeah having multiple Gateways makes it tricky, I'd expect each Gateway to do it's own Health Checks against the resource.

@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 12, 2021

Interesting. I think "who" does the health-checking is up to the implementation. You use a health-checking service or a proxy or some more complicated distributed mechanism, that is up to you and should be transparent to the end user. What matters more here is what happens when health-checks are failing and how different implementations react differently to it.

As I think more about this, it feels like BackendPolicy seems like a resource to define configuration of different types and not behavior. Making behavior consistent in this area seems infeasible.

@howardjohn
Copy link
Contributor

Another question which may be obvious but not to me - why do we need this at gateway level instead of using pod readinessProbes?

@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 12, 2021

why do we need this at gateway level instead of using pod readinessProbes?

  • Network view is sometimes different from kubelet view.
  • Pods could be ready but health-checks for traffic can be a different probe. Service readiness from gateway can go in and out of rotation more frequently than pod readiness probes.
  • I think pod probes are not strictly network-specific, users can configure a shell command/script as well.
  • The feedback loop of (a) pod failing a check (b) kubelet noticing it and then updating API-server (c) k8s core controller(s) noticing that and then updating endpoints accordingly (d) controller for Gateway/Ingress noticing the change (e) proxy hot path finally reflecting the change, can be sometimes a bit slower than desired.
  • Service-level health-check sometimes is a higher-order concept that is not specific to one pod but pods of different deployments.
  • Health-check defined this way can be seen as a way to standardize health-checking in a large environment (lot of services owned by a lot of teams)

@robscott
Copy link
Member

To follow up on this, I've written up a quick doc to try to summarize the portability of health check config across implementations. There's a spreadsheet that covers some of the most commonly supported features. I likely got at least some things wrong here, so please correct me if I did.

At a high level, most implementations support HTTP health checks with the option to specify Path, Timeout, and Interval. The next most supported fields are Hostname and Healthy/Unhealthy Thresholds.

@costinm
Copy link

costinm commented Apr 14, 2021

This seems to match what Pod readiness probe supports - any reason to not just use the exact same thing (as spec and API), and default to the HTTP readiness probe in the pod, if it exists ?

I assume like all K8S services, it will be required that gateways respect the K8S readiness semantics as well, i.e. if K8S kubelet
( and the associated EndpointSlice, Endpoint) is marked not ready, it will take priority and no traffic will be sent.

The network view may be different from kubelet view - but that doesn't mean users need to maintain 2 different endpoints.

I am concerned a bit with the scalability of such system - distributed health check in a mesh with multiple networks, security, etc are pretty difficult - if we are worried about the feeback loop from K8S we should also worry about the load and feedback loop on the health check system if it is mandated by the Gateaway API.

@robscott robscott added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 14, 2021
@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 21, 2021

I assume like all K8S services, it will be required that gateways respect the K8S readiness semantics as well, i.e. if K8S kubelet
( and the associated EndpointSlice, Endpoint) is marked not ready, it will take priority and no traffic will be sent.

This actually is a great point and requires some more discussion and probably an issue of its own. Some Ingress Controllers route traffic to the VIP of the k8s Service while others route traffic directly to the endpoints. Which behavior do we expect from implementations in Gateway API? cc @mark-church @bowei @robscott @danehans

The network view may be different from kubelet view - but that doesn't mean users need to maintain 2 different endpoints.

Users can maintain the same endpoint if that's what they want. We are not asking the users to maintain a different endpoint. Is that acceptable @costinm ?

I am concerned a bit with the scalability of such system - distributed health check in a mesh with multiple networks, security, etc are pretty difficult - if we are worried about the feeback loop from K8S we should also worry about the load and feedback loop on the health check system if it is mandated by the Gateaway API.

I think this point is similar to John's point above.
Costin, if the API is not prescriptive about how the health-checks are performed, is that okay?

@robscott robscott removed this from the v0.3.0 milestone Apr 23, 2021
@youngnick
Copy link
Contributor

I think the thing that we would probably need to be a little prescriptive about is that the Gateway (however it's implemented) should be doing its own active checking if health checks are specified.

That way we can be clear how they are distinct in behavior from adding a readiness check to a Pod and having Endpoints not visible in that way.

@howardjohn
Copy link
Contributor

I don't think its clear what "the Gateway" is, given the API is implementation agnostic. Does it mean all instances of the gateway (pod for in cluster workloads) must send requests to the pods to determine if its healthy? Does it mean one of them can and share the information with others? Can I spin up a distinct workload that does the health checks and reports it back? Stretching this really far, can that distinct workload be "kubelet" and "reporting it back" mean "put it in the pod readiness status"?

@youngnick
Copy link
Contributor

That's a great point. I was thinking of it more in terms of saying something like "if you specify a healthcheck here, it must be done by some other mechanism than a Readiness check". You may use a readiness check as well (to filter endpoints or something), but asking here implies having the Gateway controller do something more active than that. Note that this isn't taking a stance on active health-checks v passive ones, just that it's something that's not the Kubelet.

I do think it's fair to talk about "the Gateway" as a thing, since, at its heart, a Gateway describes some piece of network infrastructure that translates between traffic that doesn't know about cluster networking internals to something that does. (I've been working on this as a Gateway definition, but I don't think it's all the way there).

@hbagdi
Copy link
Contributor Author

hbagdi commented Aug 24, 2021

In search for a solution, we were unsuccessful in coming up with a portable way to support this feature.
As a compromise, we came up with an easy and portable way to support such implementation-specific features in a consistent manner. The solution is documented in #713.

If there are more portable ways to tackle this, we would love to hear about them.

For now, I'll close this issue.
/close

@k8s-ci-robot
Copy link
Contributor

@hbagdi: Closing this issue.

In response to this:

In search for a solution, we were unsuccessful in coming up with a portable way to support this feature.
As a compromise, we came up with an easy and portable way to support such implementation-specific features in a consistent manner. The solution is documented in #713.

If there are more portable ways to tackle this, we would love to hear about them.

For now, I'll close this issue.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/user-story Categorizes an issue as capturing a user story lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

8 participants