nginx-ingress-controller sleeps for 10 seconds before exiting

[rittneje]

For some reason, during shutdown nginx-ingress-controller sleeps for 10 seconds after shutting down nginx.

ingress-nginx/cmd/nginx/main.go

Lines 171 to 181 in e625c25

	exitCode := 0
	if err := ngx.Stop(); err != nil {
	klog.Warningf("Error during shutdown: %v", err)
	exitCode = 1
	}
	klog.InfoS("Handled quit, awaiting Pod deletion")
	time.Sleep(10 * time.Second)
	klog.InfoS("Exiting", "code", exitCode)
	exit(exitCode)

This seems entirely pointless, and only serves to make pod shutdown take longer.

The only benefit I can think of is it gives the pre-stop hook time to do any other actions after /wait-shutdown, since once pid 1 exits all other processes are immediately killed. But if that's the intent, (1) it should be configurable, and (2) the log message should be amended.

[strongjz]

/priority backlog
/triage accepted
/kind feature
/assign @theunrealgeek

[k8s-ci-robot]

@strongjz: GitHub didn't allow me to assign the following users: theunrealgeek.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/priority backlog
/triage accepted
/kind feature
/assign @theunrealgeek

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.

[theunrealgeek]

/assign

[theunrealgeek]

Thanks for bringing this up. I looked into this a little more than this was the commit 4 years ago that introduced the 10 second sleep but I can't find any good reasoning for it either. ngx.Stop() a few lines above is already waiting for all nginx processes to exit.

(1) it should be configurable

I agree that it should be configurable, so a PR is welcome if you like to make it.

and (2) the log message should be amended.

Technically the message isn't wrong, do you have suggestions on what it should be?

[rittneje]

@theunrealgeek To mean the current log message implies that it is waiting for some event from the control plane. Something like "Handled quit, delaying container exit for %s" would be more clear.

[theunrealgeek]

@rittneje The feature to be able to configure the delay period was merged and should be available in a future release. Thanks

[strongjz]

/close

[k8s-ci-robot]

@strongjz: Closing this issue.

In response to this:

/close

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.