Update graceful worker shutdown functionality to better handle worker terminations

[80degreeswest]

This PR simplifies graceful worker shutdown functionality by triggering it on worker shutdown (if enabled). Graceful worker shutdown allows the worker to deregister itself from taking on new work, while it stays up and finishes work in flight.

When a worker shuts down for whatever reason (normal autoscaling, for example), there is a potential for the bazel client to get stuck waiting for an execution complete response that never comes, making the build hang forever. Graceful shutdown avoids this issue, by allowing the worker to complete executions before it goes offline. This functionality also has the benefit of not interrupting the actions in flight and having them be rescheduled on another worker, prolonging the build time.

Previous configuration, which should now be removed:

server:
  admin:
    enableGracefulShutdown: false

New configuration to enable (0=disabled)

worker:
  gracefulShutdownSeconds: 900

If running on kubernetes, use terminationGracePeriodSeconds:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      terminationGracePeriodSeconds: 900

Example with no work in flight:

^C*** shutting down gRPC server since JVM is shutting down
Graceful Shutdown - The current worker will not be registered again and should be shutdown gracefully!
Graceful Shutdown - no work in the pipeline.
Graceful Shutdown - It took the worker 0 seconds to finish all actions
*** server shut down

Example of work in flight:

^C*** shutting down gRPC server since JVM is shutting down
Graceful Shutdown - The current worker will not be registered again and should be shutdown gracefully!
Graceful Shutdown - waiting for executions to finish.
Graceful Shutdown - Pipeline is still not empty after 30 seconds.
Graceful Shutdown - Pipeline is still not empty after 60 seconds.
Graceful Shutdown - It took the worker 60 seconds to finish all actions
*** server shut down

[jacobmou]

do you we prefer System.err.println over logging here? any specific reason?

[80degreeswest]

The logging does not work at this stage since it it looks like it stops logging before PreDestroy is called. So we output to stderr to match the shutdown message.

[jacobmou]

sounds like this is something we want to fix or at least there should be some comments about why we have to use errr.println.

[jacobmou]

i would prefer to do this branch first since the block is shorter. but no big deal.

if (configs.getWorker().getGracefulShutdownSeconds() ==0) {
  System.err.println(...);
} else {
  ...
}

[jacobmou]

my understanding is, this will work when the worker process received interruption. Will the new mechanism work in a scale in cycle of a non k8s environment? i was expecting autoscaling from aws would just terminated the worker instance instead of waiting for the java process to exit properly.

[80degreeswest]

It will work with AWS's lifecycle hook. There will be additional configuration needed on the EC2 side to catch the hook and issue a sigterm to stop the running buildfarm worker process. We did it in the past by writing an empty .paused file but that was a bit hacky. This is a better solution that does not require writing files but rather just stop the process and let it handle the rest.

[jacobmou]

I see. yeah sounds like we want to add comments and docs about the fact that this will rely on the lifecycle hook check feature of aws. This will make things easier for users who want to use the feature.

[80degreeswest]

Just to clarify, it always did rely on the lifecycle hook, which is how we used it in the past. Essentially, the lifecycle hook would send a signal to the EC2 instance, which then would write the .paused file which de-registed the worker. Then lifecycle hook would send the termination signal after the hook period time elapsed. There is no change to that use case here.

[jacobmou]

deleting of this mechanism might deserve some noting in the summary of the PR and future release notes

[80degreeswest]

Yep good point. Honestly i doubt anyone used this as it's a bit hacky.

[jacobmou]

I agree that this is hacky and likely not used by anyone. But the fact that the new mechanism only work on aws with lifecycle hook, i'm wondering if we want to keep this functionalities so that users on other cloud platforms can still use graceful shutdown through this.

[80degreeswest]

Ok i'll re-add this back in.

[jacobmou]

looks good! thanks for the improvement!