Graceful shutdown for Nginx

[maxlaverse]

Issue

Nginx is not being gracefully shut down when a Nginx Ingress Controller container is stopped, leading to broken connections. It looks like the SIGTERM sent to the controller to stop it is simply forwarded to the Nginx server.

The problem is that SIGTERM means "Exit as quick as you can" for Nginx (Nginx doc. For a graceful shutdown we should rather send a SIGQUIT.

A solution to this issue is to prevent the Nginx Ingress Controller to forward all the signals it gets to Nginx, allowing us to control what's sent to the Nginx process. When shutting down, the controller would then send a SIGQUIT to Nginx. Nginx itself will wait 10secondes (worker_shutdown_timeout) for the worker to shutdown.

Testing

You need a backend exposed on the Ingress that responds slowly. I used one with a 10 secondes response time. Trigger an Ingress deployment and call this slow URL. If your slow URL get an answer before the Nginx is tear down, re-execute it.

We expected this request to be answered to. You should see in the Ingress logs that it's not immediately exiting but waiting for the request to finish.
Additionally, you can strace the Nginx master process to make sure SIGQUIT is not being sent instead of SIGTERM.

[k8s-reviewable]

This change is 

[aledbf]

@maxlaverse I am not sure we should add this to the interface. We cannot assume the implementations are running a process.
This is the point where we handle the SIGTERM
https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/main.go#L34

[aledbf]

I think we cannot rely using a timer because if the user sets terminationGracePeriodSeconds: 5 this will not work

[maxlaverse]

You're right. The value should then be configurable.

While running more tests this morning I realized that Nginx has a timeout already kicking in for that scenario:
worker_shutdown_timeout]

This might not be needed at all

[aledbf]

@maxlaverse also please check https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/nginx.go#L179
We already have some code to start a new master process if it dies

[coveralls]

Coverage decreased (-0.2%) to 43.479% when pulling 79a3195 on maxlaverse:graceful_shutdown into a8bfdc2 on kubernetes:master.

[aledbf]

@maxlaverse please test quay.io/aledbf/nginx-ingress-controller:0.198
This image contains #1258

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[coveralls]

Coverage decreased (-0.2%) to 43.734% when pulling f8c4d6373506effa1f242e18cfef627725092861 on maxlaverse:graceful_shutdown into 7844415 on kubernetes:master.

[coveralls]

Coverage decreased (-0.2%) to 43.734% when pulling f8c4d6373506effa1f242e18cfef627725092861 on maxlaverse:graceful_shutdown into 7844415 on kubernetes:master.

[coveralls]

Coverage decreased (-0.2%) to 43.709% when pulling f8c4d6373506effa1f242e18cfef627725092861 on maxlaverse:graceful_shutdown into 7844415 on kubernetes:master.

[coveralls]

Coverage decreased (-0.2%) to 43.734% when pulling f8c4d6373506effa1f242e18cfef627725092861 on maxlaverse:graceful_shutdown into 7844415 on kubernetes:master.

[maxlaverse]

@aledbf I change the description of this PR to explain how I test it. I pushed some changes to main.go I was thinking of after I had read your first comment.

I had a look at your pull-request and made some comments. It's not working because of the way we detect that Nginx has stopped after we gracefully ask it to. Checking if it's answering is probably not an option as it's not accepting incoming connections.

I'm not sure what's the best way to disable the automatic restart process and get notified about Nginx having being stopped. Except if we find another way to detect that Nginx is properly shut down ?

[aledbf]

Except if we find another way to detect that Nginx is properly shut down ?

We can use https://github.com/mitchellh/go-ps to check if there's a nginx process running.

[aledbf]

Merging. I will fix the dependencies issue in another PR

[aledbf]

@maxlaverse thanks!