webhook EOF errors

[mattmoor]

/area API
/area test-and-release
/kind bug

Expected Behavior

When we run our e2e tests with chaos there are no failures due to the webhook shutting down.

Actual Behavior

We intermittently see failures like this: Post https://eventing-webhook.knative-eventing-qh1fjbnng8.svc:443/resource-conversion?timeout=30s: EOF ever since we enabled webhook chaos.

[mattmoor]

cc @vaikas

[mattmoor]

I already made two changed to try and mitigate this:

1. Bumped the terminationGracePeriodSecond in our webhook pods to 300 to give the webhook itself greater control over the drain duration.
2. Bumped the network.DefaultDrainTimeout: Bump the drain timeout #1501 to give the API Server more time to observe the endpoints change.

[mattmoor]

From the Go http.Server Shutdown documentation:

When Shutdown is called, Serve, ListenAndServe, and ListenAndServeTLS immediately return ErrServerClosed. Make sure the program doesn't exit and waits instead for Shutdown to return.

I wondered if this might be our problem, but we call ListenAndServeTLS here:

pkg/webhook/webhook.go

Lines 207 to 210 in f1b8240

	eg.Go(func() error {
	if err := server.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed {
	logger.Errorw("ListenAndServeTLS for admission webhook returned error", zap.Error(err))
	return err

and seem to properly check for ErrServerClosed

[mattmoor]

I started a thread in #sig-api-machinery here: https://kubernetes.slack.com/archives/C0EG7JC6T/p1594910493127400

[mattmoor]

Alright, so one of the things I have been wondering is around Keep-Alives and whether that might be a reason the API server takes so long to realize an endpoint is no longer good. I noticed the following comment on Go's SetKeepAlivesEnabled:

SetKeepAlivesEnabled controls whether HTTP keep-alives are enabled. By default, keep-alives are always enabled. Only very resource-constrained environments or servers in the process of shutting down should disable them.

... emphasis mine.

Now it turns out that Go's http logic is already smart about disabling keep-alive when it is shutting down:

func (s *Server) doKeepAlives() bool {
	return atomic.LoadInt32(&s.disableKeepAlives) == 0 && !s.shuttingDown()
}

However, when we lame duck we aren't yet .shuttingDown() so I believe keep-alives will continue throughout our network.DefaultDrainTimeout until Shutdown() is actually called.

I believe testing this should be as simple as calling server.SetKeepAlivesEnabled(false) here:

pkg/webhook/webhook.go

Line 221 in 25f2aa6

However, if this works then we should probably consider doing something similar across our various dataplane components as well. cc @tcnghia

[mattmoor]

We believe that @vaikas is still seeing some webhook failures that should be synced past the above change, so we aren't out of the woods yet.