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.

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

Jump to bottom

Readiness health checks #3351

Merged
merged 21 commits into from
Aug 17, 2023
Merged

Readiness health checks #3351

merged 21 commits into from
Aug 17, 2023

Conversation

ameowlia
Copy link
Member

@ameowlia ameowlia commented Jul 19, 2023 •
edited by moleske
Loading

Context

This PR is part of the effort to add readiness health checks to Cloud Foundry. Deeper discussion of why can be read here in this CFF RFC.

Design

Users can set these 3 new properties on the app manifest. Error cases for invalid values etc are handled the same way as the other aready existing health-check 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

As an App Dev
When I set the readiness health check properties in my app manifest
Then I see those properties set on my app process
And then those properties are sent to bbs on the LRP

This change includes

  • Values can be set on the app manifest
  • Values set on app manifest are validated
  • These properties are stored in the DB
  • /v3/apps/:guid/manifest returns these properties
  • Docs for the /v3/apps/:guid/manifest are updated
  • /v3/processes returns these properties on the process
  • Docs for the /v3/processes are updated
  • These processes are passed to the LRP
  • When these changes are run with a new diego-release then everything works.

Contribution Steps

  • I have reviewed the contributing guide
  • I have viewed, signed, and submitted the Contributor License Agreement
  • I have made this pull request to the main branch
  • I have run all the unit tests using bundle exec rake
  • I have run CF Acceptance Tests
  • Remove all TODOs from code

Related PRs

@ameowlia ameowlia closed this Jul 21, 2023
@ameowlia ameowlia force-pushed the readiness-health-checks branch from 5b196ca to 1e3aa63 Compare July 21, 2023 14:29
@ameowlia ameowlia changed the title Readiness health checks via app manifest Readiness health checks Jul 21, 2023
@ameowlia ameowlia reopened this Jul 21, 2023
This was referenced Jul 21, 2023
Merged
Merged
Merged
ameowlia and others added 10 commits July 31, 2023 16:02
@ameowlia @sethboyles
small docs change to trick github
9a3afa2
@ameowlia @sethboyles
Add readiness health check properties to app manifest
dc83ce3 
Only adds it to manifest and validates the values. Does not do anything
with these values yet.

Signed-off-by: Marc Paquette <[email protected]>
Signed-off-by: Amelia Downs <[email protected]>
@ameowlia @sethboyles
Set readiness health check properties on process
faadcc4 
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]>
@mariash @sethboyles
correctly default to process for readiness_health_check_type
6aaff7b 
Signed-off-by: Amelia Downs <[email protected]>
@ameowlia @sethboyles
remove readiness_health_check_timeout
be5b523 
was added by mistake.

Signed-off-by: Maria Shaldybin <[email protected]>
@ameowlia @sethboyles
cleanup: rename list of health check types
456b232 
split out readiness health check types into its own method
@ameowlia @sethboyles
Refactor: add base health check policy class
3965310
@ameowlia @sethboyles
Set readiness health check properties on LRP
86eb038 
Signed-off-by: Maria Shaldybin <[email protected]>
@ameowlia @sethboyles
Refactor: app recipe builder
c6cabd0 
now with more ruby style!
@ameowlia @sethboyles
Fix: readiness hcs default to process not empty
01145c4 
that's what we should test
@sethboyles sethboyles force-pushed the readiness-health-checks branch from b88514b to 01145c4 Compare July 31, 2023 23:02
This was referenced Aug 1, 2023
Merged
Merged
@sethboyles
Merge pull request #3368 from mariash/readiness-health-checks-story-4…
cda279e 
…-manifest

Readiness health checks story 4 manifest endpoints
@mariash mariash mentioned this pull request Aug 3, 2023
Merged
@ameowlia ameowlia mentioned this pull request Aug 8, 2023
Closed
7 tasks
@mariash
Readiness health checks story 5 processes (#3370)
5dff948 
* Add readiness health check properties to process endpoints

GET /v3/processes
GET /v3/processes/:guid
PATCH /v3/processes/:guid
GET /v3/apps/:guid/processes
GET /v3/apps/:guid/processes/:type
PATCH /v3/apps/:guid/processes/:type
POST /v3/apps/:guid/processes/:type/actions/scale

* Add readiness health check examples to API docs
@ameowlia ameowlia mentioned this pull request Aug 9, 2023
Merged
7 tasks
@mariash
Rolling deploy respects readiness health check (#3373)
31a01d2 
ActualLRP has routable set to true once readiness health check succeeds.
In case if routable is not provided instance reporter sets it to true
for backwards compatibility.

Signed-off-by: Josh Russett <[email protected]>
@mariash mariash mentioned this pull request Aug 10, 2023
Merged
@mariash
Readiness health checks story 7 respect defined interval (#3385)
5b95cf8 
* Validate health_check_interval/readiness_health_check_interval in the manifest
* Save health_check_interval/readiness_health_check_interval in database
* Regenerate bbs protobuf models
* Pass health_check_interval/readiness_health_check_interval in LRP
* Display health_check_interval/readiness_health_check_interval in manifest
* Add health-check-interval/readiness-health-check-interval to manifest diff
* Add health-check-interval/readiness-health-check-interval to manifest docs
* Add health_check_interval/readiness_health_check_interval to /v3/processes
* Add health_check_interval/readiness_health_check_interval to API docs
* Copy health_check_interval/readiness_health_check_interval during rolling deploy

Signed-off-by: Maria Shaldybin <[email protected]>
Signed-off-by: Josh Russett <[email protected]>
@moleske
Copy link
Member

moleske commented Aug 16, 2023

I clicked rerun on the mysql 5.7 unit test as that had one failed test that is a known thing (it is checking number of seconds that happened and it was off by one 🙄)

The docs check is some links are missing, so those need double checked. Not sure what the complexity rubocop is picking up, I didn't follow up on that

ameowlia and others added 4 commits August 17, 2023 17:32
@ameowlia @moleske
add need readiness check doc to index
e4ee1ff 
Signed-off-by: Michael Oleske <[email protected]>
@moleske @ameowlia
make rubocop happy
df1005c 
Signed-off-by: Amelia Downs <[email protected]>
@moleske
Disable rubocop cyclomaticcomplexity for some functions
83c59cc 
- these functions are idomatic ruby with no nested if statements
- rubocop is just counting if statements so no current concerns
- please review if this changes
@moleske
Do not use text:true on readiness health check to process
69abd47 
- see https://github.com/cloudfoundry/cloud_controller_ng/wiki/CAPI-Migration-Style-Guide#always-specify-a-size-for-a-string-and-dont-use-text

Signed-off-by: Amelia Downs <[email protected]>
@moleske moleske marked this pull request as ready for review August 17, 2023 19:40
moleske
moleske approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@moleske moleske left a comment

Choose a reason for hiding this comment

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

approving as individual small commits were pr-ed to this branch and each of them were approved. and of course all unit tests are passing

@moleske moleske merged commit 89eb910 into main Aug 17, 2023
@moleske moleske deleted the readiness-health-checks branch August 17, 2023 20:59
@philippthun philippthun mentioned this pull request Sep 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moleske moleske moleske approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ameowlia @moleske @cf-gitbot @mariash @sethboyles