Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activator should wait for active requests to drain before terminating #4654

Closed
mattmoor opened this issue Jul 9, 2019 · 4 comments · Fixed by #4671
Closed

Activator should wait for active requests to drain before terminating #4654

mattmoor opened this issue Jul 9, 2019 · 4 comments · Fixed by #4671
Assignees
Labels
area/autoscale area/networking kind/feature Well-understood/specified features, ready for coding.
Milestone

Comments

@mattmoor
Copy link
Member

mattmoor commented Jul 9, 2019

In what area(s)?

/area autoscale
/area networking
/kind good-first-issue

Describe the feature

The activator receives the stop signal here, we should hook in a new activation handler here that manages a sync.WaitGroup{} that increments/decrements on new requests, and then does a wg.Wait() after the stopCh signal is received.

@mattmoor mattmoor added the kind/feature Well-understood/specified features, ready for coding. label Jul 9, 2019
@mattmoor mattmoor added this to the Serving 0.8 milestone Jul 9, 2019
@vagababov
Copy link
Contributor

From the Server.Shutdown docs:

Shutdown gracefully shuts down the server without interrupting any active connections. Shutdown works by first closing all open listeners, then closing all idle connections, and then waiting indefinitely for connections to return to idle and then shut down. 

So, the way I read it, once we call shutdown

  1. we won't get any new requests
  2. all the current ones will be processed

Thus, I think this issue is unnecessary?

@markusthoemmes
Copy link
Contributor

Agree with @vagababov. Have we seen this not working as expected?

@mattmoor
Copy link
Member Author

mattmoor commented Jul 9, 2019

You may be right, but two concerns come to mind:

  1. What happens with streaming connections (e.g. websocket or GRPC)?
  2. Even once the code handles graceful termination, I'm concerned that terminationGracePeriodSeconds is too low for our activator pods (read: it is the default of 30s), which is much shorter than even our default request timeout.

What I describe above is likely handled by the Shutdown internally (great!), but we likely still need some config for 2.

vagababov added a commit to vagababov/serving that referenced this issue Jul 9, 2019
This will permit us to let activator linger longer, before K8s forcefully kills it
to process the requests that might take more time (e.g. streaming).

/assign @mattmoor

For knative#4654
@tcnghia
Copy link
Contributor

tcnghia commented Jul 9, 2019

/assign @vagababov

looks like this will be closed by #4671

knative-prow-robot pushed a commit that referenced this issue Jul 10, 2019
This will permit us to let activator linger longer, before K8s forcefully kills it
to process the requests that might take more time (e.g. streaming).

/assign @mattmoor

For #4654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/autoscale area/networking kind/feature Well-understood/specified features, ready for coding.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants