apiservers: add synchronous shutdown mechanism on SIGTERM+INT #50439

sttts · 2017-08-10T08:36:01Z

This is used to shutdown the auditing backend in order not to drop any pending events on the floor.

crassirostris · 2017-08-10T08:40:12Z

cmd/hyperkube/kube-aggregator.go

 			if err := o.Complete(); err != nil {
 				return err
 			}
 			if err := o.Validate(args); err != nil {
 				return err
 			}
-			if err := o.RunAggregator(wait.NeverStop); err != nil {
+			if err := o.RunAggregator(stopCh); err == nil {


err == nil

Why?

sttts · 2017-08-10T08:49:35Z

@crassirostris can you create a PR against this one to actually send the batched events?

sttts · 2017-08-10T08:50:43Z

@crassirostris either against this one or a follow-up.

Btw, I tested this manually and I see some shutdown messages (possibly from informers).

@ALL: If anybody has a Windows machine, please test this there as well.

crassirostris · 2017-08-10T08:52:11Z

@sttts Sure, I'll try to do this by the EOD or EOW

crassirostris · 2017-08-10T08:53:35Z

Thanks a lot for doing this work!

frobware

Did you try to repeatedly bang on Ctrl-C (i.e., generating multiple SIGINT's, for example)?

frobware · 2017-08-10T08:54:01Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

+		shutdownStopCh := make(chan struct{})
+		go func() {
+			defer close(shutdownStopCh)
+			<-time.After(time.Minute)


This is just temporary, correct? Otherwise 1 minute seems very arbitrary.

Yes. We can also leave that out and trust a SIGKILL following after some time anyway. @crassirostris @soltysh opinions?

Let's start with this and TODO, once we reach a point where we'll gather some feedback we can decide what to do with it further.

I prefer leaving this out, for the exact reason @sttts mentioned: in most setups SIGKILL will follow anyway

Since I can't even figure out why this is here, a comment is very necessary.

In fact we decided to remove it. Will do.

sttts · 2017-08-10T09:23:41Z

Did you try to repeatedly bang on Ctrl-C (i.e., generating multiple SIGINT's, for example)?

No. I wonder what the usual behavour is in those case? Directly calling os.Exit on the second SIGINT?

frobware · 2017-08-10T09:41:21Z

Calling os.Exit(?) on subsequent SIGINTs is probably better than trying to call the teardown sequence again, unless it can handle being repeatedly called. That seems a harder ask.

As a user you just have to expect that a Ctrl-C might take some time; repeatedly hitting Ctrl-C will be a harsher exit.

soltysh

A few nits, overall this LGTM! Thanks Stefan!!!

soltysh · 2017-08-10T09:03:44Z

cmd/hyperkube/hyperkube.go

-		hk.Println("Error:", err)
+	if !s.RespectsStopCh {
+		// for command that do not respect the stopCh, we run them in a go
+		// routine while leaving them running while when stopCh is closed.


running ~~while~~ when

soltysh · 2017-08-10T10:05:18Z

staging/src/k8s.io/apiserver/pkg/server/signal.go

@@ -0,0 +1,55 @@
+/*
+Copyright 2014 The Kubernetes Authors.


soltysh · 2017-08-10T10:06:14Z

staging/src/k8s.io/apiserver/pkg/server/signal.go

+)
+
+// SetupSignalHandler registered for SIGTERM and SIGINT. A stop channel is returned
+// which is closed on one of these signals. A exitCode channel is returned for the program


An exitCode

soltysh · 2017-08-10T10:13:30Z

cmd/hyperkube/hyperkube.go

+	exitCode := 1
+	defer func() { exitCodeCh <- exitCode }()
+
+	err := hk.Run(args, stopCh)


Nit, but I like when these kind of things are written with the short-if. Ask @pweil-, he calls me short-if 👮

Btw. I'm surprised we don't print that error here anywhere, like we do in most cases.

soltysh · 2017-08-10T10:18:23Z

staging/src/k8s.io/apiextensions-apiserver/main.go

+	if err := cmd.Execute(); err == nil {
+		exitCode = 0
+	} else {
+		glog.Error(err.Error())


That's nice, your SetupSignalHandler also nicely takes care of exiting with code != 0 :)

soltysh · 2017-08-10T10:20:23Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

+		shutdownStopCh := make(chan struct{})
+		go func() {
+			defer close(shutdownStopCh)
+			<-time.After(time.Minute)


Let's start with this and TODO, once we reach a point where we'll gather some feedback we can decide what to do with it further.

soltysh · 2017-08-10T10:35:02Z

@frobware @sttts I'm not sure you're gonna get multiple SIGINTs here.

sttts · 2017-08-10T10:53:12Z

@frobware @crassirostris @soltysh thanks for the quick review. Addressed all comments. ptal

crassirostris · 2017-08-10T12:05:11Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

+		shutdownStopCh := make(chan struct{})
+		go func() {
+			defer close(shutdownStopCh)
+			<-time.After(time.Minute)


I prefer leaving this out, for the exact reason @sttts mentioned: in most setups SIGKILL will follow anyway

crassirostris · 2017-08-10T12:11:14Z

staging/src/k8s.io/apiserver/pkg/server/signal.go

+			}
+		case code := <-exit:
+			close(stop)
+			os.Exit(code)


Doesn't that create a race between main function returning 0 and the os.Exit call?

Good catch. Fixing.

Wondering why I did this exit channel logic at all. ptal.

crassirostris · 2017-08-10T12:51:01Z

cmd/hyperkube/hyperkube.go

@@ -174,7 +176,22 @@ func (hk *HyperKube) Run(args []string) error {
 	logs.InitLogs()
 	defer logs.FlushLogs()

-	err = s.Run(s, s.Flags().Args())
+	if !s.RespectsStopCh {
+		// for commands that do not respect the stopCh, we run them in a go


Small nit: capitalize the first letter for conformity.

crassirostris · 2017-08-10T12:59:26Z

staging/src/k8s.io/apiserver/pkg/server/signal.go

+	stop := make(chan struct{})
+	c := make(chan os.Signal, 2)
+	signal.Notify(c, shutdownSignals...)
+	go func() {


Nit: will this work?

<-c close(stop) <-c os.Exit(1)

Slightly nicer 👍

crassirostris · 2017-08-10T13:00:52Z

staging/src/k8s.io/kube-aggregator/main.go

 	cmd.Flags().AddGoFlagSet(flag.CommandLine)
 	if err := cmd.Execute(); err != nil {
-		panic(err)
+		glog.Fatal(err.Error())


err.Error()

Is it necessary? Can it be just glog.Fatal(err)?

right, Fatalf wants a string. Fatal is generic.

crassirostris · 2017-08-10T13:01:30Z

staging/src/k8s.io/sample-apiserver/main.go

 	cmd.Flags().AddGoFlagSet(flag.CommandLine)
 	if err := cmd.Execute(); err != nil {
-		panic(err)
+		glog.Fatal(err.Error())


Same as above

crassirostris

Thanks!

sttts · 2017-08-10T17:28:41Z

@crassirostris @soltysh here is still a slight chance to drop events. To be really sure we want to disable the http handlers and at kill all existing connections. But that's follow-up. At least we have a chance to implement this now.

crassirostris · 2017-08-10T17:34:28Z

@sttts

To be really sure we want to disable the http handlers and at kill all existing connections

You mean some requests may go through not audited while auditing is being shut down?

At least we have a chance to implement this now

Yes, thanks again for working on this :)

sttts · 2017-08-11T16:14:29Z

/assign @smarterclayton

@smarterclayton is it approved?

sttts · 2017-08-14T08:32:44Z

/retest

smarterclayton · 2017-08-14T17:09:05Z

/approve

sttts · 2017-08-14T19:42:27Z

/approve no-issue

k8s-github-robot · 2017-08-14T19:42:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, smarterclayton, sttts

Associated issue requirement bypassed by: sttts

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [smarterclayton,sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [smarterclayton,sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/kube-gen/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [smarterclayton,sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-08-14T23:41:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-15T02:29:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-15T05:17:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

sttts · 2017-08-15T07:15:51Z

@mikedanese there is something odd with the signal_windows.go file, compare https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/50439/pull-kubernetes-verify/45239/. Our update-bazel.sh script on Mac will not create that block.

k8s-github-robot · 2017-08-15T08:34:00Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-15T09:27:06Z

Automatic merge from submit-queue

@tallclair

Automatic merge from submit-queue advanced audit: shutdown batching audit webhook gracefully Follow-up of #50439 When the `stopCh` passed to the batching audit webhook is closed, it stops accepting new events and when `Shutdown` method is called afterwards, it blocks until the last request to the webhook has finished. /cc @tallclair @soltysh

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2017

k8s-github-robot assigned mikedanese and deads2k Aug 10, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 10, 2017

sttts assigned soltysh and crassirostris and unassigned mikedanese Aug 10, 2017

crassirostris reviewed Aug 10, 2017

View reviewed changes

frobware reviewed Aug 10, 2017

View reviewed changes

soltysh approved these changes Aug 10, 2017

View reviewed changes

crassirostris reviewed Aug 10, 2017

View reviewed changes

crassirostris approved these changes Aug 10, 2017

View reviewed changes

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2017

sttts force-pushed the sttts-shutdown-apiservers branch from 81879e2 to a47779e Compare August 10, 2017 17:18

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2017

sttts force-pushed the sttts-shutdown-apiservers branch from a47779e to f998764 Compare August 10, 2017 17:25

k8s-github-robot added the release-note-label-needed label Aug 10, 2017

sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 10, 2017

sttts force-pushed the sttts-shutdown-apiservers branch from c88f3b1 to f9f7625 Compare August 11, 2017 07:26

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2017

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2017

sttts force-pushed the sttts-shutdown-apiservers branch from f9f7625 to f1c10c1 Compare August 11, 2017 16:13

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2017

k8s-ci-robot assigned smarterclayton Aug 11, 2017

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2017

crassirostris mentioned this pull request Aug 13, 2017

advanced audit: shutdown batching audit webhook gracefully #50577

Merged

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2017

sttts force-pushed the sttts-shutdown-apiservers branch from f1c10c1 to 5a75be1 Compare August 15, 2017 07:15

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 15, 2017

apiservers: add synchronous shutdown mechanism on SIGTERM+INT

11b2536

sttts force-pushed the sttts-shutdown-apiservers branch from 5a75be1 to 11b2536 Compare August 15, 2017 07:17

sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 15, 2017

k8s-github-robot merged commit 4d6db74 into kubernetes:master Aug 15, 2017

crassirostris mentioned this pull request Aug 16, 2017

Graceful shutdown of batching audit webhook #50026

Closed

apiservers: add synchronous shutdown mechanism on SIGTERM+INT #50439

apiservers: add synchronous shutdown mechanism on SIGTERM+INT #50439

Conversation

sttts commented Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Aug 10, 2017

sttts commented Aug 10, 2017

crassirostris commented Aug 10, 2017

crassirostris commented Aug 10, 2017

frobware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Aug 10, 2017

frobware commented Aug 10, 2017

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 10, 2017

sttts commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris left a comment

Choose a reason for hiding this comment

sttts commented Aug 10, 2017

crassirostris commented Aug 10, 2017

sttts commented Aug 11, 2017

sttts commented Aug 14, 2017

smarterclayton commented Aug 14, 2017

sttts commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

fejta-bot commented Aug 14, 2017

fejta-bot commented Aug 15, 2017

fejta-bot commented Aug 15, 2017

sttts commented Aug 15, 2017

k8s-github-robot commented Aug 15, 2017

k8s-github-robot commented Aug 15, 2017

sttts commented Aug 10, 2017 •

edited

Loading