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

GPII-3450: Add readiness and liveness endpoints for Cloud Based Flow Manager and Preferences Server #699

Merged
merged 10 commits into from Oct 30, 2018

Conversation

Projects
None yet
4 participants
@cindyli
Copy link
Contributor

commented Oct 24, 2018

The gpii-ops pull request should be merged into gpii-infra repository once this pull request makes into the universal master so that the deployment to the production and staging environments continues to function.

@cindyli cindyli requested a review from amb26 Oct 24, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

cindyli added some commits Oct 24, 2018

GPII-3450: Improved the docs and production config test related to th…
…e new usage of GPII_FLOWMANAGER_TO_PREFERENCESSERVER_URL.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

ok to test

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

* **route:** `/user/:gpiiKey/logout` where `:gpiiKey` should be the GPII key of the user
* **method:** `GET`
* **return:** Message saying that user successfully logged out of the system or an error message.

## APIs on Cloud Based Flow Manager

### Check the readiness of Cloud Based Flow Manager (GET /ready)

This comment has been minimized.

Copy link
@mrtyler

mrtyler Oct 25, 2018

Contributor

I think something is backwards here, or maybe the endpoint names are confusing to me. From https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/:

The kubelet uses liveness probes to know when to restart a Container. For example, liveness probes could catch a deadlock, where an application is running, but unable to make progress. Restarting a Container in such a state can help to make the application more available despite bugs.

The kubelet uses readiness probes to know when a Container is ready to start accepting traffic. A Pod is considered ready when all of its Containers are ready. One use of this signal is to control which Pods are used as backends for Services. When a Pod is not ready, it is removed from Service load balancers.

This description of /ready sounds like a liveness check, while the description of /health sounds like a readiness check.

This comment has been minimized.

Copy link
@cindyli

cindyli Oct 25, 2018

Author Contributor

Thanks for the clarification on readiness and liveness, @mrtyler. I've swapped their implementations and improved documentation. Ready for another look. Thanks.

* **description**: Check whether Cloud Based Flow Manager is up and running.
* **route:** `/ready`
* **method:** `GET`
* **return:** Return http status code 200 when Cloud Based Flow Manager is up and running. Otherwise, return http status

This comment has been minimized.

Copy link
@mrtyler

mrtyler Oct 25, 2018

Contributor

It might help to elaborate on what "is up and running" means?

checks the communication with Preferences Server.
* **route:** `/health`
* **method:** `GET`
* **return:** Return http status code 200 when Cloud Based Flow Manager is up and running. Otherwise, return http status

This comment has been minimized.

Copy link
@mrtyler

mrtyler Oct 25, 2018

Contributor

Should this say "is up and running" again? Maybe it should say "is able to handle requests"?

As above, I think it might help to say more about what this means.

GPII-3450: Swapped the implementation for readiness and liveness endp…
…oints and improved docs as per code review comments.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@@ -25,7 +25,10 @@ require("./UserLogonHandlers.js");
require("./BrowserChannel.js");
require("./DefaultSettingsLoader.js");
require("./PSPChannel.js");
require("./CloudBasedFlowManager.js"); // TODO: move this include to be operated by a config
require("./PrefsServerDataSource.js");
require("./CloudBasedFlowManager.js");

This comment has been minimized.

Copy link
@amb26

amb26 Oct 25, 2018

Member

See the TODO which was removed - I think the time has come to remove all these includes from here and stick them in a config since none of them are actually referenced by any of the definitions in this file. At the least they should be moved to CloudBasedFlowManager.js but preferably to gpii.flowManager.config.cloud.base.json5 itself

This comment has been minimized.

Copy link
@cindyli

cindyli Oct 25, 2018

Author Contributor

I ended up moving the inclusion of CloudBasedFlowManager.js into flowManager/index.js and moving all cloud related scripts into CloudBasedFlowManager.js. This makes it easier for both integration tests and configs to load scripts for running cloud. For example, fluid.require("%gpii-universal"); used in integration tests will load all flow manager source code. If CloudBasedFlowManager.js is only moved into the cloud config files, those integration tests will need to include it separately.


// "gpii.flowManager.prefsServerDataSource" is used by the cloud based flow manager to communicate with the
// preferences server.
fluid.defaults("gpii.flowManager.prefsServerDataSource", {

This comment has been minimized.

Copy link
@amb26

amb26 Oct 25, 2018

Member

I'm a bit puzzled where all this impl came from since I don't see a big negative diff elsewhere in the pull?

This comment has been minimized.

Copy link
@cindyli

cindyli Oct 25, 2018

Author Contributor

prefsServerDataSource is to replace the previous preferencesDataSource. The old preferencesDataSource only needs to read/write preferences from /preferences endpoint on the prefs server. As now the cloud based flow manager also needs to ping /ready endpoint, I added this new data source for handling requests to multiple endpoints on the prefs server.

@mrtyler
Copy link
Contributor

left a comment

Docs LGTM (I didn't review the code).

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Pouch manager reset failure again:
19:27:20.481: jq: FAIL: Module "Testing Pouch Manager" Test name "Testing the load and the reset processes" - Message: The response should have a reasonable status code - at sequence position 18 of 20

This is becoming quite a hazard to work

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

ok to test

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

at endReadableNT (_stream_readable.j .... [output suppressed at 1024 chars - for more output, increase fluid.logObjectRenderChars]
"{"error":"OpenError","reason":"IO error: /tmp/pouchManagerTests-4pqs8va6-339976/gpii-mrview-ac9eefd3422f906a613ecb54cb900916/LOCK: No such file or directory"}\n" etc

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

same error on pouchDB cleanup. ok to test

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2018

@amb26 amb26 merged commit 73690cf into GPII:master Oct 30, 2018

1 check passed

default Build finished.
Details

@cindyli cindyli deleted the cindyli:GPII-3450 branch Nov 28, 2018

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