[WIP] fix potential issue with shutdown

[ElvinEfendi]

What this PR does / why we need it:

We have two layers of reverse proxies where the second layer is ingress-nginx instances. Everytime we deploy ingress-nginx the first layer proxies see significant number of 502s (should say that we handle millions of requests per minutes).

I think the problem is we fail readiness probes only after Nginx exits. Once the readiness probe fails only then K8s components proxying packets to ingress-nginx replicas starts removing the particular ingress-nginx replica from their upstream pool, but this takes time. During that time there's no Nginx running but the pod can still get requests, which will fail with connection refused because there's nothing listening on the ports since Nginx is shutdown. We currently sleep after Nginx exits, but this does not help with anything, is redundant.

Inline with the code I described what I'd like to do in this PR.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aledbf]

For this, we should add a check in

ingress-nginx/internal/ingress/controller/checker.go

Line 38 in 17e788b

func (n *NGINXController) Check(_ *http.Request) error {

checking for n.isShuttingDown to start failing the health check

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[yvespp]

@ElvinEfendi is this actually fixed in 0.26? In my tests the upstream proxy still sees errors because nginx refuses new connections immediately when the pod is terminated.
The upstream proxy calls the health check of nginx, this check has to fail 3 times before it removes the nginx from the pool and no new connections get routet to that instance.

As far as I can see, there is no time defined where the health check fails but nginx still accepts new connections.

[aledbf]

@yvespp did you update the ingress nginx deployment?

https://github.com/kubernetes/ingress-nginx/blob/master/deploy/static/with-rbac.yaml#L25
https://github.com/kubernetes/ingress-nginx/blob/master/deploy/static/with-rbac.yaml#L81

[yvespp]

@aledbf yes I did, even with the hook nginx stops accepting new connections immediately.

[aledbf]

even with the hook nginx stops accepting new connections immediately.

Yes, that is the intent. NGINX should not accept new connections, only drain the existing ones.

The upstream proxy calls the health check of nginx, this check has to fail 3 times before it removes the nginx from the pool and no new connections get routet to that instance.

You mean the cloud LB right?

As far as I can see, there is no time defined where the health check fails but nginx still accepts new connections.

You mean the cloud LB right? If that's the case, you are right, we don't set additional annotations. Please check https://gist.github.com/mgoodness/1a2926f3b02d8e8149c224d25cc57dc1

[aledbf]

@yvespp not sure I understand what's exactly the issue

[yvespp]

@aledbf We are on prem and use F5 as a TCP loadbalancer in front of the nginx pods:
client -> F5 TCP LB -> nginx Pods

1. The F5 checks the nginx health check every 5 seconds.
2. A nginx pod starts to terminate and immediately stops accepting new connections.
3. The F5 still sends new connections to the nginx pod for 5 seconds because the health check did not fail yet.

I think what would help here is a configurable sleep in wait-shutdown hook between the stop of the controller (which fails the health check) and the shutdown of nginx. Ningx would still acception new connections until the F5 has realized that it is down and stops sending new connections to it.

[aledbf]

First, thanks for the context.

Ningx would still acception new connections until the F5 has realized that it is down and stops sending new connections to it.

What you are proposing makes sense, i.e., a new flag for the time to sleep before shutting down NGINX using the default readiness probe failure value (30 seconds). That said, such change also requires an increase in the default 300s to 330s.

Please open a new issue with the comment you just posted and a link to this comment.

Edit: the sleep goes here

ingress-nginx/internal/ingress/controller/nginx.go

Line 406 in 182de47

klog.Info("Stopping NGINX process")

[aledbf]

@yvespp also, pull requests are welcome :)

[yvespp]

@ElvinEfendi done, thanks!