feat: Add support for delaying webhook shutdown #3170

dippynark · 2023-11-23T22:17:40Z

What this PR does / why we need it:

Currently, as far as I can tell, Gatekeeper shuts down immediately after a TERM signal is received. If a Gatekeeper Pod happens to shutdown just as the API server is setting up a connection for a webhook request (but before it realises the Pod has been terminated) then the request might fail trying to connect to a server that is no longer accepting connections.

To fix this, this PR adds a --shutdown-delay-duration flag with a default of 10 seconds. This matches with the similar API Server flag

Special notes for your reviewer:

I wasn't sure how best to unit test this given it interacts with signals
It reimplements the controller-runtime signal handler but does not differentiate between Windows and Unix
Very happy to completely reimplement this or remove it depending on the opinions of the reviewer, but just thought I'd give it a go first as it's a small change

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

codecov-commenter · 2023-11-23T22:25:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2370c50) 53.81% compared to head (f95e90b) 53.72%.

❗ Current head f95e90b differs from pull request most recent head 327ac16. Consider uploading reports for the commit 327ac16 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3170      +/-   ##
==========================================
- Coverage   53.81%   53.72%   -0.10%     
==========================================
  Files         136      136              
  Lines       12198    12198              
==========================================
- Hits         6564     6553      -11     
- Misses       5134     5141       +7     
- Partials      500      504       +4

Flag	Coverage Δ
unittests	`53.72% <ø> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

mprahl · 2023-11-27T15:46:11Z

main.go

+		// Cancel the context once the wait period expires but avoid blocking if
+		// a second signal is received
+		go func() {
+			<-time.After(waitPeriod)


From my reading of the PR, it seems that you intercept the signal and then wait 10 seconds to cancel the context. The issue is that the context being cancelled is what tells all the goroutines to shutdown gracefully. I don't know if this is 100% the case in this codebase, but as a rule of thumb, that is how it's done.

In my opinion, the ideal way to handle this is to let the context be cancelled as is currently done, but use a WaitGroup and have each goroutine call Done on the WaitGroup after they stop. You would then block in innerMain until the WaitGroup has completed or your timeout of 10 seconds. That being said, Kubernetes can already force kill the process after a timeout, so you could choose to rely on that instead of custom logic.

Additionally, you could create a new context with context.WithCancel from the controller-runtime context and pass that to all the goroutines so that when they exit with an error, they cancel that context thus signaling everything to shutdown gracefully.

Hey @mprahl, I might be misunderstanding some of the above, but delaying graceful shutdown was the intention here because as soon as graceful shutdown is triggered all webhook server listeners are closed and any TCP connections that are attempted by the API server just after this point will be rejected: https://github.com/kubernetes-sigs/controller-runtime/blob/2154ffbc22e26ffd8c3b713927f0df2fa40841f2/pkg/webhook/server.go#L248-L253

As a consumer of controller-runtime, the only thing we have control over when it comes to webhook server shutdown is the context and when it gets cancelled.

If I understand correctly, you are talking about configuring the manager to wait until all Go routines are shutdown gracefully, which is already happening (defaults to 30 seconds):

https://github.com/kubernetes-sigs/controller-runtime/blob/2154ffbc22e26ffd8c3b713927f0df2fa40841f2/pkg/manager/manager.go#L255-L259

https://github.com/kubernetes-sigs/controller-runtime/blob/2154ffbc22e26ffd8c3b713927f0df2fa40841f2/pkg/manager/internal.go#L55

Perhaps this is something that should supported properly in controller-runtime instead of manipulating the context here?

@dippynark, I'm not quite understanding the intent of this change because cancelling the context is the only way to signal to controller-runtime to shutdown.

Are you saying that there are other components within Gatekeeper that intercept the OS signal directly as well instead of relying on that parent context associated with the OS signal and it's child contexts? If not, then I read this code as sleeping for 10 seconds after the exit signal is received and then it instructs controller-runtime to shutdown, which just delays the existing behavior by 10 seconds.

@mprahl The intent is to wait for some time after the TERM signal is received to get the following behaviour:

TERM signal is sent to the Gatekeeper process by the kubelet

API server stops sending new connections to Gatekeeper webhooks

Gatekeeper shuts down webhook server listeners

Currently step 2 can come after step 3 if we are unlucky and some connections get rejected (i.e. there is a race condition) so this change is to try to ensure that step 3 always comes after step 2 by delaying shutdown for a short amount of time

Got it. That makes sense to me now.

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

maxsmythe

Thanks for submitting! I like the idea, though I'm wondering if the best route forward is to submit a PR to controller-runtime so all projects can benefit?

maxsmythe · 2023-12-02T02:40:33Z

main.go

+// setupSignalHandler registers a signal handler for SIGTERM and SIGINT and
+// returns a context which is canceled when one of these signals is caught after
+// waiting for the specified duration. If a second signal is caught then we
+// terminate immediately with exit code 1. The implementation has been stolen


To avoid misinterpretation, let's avoid "stolen". "This augments controller-runtime's model to support the delay".

Also, have you considered submitting this to controller-runtime? Generally it's better to avoid forks when possible.

@maxsmythe Thanks for the review, good point! I have created a controller-runtime PR here: kubernetes-sigs/controller-runtime#2601

Happy for this PR to be closed for now and I will create a new one if/when the controller-runtime PR is merged

Thank you for contributing!

ritazh · 2023-12-06T15:45:29Z

Closing in favor of kubernetes-sigs/controller-runtime#2601

#3170 (comment)

dippynark requested a review from a team as a code owner November 23, 2023 22:17

dippynark changed the title ~~feat: Add shutdown grace period flag~~ feat: Add support for graceful shutdown Nov 23, 2023

Add shutdown grace period flag

76718fa

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

dippynark force-pushed the add-shutdown-grace-period branch from c37453b to 76718fa Compare November 23, 2023 22:20

dippynark added 2 commits November 23, 2023 22:44

Use wait instead grace

f73f60c

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

Improve comment

c7fe1fb

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

dippynark force-pushed the add-shutdown-grace-period branch from 55c2900 to c7fe1fb Compare November 23, 2023 22:53

Improve flag description

7cdefb4

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

mprahl reviewed Nov 27, 2023

View reviewed changes

Merge branch 'master' into add-shutdown-grace-period

b2e1593

dippynark changed the title ~~feat: Add support for graceful shutdown~~ feat: Add support for delaying webhook shutdown Nov 29, 2023

Improve flag description

1c2ad5b

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

dippynark requested a review from mprahl November 29, 2023 21:59

Use --shutdown-delay-duration

40a3247

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

dippynark force-pushed the add-shutdown-grace-period branch from 8509e10 to 40a3247 Compare December 1, 2023 17:56

dippynark added 2 commits December 1, 2023 17:58

Merge branch 'master' into add-shutdown-grace-period

f95e90b

Improve flag description

327ac16

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

maxsmythe reviewed Dec 2, 2023

View reviewed changes

dippynark mentioned this pull request Dec 2, 2023

✨ Add support for controller-manager webhook shutdown delay kubernetes-sigs/controller-runtime#2601

Open

ritazh closed this Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for delaying webhook shutdown #3170

feat: Add support for delaying webhook shutdown #3170

dippynark commented Nov 23, 2023 •

edited

Loading

codecov-commenter commented Nov 23, 2023 •

edited

Loading

mprahl Nov 27, 2023 •

edited

Loading

dippynark Nov 29, 2023 •

edited

Loading

mprahl Nov 29, 2023

dippynark Nov 29, 2023 •

edited

Loading

mprahl Nov 29, 2023

maxsmythe left a comment

maxsmythe Dec 2, 2023

dippynark Dec 2, 2023

maxsmythe Dec 6, 2023

ritazh commented Dec 6, 2023

feat: Add support for delaying webhook shutdown #3170

feat: Add support for delaying webhook shutdown #3170

Conversation

dippynark commented Nov 23, 2023 • edited Loading

codecov-commenter commented Nov 23, 2023 • edited Loading

Codecov Report

mprahl Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

dippynark Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

mprahl Nov 29, 2023

Choose a reason for hiding this comment

dippynark Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

mprahl Nov 29, 2023

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe Dec 2, 2023

Choose a reason for hiding this comment

dippynark Dec 2, 2023

Choose a reason for hiding this comment

maxsmythe Dec 6, 2023

Choose a reason for hiding this comment

ritazh commented Dec 6, 2023

dippynark commented Nov 23, 2023 •

edited

Loading

codecov-commenter commented Nov 23, 2023 •

edited

Loading

mprahl Nov 27, 2023 •

edited

Loading

dippynark Nov 29, 2023 •

edited

Loading

dippynark Nov 29, 2023 •

edited

Loading