Readiness check behaves wrongly

[SvenW]

Describe the bug
The default readiness check behaves strangely. If the router gets a SIGINT/SIGTERM it cleans up all remaining connections, however it keeps responding {"status":"UP"} throughout the whole termination process which makes it get new connections that it wont be able to terminate in time before being killed by kubernetes. This is fine in the liveness probe part but not in the readiness however.

To Reproduce
Steps to reproduce the behavior:

1. Setup services '...'
2. Look at the /health endpoint
3. See that it gives a up status even though it has received a SIGINT/SIGTERM

Expected behavior
As soon as the router gets a termination signal it should stop answering {"status":"UP"} on the readiness probe checks. Still fine to do it on the liveness probe though. Would be nice to be able to configure how long this termination grace period should be in the YAML.

Additional context
This is when running on GKE v1.24.12

[Meemaw]

You can already configure terminationGracePeriodSeconds:

router/helm/chart/router/values.yaml

Line 207 in daf9dda

terminationGracePeriodSeconds: 30

[SvenW]

You can already configure terminationGracePeriodSeconds:

router/helm/chart/router/values.yaml

Line 207 in daf9dda

terminationGracePeriodSeconds: 30

Yes, but that's not sufficient unfortunately. In order for the readiness to behave correctly both that value and being able to configure how long the router itself is able to work on draining connections needs to be configurable. We need to be able to respond to requests during the time when the LB is still removing us and after that removal stop serving requests and drain old connections

[SvenW]

I've bypassed this by adding a shell script that captures the signals and creates a file when it shuts down. The readiness probe then checks if that file exists in order to determine if the pod is still ready or not. This works but it's not especially nice :(

[garypen]

I agree this isn't a great situation. I've got a WIP PR which addresses this problem and I've bumped the priority to P1. My hope is that I'll find some time to fix this soon.

[voslartomas]

@SvenW Could you share what exactly did you do to overcome this issue please?

I have tried to add touch /tmp/shutdown file in preStop hook and then check it in readinees probe, but still I can see 502 when deploying new version of router.

[SvenW]

@SvenW Could you share what exactly did you do to overcome this issue please?

I have tried to add touch /tmp/shutdown file in preStop hook and then check it in readinees probe, but still I can see 502 when deploying new version of router.

Sure. Disregard the ugliness in my little bash script here:

#!/bin/bash

trap 'kill -TERM $PID' TERM INT
exec /dist/router_wrapper.sh &
PID=$!
trap 'touch $SHUTDOWN_FILE' TERM INT
wait $PID
sleep 20
trap - TERM INT
kill -SIGINT $PID
wait $PID
EXIT_STATUS=$?

In order for it to work I had to send a SIGINT to the router pid, if I didn't it didn't receive the termination as it should, and then in my deployment.yaml for the router I had to rewrite the readiness probe to a bash based like this:

readinessProbe:
  exec:
    command:
    - sh
    - -c
    - "if [ -f /tmp/shutdown ]; then exit 1; else exit 0; fi;"

[voslartomas]

That makes sense, that is actually beautiful, thanks!

[voslartomas]

@SvenW And do you have any special configuration in deployment?

[SvenW]

@SvenW And do you have any special configuration in deployment?

I just use the default terminationGracePeriodSeconds which is 30s and for the probe I've used the default values for now

[voslartomas]

All right, one more question why do you have that 20s sleep?

[SvenW]

All right, one more question why do you have that 20s sleep?

Something that is less than the terminationGracePeriodSeconds so we have a little time to say we're not ready before removing the currently running requests as the router does for us.

[voslartomas]

All right, one more question why do you have that 20s sleep?

Something that is less than the terminationGracePeriodSeconds so we have a little time to say we're not ready before removing the currently running requests as the router does for us.

But this trap 'kill -TERM $PID' TERM INT first command will send term signal to router as soon as it will receive it right? What is that for? I thought that we want to catch that signal, first create shutdown file and wait for 20s and then send term signal to router? Or am I missing something?

[SvenW]

All right, one more question why do you have that 20s sleep?

Something that is less than the terminationGracePeriodSeconds so we have a little time to say we're not ready before removing the currently running requests as the router does for us.

But this trap 'kill -TERM $PID' TERM INT first command will send term signal to router as soon as it will receive it right? What is that for? I thought that we want to catch that signal, first create shutdown file and wait for 20s and then send term signal to router? Or am I missing something?

It is catching the term signal now. We are sending the SIGINT signal to the pid after 20s. You can test it locally by building your local Dockerfile and then sending a SIGINT or SIGTERM signal to that container to validate that it's working as it should. If you have ideas of improving the bash script I would really appreciate it

[voslartomas]

All right it works, I just had to add preStop with sleep for 10 seconds for some reason, after that 0 502 errors on deployment. Thanks!