Update liveness status

[tanoshii]

Hi, I created a livenessCheck that just check a variable:

let isMyServiceOk;
const checker = new health.LivenessCheck('MyService', new Promise((resolve, reject) => {
    if (isMyServiceOk) {
      resolve();
    } else {
      reject({ message: 'Failed!' });
    }
}));

I registered the livenessCheck and then registered the endpoint.

const healthcheck = new health.HealthChecker();
healthcheck.registerLivenessCheck(checker);
app.use('/health', health.LivenessEndpoint(healthcheck));

I expected that if I changed the value of "isMyServiceOk" the response from /health change too, but once the promise is resolved the first time, it just return always the same status.

Is there a way to update the livenessCheck each time endpoint is called?

[seabaylea]

The behaviour your looking for is what should occur... @BethGriggs could you take a look?

[Templum]

Maybe I oversee something but in the documentation it states:

const livePromise = new Promise(function (resolve, _reject) {
  setTimeout(function () {
    console.log('ALIVE!');
    resolve();
  }, 10);
});
let liveCheck = new health.LivenessCheck("liveCheck", livePromise);
healthcheck.registerLivenessCheck(liveCheck);

However I believe what you want is an function that returns a Promise, because once the promise is resolved (for the first time) it will stay in that state. So everytime you check the state, you check the old resolved/reject promise:

const livePromise = () => { return new Promise(function (resolve, _reject) {
  setTimeout(function () {
    console.log('ALIVE!');
    resolve();
  }, 10);
});
}
let liveCheck = new health.LivenessCheck("liveCheck", livePromise);
healthcheck.registerLivenessCheck(liveCheck);

[runofthemillgeek]

Agree with @Templum here. I was just going through this and pondering how this could work with just only one promise created. You need to create a new promise object every single call and wait for that to resolve/reject for this to work.

[BethGriggs]

@tanoshii, does @Templum's suggestion work for you?

It seems like this is just a case of updating our documentation over in https://github.com/CloudNativeJS/cloud-health, would you agree @Templum?

[Templum]

@BethGriggs based on your tests, your implementation is not supporting it. At least the tests are also wrongly testing with a single promise.

So I believe this might be also something that needs a fix to the coding.

[micksi]

I have been looking a little into this issue as I just started using it and experienced the same behaviour as @tanoshii. I believe the issue lies in the implementation of LivenessCheck in the cloud-heath module as it expects a promise and not a "promise generator" e.g. a function returning a promise.

As expected the tests in cloud-health shows the same "wrongly" implementation suggest only a single promise is allowed in livenessCheck. @seabaylea are you able to shed some light on this issue and maybe outline what the intended solution should do. I will gladly make a pull request if I know what the intended behaviour is.

[BethGriggs]

@micksi, I agree that we need is a function returning a promise in the LivenessCheck over in Cloud-Health. I haven't had a chance to PR that yet - if you would like to that would be great, if not I aim to get to it sometime this week.

[micksi]

@BethGriggs I'm trying to prepare a pull request but a lot of the tests doesn't seem to do the intended.
Just to clarify:

• readinessChecks should resolve the checks once and remember the result
• livenessChecks should generate new promises every time livenessChecks are called
• shutdownChecks should resolve the checks once and remember the result

The documentation should probably be updated to reflect this?

Example of check instantiation

// Instantiation of check for readiness and shutdownChecks
const readinessPromise = new Promise<void>(function (_resolve, _reject) {
  // tslint:disable-next-line:no-unused-expression no-shadowed-variable
  setTimeout(_resolve, 100, 'foo');
});
const promise = () => readinessPromise;
let check = new ReadinessCheck("check", promise)

// Instantiation of livenessChecks
const promiseone = () => new Promise<void>(function (resolve, _reject) {
  resolve()
});
let checkone = new LivenessCheck("checkone", promiseone)
healthcheck.registerLivenessCheck(checkone)

[BethGriggs]

@micksi, I agree with your statements about livenessChecks and shutdownChecks.

For readinessChecks, I can now imagine a scenario where after the initial ready check you would like your application to be marked as "Not Ready" based on certain check conditions. In that case, I think we'd like the readinessCheck to be able to run multiple times.

[micksi]

@BethGriggs currently such a state as "Not Ready" does not exist and I would expect it should return "STARTING" instead if the readinessPromises haven't resolved yet. If the readinessCheck fails it should report "DOWN" and be eligible to be taken down by Kubernetes. I might be mistaken though but I believe readinessCheck should still only be checked once.

[BethGriggs]

In terms of the "not ready" state - I think in the case of readiness checks "DOWN" would mean stop serving me traffic rather than take me offline. If that is the case, should it be checked it more than once?

https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#define-readiness-probes

(I could be mistaken, and thanks for your help!)

[micksi]

Been reading more into the expected behaviour since it seemed a little disambiguous and came to this issue, kubernetes/kubernetes#37450 . It seems you are right readiness should generate new promises to ensure it is ready to receive data.
I will fix the PR to reflect that later today.

TL;DR readinessCheck should run multiple times

[tanoshii]

@BethGriggs @Templum No, it didn't work, but that can be a good solution. In the current version, if I pass a function instead of a promise it is interpreted as always resolved without executing the function.

[BethGriggs]

@tanoshii, @seabaylea has recently published v2.0.0 of both cloud-health and cloud-health-connect to npm - this release should fix the issue reported here. Note that this was a breaking change, the README/tutorial has been updated to reflect the new implementation https://github.com/CloudNativeJS/cloud-health/blob/master/README.md#using-cloud-health-with-nodejs