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

MP Health 2.0 readiness check returns HTTP 200 before app is ready #9609

Open
aguibert opened this issue Nov 1, 2019 · 4 comments
Open

MP Health 2.0 readiness check returns HTTP 200 before app is ready #9609

aguibert opened this issue Nov 1, 2019 · 4 comments

Comments

@aguibert
Copy link
Member

@aguibert aguibert commented Nov 1, 2019

If I have an app that enables the mpHealth-2.0 feature enabled in server.xml and has an app baked into the /dropins directory in a docker container using the openliberty:microProfile3 base image, the readiness check at /health/ready returns HTTP 200 before the app is actually available.

Steps to reproduce

The issue can be reproduced by cloning this repo: https://github.com/aguibert/microshed-testing/tree/mphealth-issue

git clone git@github.com:aguibert/microshed-testing.git
cd microshed-testing
git checkout mphealth-issue
./gradlew :microshed-testing-everything-app:test

After running the gradle test, the tests will fail with HTTP 404 errors because the tests try to run before the app is ready.

@aguibert

This comment has been minimized.

Copy link
Member Author

@aguibert aguibert commented Nov 1, 2019

I've attached trace of the failing scenario with HEALTH=all trace spec here:
trace.log

@pgunapal

This comment has been minimized.

Copy link
Member

@pgunapal pgunapal commented Nov 15, 2019

@aguibert I took a look at the trace and it seems like the /health/ready endpoint was called before the ApplicationStateListener.applicationStarting() method registered the application for mpHealth, while the server was starting. After discussing the issue with a few folks, I found that there is a design-issue that was being discussed (#7709), which I believe should resolve this issue.

The new design is basically, delaying the opening of the ports till the server has actually started. I worked with Bill W., got a PB to try out from the PR (#8016) associated with that issue. I replaced the wlp directory in the Docker Container from MicroShed with the one from the PB, and tried to reproduce the issue, and I could not anymore.

Basically, the ports will not be exposed till the server has started, meaning the /health/ready endpoint can not be accessed, till the server has started, so a false positive HTTP 200 will not be returned before the app is ready. I also tested the case where the server has started and ports are now opened, however the application has not started yet, this works fine with the current implementation of MP Health 2.0, as it returns the expected the HTTP 503 status, since the ApplicationStateListener works as expected and the deployed applications are registered correctly.

According to Bill W, the PR (#8016) will only be merged after 19.0.0.12, is that okay with you?

@aguibert

This comment has been minimized.

Copy link
Member Author

@aguibert aguibert commented Nov 15, 2019

HI @pgunapal, thanks for following up on this. That sounds like a good solution. So to summarize the flow of events we will have:

  1. Server starts with ports shut
  2. applicationStarting() events fire (for all apps on disk at the time)
  3. Server starts and ports open
  4. appStarting() events finish, allowing /health/ready to return HTTP 200
@pgunapal

This comment has been minimized.

Copy link
Member

@pgunapal pgunapal commented Nov 15, 2019

Yeah, that's the flow of the events, in a normal case.

For a case where the app have not started yet:
1. Server starts with ports shut
2. applicationStarting() events fire (for all apps on disk at the time)
3. Server starts and ports open
4. Apps still in STARTING state, /health/ready will return HTTP 503, until apps have started.
5. Apps have started, appStarting() events finish, allowing /health/ready to return HTTP 200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.