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

Initialize health check gateway state to Unknown #7087

Merged

Conversation

@stevendanna
Copy link
Contributor

stevendanna commented Oct 23, 2019

Previously, the gateway state had no health check information for a
given service until the first health-check. This resulted in a 404 for
the /services/SERVICE_NAME/SERVICE_GROUP/health endpoint. However,
according to the documentation, a 404 response code means the service
isn't loaded:

 /{name}/{group}/health:
     get:
         description: Health check status and output for the given service group
         responses:
             200:
                 description: Health Check - Ok / Warning
                 body:
                     application/json:
                         type: healthCheckOutput
             404:
                 description: Service not loaded
             500:
                 description: Health Check - Unknown
             503:
                 description: Health Check - Critical

This change avoids the problem by populating the gateway state with
Unknown when we start the health checks. As a result we now return 500
for the health endpoint of a loaded service until the first
health-check comes back, in accordance with the API documentation.

Fixes #6747

REVIEWER NOTES:

  • I'm not actually sure this is the best solution. It is a bit hard to follow all of the conditions in which we might call start_health_checks. We could instead check for the existence of the health check directly in http_gateway.rs by checking service_from_services(&service_group, state.gateway_state.lock_gsr().services_data())
@chef-expeditor

This comment has been minimized.

Copy link

chef-expeditor bot commented Oct 23, 2019

Hello stevendanna! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@christophermaier

This comment has been minimized.

Copy link
Contributor

christophermaier commented Oct 23, 2019

@stevendanna Which issue does this address? #674 doesn't seem like the right one.

Copy link
Contributor

christophermaier left a comment

I think that while the GatewayState is basically storing a collection of JSON strings, this looks like a good way to go.

Ideally, the GatewayState could be a bit smarter, but it's how it is as a matter of historical contingency.

@christophermaier

This comment has been minimized.

Copy link
Contributor

christophermaier commented Oct 23, 2019

@stevendanna You might want to rebase this once #7088 lands, since it takes care of the compiler warnings you found.

@stevendanna

This comment has been minimized.

Copy link
Contributor Author

stevendanna commented Oct 23, 2019

Sorry, correct fix issue is #6747

I'll fix that up in the commit message when I rebase on top of that change you mentioned.

Previously, the gateway state had no health check information for a
given service until the first health-check. This resulted in a 404 for
the `/services/SERVICE_NAME/SERVICE_GROUP/health` endpoint. However,
according to the documentation, a 404 response code means the service
isn't loaded:

```
 /{name}/{group}/health:
     get:
         description: Health check status and output for the given service group
         responses:
             200:
                 description: Health Check - Ok / Warning
                 body:
                     application/json:
                         type: healthCheckOutput
             404:
                 description: Service not loaded
             500:
                 description: Health Check - Unknown
             503:
                 description: Health Check - Critical
```

This change avoids the problem by populating the gateway state with
Unknown when we start the health checks. As a result we now return 500
for the health endpoint of a loaded service until the first
health-check comes back, in accordance with the API documentation.

Fixes #6747

Signed-off-by: Steven Danna <steve@chef.io>
@stevendanna stevendanna force-pushed the stevendanna:ssd/unknown-health-check branch from b668d3a to 4dc2dbe Oct 23, 2019
@stevendanna

This comment has been minimized.

Copy link
Contributor Author

stevendanna commented Oct 24, 2019

Rebased.

@christophermaier christophermaier merged commit 5d0f52f into habitat-sh:master Oct 24, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3882 passed (40 minutes, 53 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #950 passed (46 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@stevendanna stevendanna deleted the stevendanna:ssd/unknown-health-check branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.