Readiness health checks story 2 in db #3361

ameowlia · 2023-07-26T14:56:43Z

This PR is part of a larger effort detailed here in this Cloud Controller PR and here in this CFF RFC.

In this story, the 3 new app manifest properties (created in this PR), are set in the db.

New App Manifest Properties

applications:
- name: test-app
  processes:
  - type: web
    health-check-http-endpoint: /health
    health-check-invocation-timeout: 2
    health-check-type: http
    timeout: 80
    readiness-health-check-http-endpoint: /ready       # 👈 new property
    readiness-health-check-invocation-timeout: 2       # 👈 new property
    readiness-health-check-type: http                  # 👈 new property

Acceptance Steps

When I provide valid properties they are saved on the db. Below are examples of valid configs.

 # nothing! will default to process in the db

  readiness-health-check-http-endpoint: /ready
  readiness-health-check-invocation-timeout: 2
  readiness-health-check-type: http

  readiness-health-check-type: http

  readiness-health-check-type: port

  readiness-health-check-type: process

When I provide invalid properties, I get an error, and they are not saved in the db. Below are examples of invalid configs:

  readiness-health-check-http-endpoint: ready
  readiness-health-check-invocation-timeout: -10
  readiness-health-check-type: http

  readiness-health-check-http-endpoint: /ready
  readiness-health-check-type: port

  readiness-health-check-type: meow

Previous PRs

Readiness health checks story 1: adding properties to app manifest #3354

Now when a user supplies the readiness health check properties via the app manifest, the values are stored on the process, and saved in the database. Signed-off-by: Renee Chu <[email protected]>

ameowlia · 2023-07-26T17:48:42Z

app/models/runtime/constraints/health_check_policy.rb

 class HealthCheckPolicy
-  def initialize(process, health_check_timeout, health_check_invocation_timeout)
+  def initialize(process, health_check_timeout, health_check_invocation_timeout, health_check_type, health_check_http_endpoint)


We refactored the health check policy, to contain all of the checks for health checks, and removed some checks that were in the process model. We think this consolidates these checks more.

Also we refactored so that we could make a ReadinessHealthCheckPolicy that inherits from this class.

Signed-off-by: Amelia Downs <[email protected]>

was added by mistake. Signed-off-by: Maria Shaldybin <[email protected]>

sethboyles · 2023-07-27T19:30:42Z

app/models/helpers/health_check_types.rb

+    def self.constants_to_array
+      [
+        HTTP,
+        NONE,


I don't understand why we have NONE at all, do you? Trying to set a health check to none via the API results in the following error:

± sb |readiness-health-checks-story-2-in-db → origin {3} ?:1 ✗| → cf curl -X PATCH v3/processes/c6493e8f-84b0-4e84-be1f-7156b7704039 -d '{"health_check": {"type": "none","data": { }}}' { "errors": [ { "detail": "Health check type must be \"port\", \"process\", or \"http\"", "title": "CF-UnprocessableEntity", "code": 10008 } ] }

It looks like it used to be an option, but no longer is. It looks like for awhile there was backwards compatible logic where it would map none -> process. I suspect it is dead code now and could be ripped out. If you want we can do it in another story.

It'd be nice to clean up, but NONE's presence doesn't seem too onerous right now. I'll leave it to you to decide if you want to do it, and if not I'll create a story/github issue to track it.

I added a story in our epic to do it.

app/models/helpers/health_check_types.rb

sethboyles

LGTM! The only major thing I would ask you to consider changing is the ReadinessHealthCheckPolicy < HealthCheckPolicy inheritance pattern. I think having a BaseHealthCheckPolicy that both policies inherit from would avoid any surprises that might come from changing HealthCheckPolicy and seeing the effects in ReadinessHealthCheckPolicy

split out readiness health check types into its own method

ameowlia · 2023-07-28T14:54:21Z

@sethboyles The latest commit adds BaseHealthCheckPolicy class for both the ReadinessHealthCheckPolicy and HealthCheckPolicy classes to inherit from. Both the ReadinessHealthCheckPolicy and HealthCheckPolicy are very slimmed down now. I wasn't sure about the best way to test all these components.

Options for testing health check policy classes

Test all the main logic once in base_health_check_policy_spec. Then only test the differences on each of the inheriting classes.
Don't test BaseHealthCheckPolicy at all since nothing uses it explicitly. Put all tests in on ReadinessHealthCheckPolicy and HealthCheckPolicy.
Test everywhere! Put tests on BaseHealthCheckPolicy, ReadinessHealthCheckPolicy, and HealthCheckPolicy. 👈 This is the current state of the world.

I chose option 3. I am happy to change the testing pattern if you would prefer a different option.

sethboyles · 2023-07-28T17:29:17Z

LGTM!

cf-gitbot added the unscheduled label Jul 26, 2023

ameowlia force-pushed the readiness-health-checks-story-2-in-db branch 3 times, most recently from e28fe36 to 44ea08a Compare July 26, 2023 16:30

ameowlia marked this pull request as ready for review July 26, 2023 16:32

Set readiness health check properties on process

85b22b9

Now when a user supplies the readiness health check properties via the app manifest, the values are stored on the process, and saved in the database. Signed-off-by: Renee Chu <[email protected]>

ameowlia force-pushed the readiness-health-checks-story-2-in-db branch from 44ea08a to 85b22b9 Compare July 26, 2023 16:44

ameowlia commented Jul 26, 2023

View reviewed changes

mariash and others added 2 commits July 26, 2023 18:00

correctly default to process for readiness_health_check_type

d342ad4

Signed-off-by: Amelia Downs <[email protected]>

remove readiness_health_check_timeout

0dbbb06

was added by mistake. Signed-off-by: Maria Shaldybin <[email protected]>

sethboyles reviewed Jul 27, 2023

View reviewed changes

ameowlia mentioned this pull request Jul 27, 2023

Readiness health checks story 3 add to lrps #3362

Merged

sethboyles reviewed Jul 27, 2023

View reviewed changes

app/models/helpers/health_check_types.rb Outdated Show resolved Hide resolved

sethboyles approved these changes Jul 27, 2023

View reviewed changes

cleanup: rename list of health check types

2cfe394

split out readiness health check types into its own method

ameowlia force-pushed the readiness-health-checks-story-2-in-db branch from 29f6068 to 2cfe394 Compare July 27, 2023 21:23

Refactor: add base health check policy class

2693cf8

ameowlia merged commit acb51bf into readiness-health-checks Jul 28, 2023

cf-gitbot removed the unscheduled label Jul 28, 2023

ameowlia deleted the readiness-health-checks-story-2-in-db branch July 28, 2023 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readiness health checks story 2 in db #3361

Readiness health checks story 2 in db #3361

ameowlia commented Jul 26, 2023 •

edited

Loading

ameowlia Jul 26, 2023

sethboyles Jul 27, 2023

ameowlia Jul 27, 2023

sethboyles Jul 27, 2023

ameowlia Jul 27, 2023

sethboyles left a comment

ameowlia commented Jul 28, 2023

sethboyles commented Jul 28, 2023

Readiness health checks story 2 in db #3361

Readiness health checks story 2 in db #3361

Conversation

ameowlia commented Jul 26, 2023 • edited Loading

New App Manifest Properties

Acceptance Steps

Previous PRs

ameowlia Jul 26, 2023

Choose a reason for hiding this comment

sethboyles Jul 27, 2023

Choose a reason for hiding this comment

ameowlia Jul 27, 2023

Choose a reason for hiding this comment

sethboyles Jul 27, 2023

Choose a reason for hiding this comment

ameowlia Jul 27, 2023

Choose a reason for hiding this comment

sethboyles left a comment

Choose a reason for hiding this comment

ameowlia commented Jul 28, 2023

sethboyles commented Jul 28, 2023

ameowlia commented Jul 26, 2023 •

edited

Loading