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

SpringBootHealthCheckEnricher should auto detect configured liveness and readiness probes #2665

Open
rohanKanojia opened this issue Feb 13, 2024 · 14 comments · May be fixed by #3036
Open

SpringBootHealthCheckEnricher should auto detect configured liveness and readiness probes #2665

rohanKanojia opened this issue Feb 13, 2024 · 14 comments · May be fixed by #3036
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@rohanKanojia
Copy link
Member

Component

JKube Kit

Is your enhancement related to a problem? Please describe

Description

Originally posted by @adriannowak in Gitter thread

jkube-healthcheck-spring-boot cannot distinguish between readiness and liveness probe, it sets the same '/actuator/health' endpoint for those two probes. shoudn't it be /actuator/health/readiness and /actuator/health/liveness

When user configures Spring Boot to enable liveness and readiness probes via application.properties:

management.health.probes.enabled=true

It enables /actuator/health/liveness and /actuator/health/readiness endpoints. However SpringBootHealthCheckEnricher still generates liveness and readiness probes with /actuator/health value:

$ mvn k8s:resource


$ cat target/classes/META-INF/jkube/kubernetes.yml | grep -A4 Probe
          livenessProbe:
            failureThreshold: 3
            httpGet:
              path: /actuator/health
              port: 8080
--
          readinessProbe:
            failureThreshold: 3
            httpGet:
              path: /actuator/health
              port: 8080

Describe the solution you'd like

SpringBootHealthCheckEnricher detects explicit liveness and readiness probes are enabled via checking project configuration.

Describe alternatives you've considered

No response

Additional context

No response

@rohanKanojia rohanKanojia added the enhancement New feature or request label Feb 13, 2024
@manusa manusa added the bug Something isn't working label Feb 14, 2024
@l3002
Copy link
Contributor

l3002 commented Feb 15, 2024

@manusa / @rohanKanojia : I am willing to pick this up if it is available for contributors. Kindly assign this to me.

@rohanKanojia
Copy link
Member Author

@l3002 : We need to figure out what property gets set whenever probes are enabled, also is the behavior the same when Spring boot configuration is provided in the form of application.yml instead of application.properties.

Are you okay with investigating this part first and then sharing your findings?

@l3002
Copy link
Contributor

l3002 commented Feb 15, 2024

yeah, not a problem from my end. I'll share my investigation findings before implementing anything.

@l3002
Copy link
Contributor

l3002 commented Feb 23, 2024

@manusa / @rohanKanojia, I'm sorry I haven't been able to get to this for a while, I've been cramped up with work from my day job. I'll try to propose a solution for this by the end of next week. I hope that's fine.

@l3002
Copy link
Contributor

l3002 commented Feb 28, 2024

Hi @manusa / @rohanKanojia: After doing a bit research on the topic, I'm unable to understand the point of changing the endpoints with the explicit actuator endpoint. Correct me if I'm wrong that we require LivenessStateHealthIndicator and ReadinessStateHealthIndicator from the endpoint to check the liveness and readiness of the application while it is deployed on kubernetes. If that's the case then, I don't think we require explicit endpoints for retrieving this data from actuators as per below:

image

Both the global endpoint /actuator/health and explicit endpoints /acutator/health/liveness & /acutator/health/readiness can be used to retrieve the data. So, I'm not sure if this is required.

But If this still holds some importance then I guess we just need to set the value for PATH in config with regards to the project configuration in SpringBootHealthCheckEnricher$Config enum:

image

@manusa
Copy link
Member

manusa commented Feb 29, 2024

Both the global endpoint /actuator/health and explicit endpoints /acutator/health/liveness & /acutator/health/readiness can be used to retrieve the data. So, I'm not sure if this is required.

Path to documentation https://docs.spring.io/spring-boot/docs/current/reference/html/actuator.html#actuator.endpoints.kubernetes-probes

We don't know if the user has customized the health checks with specific checks for readiness and liveness (which are different concepts, Kubernetes acts differently upon failure of the probe -liveness failure triggers restart, readiness failure isolates the pod for a period until healthy.).

It's important that we respect this and that we define the probe as per the specific endpoints instead of the global endpoint.

@l3002
Copy link
Contributor

l3002 commented Feb 29, 2024

Okay, I understand it now. So then, I guess it would be better to completely replace the global endpoint with two separate specific endpoints for liveness and readiness. Would that be fine?

@rohanKanojia
Copy link
Member Author

So then, I guess it would be better to completely replace the global endpoint with two separate specific endpoints for liveness and readiness.

We should give precedence to specific endpoints when we detect user has explicitly configured them. Otherwise, we can stick to current behavior.

@l3002
Copy link
Contributor

l3002 commented Mar 19, 2024

Hi @manusa / @rohanKanojia, I'm really sorry, I haven't been able to give this issue a considerable amount of time. Though, I do have a plan to implement this through by adding two additional constants to Config enum for liveness probe path & readiness probe path and replacing any instances of PATH with a conditional statement. I still need some time to test this idea and make it as simple and readable as possible.

@rohanKanojia
Copy link
Member Author

rohanKanojia commented Apr 22, 2024

@l3002 : polite ping, Did you get any time to revisit this issue? If not, shall I unassign you from this issue?

@l3002
Copy link
Contributor

l3002 commented Apr 22, 2024

@rohanKanojia: I'm still working on this, I'm trying to figure out, All the places where the changes are needed, after changing the Config Enum. Though, I haven't been able to give this issue much time since past couple of weeks, I'm still working on it and possibly will be able to close this by 2nd week of the next month.

As this is a part of enhancement, I'm trying to be as cautious as possible. Just don't want to accidently break anything, this is a pretty big issue for me.

@rohanKanojia
Copy link
Member Author

I'm trying to figure out, All the places where the changes are needed, after changing the Config Enum.

I think my original motivation behind creating this issue was to auto detect this configuration. I didn't mean to add a new configuration option for this. Let me try to explain step by step what needs to be done:

  1. Add a boolean field in SpringBootConfiguration class named managementHealthProbesEnabled. This would be storing value of detected management.health.probes.enabled property. You would need to add an equivalent builder method call for this new field here:
    configBuilder
    .managementPort(Optional.ofNullable(properties.getProperty("management.port")).map(Integer::parseInt).orElse(null))
    .serverPort(Integer.parseInt(properties.getProperty("server.port", DEFAULT_SERVER_PORT)))
    .serverKeystore(properties.getProperty("server.ssl.key-store"))
  2. SpringBootHealthCheckEnricher would read the value of springBootConfiguration.isManagementHealthProbesEnabled() to decide whether to go with generic health check path /actuator/health or specifics /actuator/health/liveness / /actuator/health/readiness
    String actuatorBasePath = springBootConfiguration.getActuatorDefaultBasePath();
  3. Right now buildProbe has no information about what kind of probe it's building, maybe we can add another String field called actuatorPathSuffix that will base passed as /readiness and /liveness from getReadinessProbe() and getLivenessProbe() respectively. When springBootConfiguration.isManagementHealthProbesEnabled() would evaluate to true, we will append these suffixes to actuatorBasePath if there is no explicit Config.PATH provided.
  4. Add tests verifying above mentioned changes work as expected

@l3002
Copy link
Contributor

l3002 commented Apr 23, 2024

@rohanKanojia : Oh, okay. I thought of creating the two separate Config fields for LIVENESS_PROBE_PATH & READINESS_PROBE_PATH in addition to the PATH already present and then add a new field to SpringBootConfiguration class for management.health.probes.enabled, but it would indeed be much easier to add a suffix based on the value of springBootConfiguration.managementHealthProbesEnabled as the base path already matches both of the specific parts.

Thanks for your insight. I'll try to implement this in code and test the result this week and update.

@l3002
Copy link
Contributor

l3002 commented Apr 29, 2024

@manusa / @rohanKanojia : Wanted to give both of you an update, I've implemented the changes and now, I'm working on Unit Tests for the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants