Support graceful shutdown on both HTTP/1.1 and HTTP/2 #2106

maskit · 2017-06-09T07:38:00Z

For HTTP/1.1, add Connection: close header
For HTTP/2, send GOAWAY frames

Generalize the flag name
Close listening sockets when graceful shutdown has been scheduled
Send the GOAWAY frame if a response header hasConnection: close

The last change also fixes an inconsistent behavior that H2 sessions won't be closed even if Connection: close header is set by a plugin.

(This still depends on proxy.config.stop.shutdown_timeout. I'm going to add support for traffic_ctl stop --drain later.)

maskit · 2017-06-09T07:44:49Z

@mlibbey This will enable to shed H2 traffic with header_rewrite. (#1693)

maskit · 2017-06-09T08:03:14Z

@zizhong Please take a look.

zwoop · 2017-06-09T16:11:44Z

@maskit This is a 7.1.0 candidate?

jpeach

Review placeholder :)

I'll try to look this weekend.

maskit · 2017-06-09T16:48:17Z

@zwoop I marked this 8.0 because of necessity but that should be possible. We'll need #1710, #1937 and #2094 if we backport this.

zwoop · 2017-06-09T16:49:52Z

@maskit I trust your judgement here, let me know what you think is the right thing to do. My goal right now is to make 7.1.0 rock solid, and since we're working on other issues, there's still time.

maskit · 2017-06-09T16:55:37Z

@zwoop Then I'd say yes.

Although it's not merged yet, we'll need #2096 too.

jpeach · 2017-06-11T04:03:09Z

proxy/http/HttpProxyServerMain.cc

+{
+  sslNetProcessor.stop_accept();
+  netProcessor.stop_accept();
+}


This doesn't work correctly. If you are running under traffic_manager then what happens is that the client connection just hangs because traffic_manager is still listening on the sockets.

Refusing new connections while draining should absolutely be configurable. Many configurations need to continue to serve clients while the GSLB directs traffic elsewhere. @zwoop.

I saw an assert at one point:

traffic_server: using root directory '/opt/ats' [Jun 10 20:51:40.206] Server {0x7fd9da1b8700} DEBUG: <Main.cc:417 (periodic)> (server) limiting connections based on memory usage has been disabled [Jun 10 20:52:28.020] Server {0x7fd9da3bc700} DEBUG: <Main.cc:285 (periodic)> (server) received exit signal, shutting down in 30secs Fatal: UnixNetVConnection.cc:1401: failed assertion `con.fd == NO_FD` traffic_server: received signal 6 (Aborted) traffic_server - STACK TRACE: /opt/ats/bin/traffic_server(_Z19crash_logger_invokeiP9siginfo_tPv+0xc3)[0x504c72] /lib64/libpthread.so.0(+0x115c0)[0x7fd9ddb065c0] /lib64/libc.so.6(gsignal+0x9f)[0x7fd9dccb891f] /lib64/libc.so.6(abort+0x16a)[0x7fd9dccba51a] /opt/ats/lib/libtsutil.so.8(_Z11ink_warningPKcz+0x0)[0x7fd9df2d58f2] /opt/ats/lib/libtsutil.so.8(_Z17ats_base64_encodePKhmPcmPm+0x0)[0x7fd9df2d2d65] /opt/ats/bin/traffic_server(_ZN18UnixNetVConnection4freeEP7EThread+0x2d9)[0x7a8425] /opt/ats/bin/traffic_server(_ZN18UnixNetVConnection11acceptEventEiP5Event+0x16d)[0x7a7049] /opt/ats/bin/traffic_server(_ZN12Continuation11handleEventEiPv+0x72)[0x507de6] /opt/ats/bin/traffic_server(_ZN7EThread13process_eventEP5Eventi+0x134)[0x7ca102] /opt/ats/bin/traffic_server(_ZN7EThread7executeEv+0x10f)[0x7ca391]

Refusing new connections while draining should absolutely be configurable. Many configurations need to continue to serve clients while the GSLB directs traffic elsewhere.

I'm fine with keeping sockets open but I think it should have been done before starting graceful shutdown. If we don't close sockets, number of active connections won't goes down without external traffic control.

This doesn't work correctly. If you are running under traffic_manager then what happens is that the client connection just hangs because traffic_manager is still listening on the sockets.

Right. Is it OK if I close sockets only if traffic_manager isn't used ? This PR doesn't include support for traffic_manager.

(This still depends on proxy.config.stop.shutdown_timeout. I'm going to add support for traffic_ctl stop --drain later.)

It was not clear. I mean another Pull Request.

As for the assert, it seems the NetVConnection has to be closed with close_UnixNetVConnection() before calling its free. Otherwise, net_connections_currently_open_stat won't be decremented.

If we don't close sockets, number of active connections won't goes down
without external traffic control.

@maskit That's right. If you are running a CDN you have external load balancing, so you don't need to give errors to clients. I can see the utility of refusing connections, but it should not be the default.

If you are running a CDN you have external load balancing

What if you aren't?

jpeach · 2017-06-11T04:05:37Z

proxy/ProxyClientSession.h

+  is_draining() const
+  {
+    return http_client_session_draining;
+  }


I don't think there's any value in disguising the global variable here. Just use it directly where you need it.

I wrapped it because the variable will be replaced with a metric you suggested.

jpeach · 2017-06-11T04:06:07Z

proxy/ProxyClientSession.h

 #include "InkAPIInternal.h"
 #include "http/HttpServerSession.h"

+extern volatile bool http_client_session_draining;


There should be a metric that publishes this state so that operators can tell we are in draining state.

Can I left that as a future work? Because main purpose of this PR is generalizing the flag and supporting graceful shutdown on HTTP/1.1. This is just a replacement of http2_drain.

jpeach · 2017-06-11T04:09:08Z

proxy/http2/Http2ConnectionState.h

 };

-enum Http2ShutdownState { NOT_INITIATED, INITIATED, IN_PROGRESS };
+enum Http2ShutdownState { NOT_PLANNED, NOT_INITIATED, INITIATED, IN_PROGRESS };


What do these states mean? I can't figure out from the usage what you intend here.

These enums should be renamed to be consistent with the rest of the code. I think that all you need are:

enum Http2ShutdownState { HTTP2_SHUTDOWN_NONE, HTTP2_SHUTDOWN_BEGIN, HTTP2_SHUTDOWN_INPROGRESS };

NOT_PLANNED: Shutdown is not planned (it'll keep working)
NOT_INITIATED: Shutdown is planned but nothing is happened yet (The 1st GOAWAY will be sent)
INITIATED: The first GOAWAY is sent and waiting the 2sec before sending the 2nd GOAWAY
IN_PROGRESS: The second GOAWAY is sent and waiting shutdown timeout

I'll rename them. Let me know if you have better names.

Let's start by making them consistent with other enums, so:

HTTP2_SHUTDOWN_NONE, HTTP2_SHUTDOWN_NOT_INITIATED, HTTP2_SHUTDOWN_INITIATED, HTTP2_SHUTDOWN_IN_PROGRESS

Would you be able to make another PR later that reduces this down to just 3 values?

It should be possible if I change the conditions, but I guess the condition will be a bit complex.
I'll make them consistent for now.

jpeach · 2017-06-11T04:18:37Z

proxy/http2/Http2Stream.h

  void reenable(VIO *vio);
  virtual void transaction_done();
+  virtual bool
+  ignore_keep_alive()


bool ignore_keep_alive() override { ... }

Will do after #2113. (Many warnings show up.)

jpeach · 2017-06-11T04:19:01Z

proxy/http2/Http2Stream.h

+  virtual bool
+  ignore_keep_alive()
+  {
+    return false;


This needs a comment explaining it.

Honestly, I don't understand why it was true. Without this change, Connection: close is always added (and is removed while converting headers from 1.1 to 2). I think it should be handled as the same as HTTP/1.1.

OK, sounds good. Just comment the explanation?

jpeach · 2017-06-11T04:25:21Z

proxy/http2/Http2ConnectionState.h

 };

-enum Http2ShutdownState { NOT_INITIATED, INITIATED, IN_PROGRESS };
+enum Http2ShutdownState { NOT_PLANNED, NOT_INITIATED, INITIATED, IN_PROGRESS };


Looking at the code for handling HTTP2_SESSION_EVENT_SHUTDOWN_INIT, is it safe to schedule the HTTP2_SESSION_EVENT_SHUTDOWN_CONT in 2 seconds? What guarantees that the Http2ConnectionState is still alive at that time?

Not sure, but you can say the same thing for other events, it should be guaranteed though. It's not special.

There's only one other schedule_in in HTTP/2 and that keeps hold of the returned Action so it can be canceled. I don't this this usage is safe.

I don't get it. Are you suggesting any of these?

Keep Http2ConnectionState alive until the event processed

Make it cancelable and cancel the event when Http2ConnectoinState dies

Is it guaranteed that Http2ConnectionState is still alive when HTTP2_SESSION_EVENT_RECV is going to be processed?

I just don't understand how this schedule_in is safe. If the object is destroyed before the 2sec timeout, the event will run on a freed object AFAICT. I think you have to make it cancelable and cancel the event when Http2ConnectionState dies.

The HTTP2_SESSION_EVENT_RECV is different, since send_connection_event() delivers the event directly, so it is reentrant on the event handler.

Ok, I understand your point.

HTTP2_SESSION_EVENT_RECV was a bad example :) I wanted to know how other places guarantee the continuation is still alive. Anyway, I'll look into it and try to make it solid.

jpeach · 2017-06-13T03:58:46Z

@zwoop @maskit I don't think this is a backport candidate until the whole feature is complete. I don't want us to get stuck on something just for compatibility reasons.

maskit · 2017-06-13T06:04:25Z

Even if I couldn't complete the whole feature, I think most changes we have already merged on master (and code on this PR) is still valid. HTTP/2 sessions should be closed if (proxy's) response headers have Connection: close. I'd rather remove only the shutdown part than postpone all the changes to 8.0.

maskit · 2017-06-13T07:42:55Z

Done

Made enum names consistent
Added override
Explained why ignore_keep_alive() should return false
Removed code that closes listening sockets
- It refuses new connections on SessionAccept instead (and it's not configurable for now)

WIP

Cancel HTTP2_SESSION_EVENT_SHUTDOWN_CONT event when Http2ConnectionState dies

Need discussion

Whether refuse new connections while draining

maskit · 2017-06-13T08:10:31Z

I don't see any reason to not refuse new connections while draining. Refusing new connections might be overkill for users who use load balancer, but if ATS doesn't refuse connections, draining feature will be only for users who use load balancer (or some external controller). You can remove servers from load balancer before starting draining so I think it shouldn't be a problem even if the behavior is not configurable.

zwoop · 2017-06-13T12:23:09Z

Moving out to 8.0.0 for now, since there seems to be some contention on this topic. :)

jpeach · 2017-06-13T15:27:06Z

On Jun 13, 2017, at 1:10 AM, Masakazu Kitajo ***@***.***> wrote: I don't see any reason to not refuse new connections while draining.

It definitely has to be configurable.

Refusing new connections might be overkill for users who use load balancer, but if ATS doesn't refuse connections, draining feature will be only for users who use load balancer (or some external controller).

That's everyone. You have to manage getting the traffic into ATS somehow :) Also, if you refuse connections you have to actually refuse them; you can't leave them to time out in the listen backlog and you can't shut them down once you have accepted them.

You can remove servers from load balancer before starting draining so I think it shouldn't be a problem even if the behavior is not configurable.

Not if you are using a DNS-based GSLB.

maskit · 2017-06-13T16:32:39Z

You have to manage getting the traffic into ATS somehow :)

Can we assume that all ATS operators can control it? If the answer was yes, why is it need to be configurable?

Also, if you refuse connections you have to actually refuse them;

I agree. That's why I tried to close listening sockets. So, is it OK to close the sockets from TS if TS isn't managed by TM (and users want it) ?

Not if you are using a DNS-based GSLB.

Why not? Isn't it possible to remove servers? I'm not familiar with DNS-based GLSB.

bryancall · 2017-06-14T15:56:09Z

[approve ci autest]

zwoop · 2017-07-20T11:18:23Z

@maskit @masaori335 Where are we with this? Is this still a WIP, or getting ready for review / merge? If so, please remove the WIP label.

maskit · 2017-07-20T11:42:33Z

It's ready for review.

bryancall · 2017-08-16T16:42:10Z

This needs to be reviewed.

zizhong · 2017-08-18T20:55:00Z

I'm thinking about adding tests. @maskit do you have any good idea about how to test this?

maskit · 2017-08-19T08:54:59Z

@zizhong I think autest would be an option.

We would need an origin server ( or a plugin) which returns a response with few seconds of delay without Connection: close.

The test would be something like:

Set proxy.config.stop.shutdown_timeout to 10 sec
Launch Traffic Server
Send an HTTP request
Send SIGTERM to the process
Check if the response has Connection: close
Wait for 10 sec
Check if the process doesn't exist

I think we don't have any ways to test this behavior on HTTP/2.

zizhong · 2017-08-21T20:08:58Z

@maskit Something more useful is that ATS sends the GOAWAY frame if a response header has Connection: close no matter the draining started or not. This will enable a plugin to initiate the graceful shutdown process for both HTTP/1 and HTTP/2.

maskit · 2017-08-22T00:02:17Z

@zizhong That logic is already implemented :)
https://github.com/apache/trafficserver/pull/2106/files#diff-83845675d9d6c1da389e9e0cdbe4ceb3R565

maskit · 2017-10-25T07:01:53Z

Rebased on the latest master. The code hasn't been changed since Jul 20. I just resolved conflicts.

masaori335

Looks good. One minor change is required. Also commits should be squashed in few commits.

masaori335 · 2017-11-02T00:54:40Z

proxy/ProxyClientSession.cc

 #include "HttpDebugNames.h"
 #include "ProxyClientSession.h"

+volatile bool http_client_session_draining = false;


We removed volatile keyword by #2603. Should we use std::atomic<bool>?

For HTTP/1.1, add Connection: close header For HTTP/2, send GOAWAY frames

maskit · 2017-11-06T05:36:03Z

Made a small change.

       RecInt timeout = 0;
-      if (RecGetRecordInt("proxy.config.stop.shutdown_timeout", &timeout) == REC_ERR_OKAY && timeout) {
+      if (RecGetRecordInt("proxy.config.stop.shutdown_timeout", &timeout) == REC_ERR_OKAY && timeout &&
+          !http_client_session_draining) {
         http_client_session_draining = true;

maskit · 2017-11-06T06:34:10Z

@jpeach Can you take another look?

shinrich

Just noticed this PR. Looks great! We had an internal issue filed on using the GOAWAY to make HTTP/2 draining go faster. This is a superset of that issue.

maskit · 2017-12-05T01:59:10Z

[approve ci]

zizhong · 2017-12-05T23:43:42Z

doc/admin-guide/files/records.config.en.rst

   This setting specifies the number of active client connections
   for use by :option:`traffic_ctl server restart --drain`.

+.. ts:cv:: CONFIG proxy.config.restart.stop_listening INT 0


@maskit
Tests are needed for both HTTP/1 draining and HTTP/2 graceful shutdown. They can help you when you add "traffic_ctl --drain" feature.
For how to test it in HTTP/2, you can check out this gist. https://gist.github.com/zizhong/401bf0618c0a977d0cd7e1042ee632fc

Thanks for the test. Nice job. I tested it manually but we should automate it, and the test is helpful even without supporting "traffic_ctl --drain". Can you make the test autest compatible and submit a PR?

Also, as you know, the GOAWAY frame for graceful shutdown has the unique stream id. So, it would be great if the test checks the stream id.

Sure. Will do.

I think the requested changes (at least on this PR) are already addressed.

maskit · 2017-12-13T05:08:00Z

@jpeach I dismissed your change request because I think they are already addressed. Please file issues if there's something I missed.

maskit added HTTP HTTP/2 labels Jun 9, 2017

maskit added this to the 8.0.0 milestone Jun 9, 2017

maskit self-assigned this Jun 9, 2017

maskit requested review from jpeach and masaori335 June 9, 2017 07:38

maskit force-pushed the drain branch from f2b468b to 3745582 Compare June 9, 2017 07:53

jpeach requested changes Jun 9, 2017

View reviewed changes

maskit modified the milestones: 7.1.0, 8.0.0 Jun 9, 2017

maskit added the Backport Marked for backport for an LTS patch release label Jun 9, 2017

jpeach previously requested changes Jun 11, 2017

View reviewed changes

maskit force-pushed the drain branch from 3745582 to 070f0de Compare June 13, 2017 07:32

maskit added the WIP label Jun 13, 2017

zwoop modified the milestones: 8.0.0, 7.1.0 Jun 13, 2017

maskit removed WIP Backport Marked for backport for an LTS patch release labels Jul 20, 2017

maskit force-pushed the drain branch from 2ba016e to 77b9dc3 Compare October 25, 2017 05:53

masaori335 requested changes Nov 2, 2017

View reviewed changes

maskit force-pushed the drain branch from 9229c24 to c350f9f Compare November 2, 2017 01:48

masaori335 previously approved these changes Nov 6, 2017

View reviewed changes

Support graceful shutdown on both HTTP/1.1 and HTTP/2

fa6472a

For HTTP/1.1, add Connection: close header For HTTP/2, send GOAWAY frames

maskit dismissed masaori335’s stale review via fa6472a November 6, 2017 05:34

maskit force-pushed the drain branch from c350f9f to fa6472a Compare November 6, 2017 05:34

masaori335 approved these changes Nov 6, 2017

View reviewed changes

shinrich approved these changes Nov 30, 2017

View reviewed changes

zizhong reviewed Dec 5, 2017

View reviewed changes

zizhong mentioned this pull request Dec 7, 2017

improve draining #2906

Closed

zizhong approved these changes Dec 13, 2017

View reviewed changes

maskit merged commit 6ab6a78 into apache:master Dec 13, 2017

maskit mentioned this pull request Dec 13, 2017

Fix a build issue due to recent variable name change #2920

Merged

shinrich mentioned this pull request Jan 31, 2018

Do not propagate origin Connection:close to client http/2 session. #3061

Closed

Support graceful shutdown on both HTTP/1.1 and HTTP/2 #2106

Support graceful shutdown on both HTTP/1.1 and HTTP/2 #2106

Uh oh!

Conversation

maskit commented Jun 9, 2017

Uh oh!

maskit commented Jun 9, 2017

Uh oh!

maskit commented Jun 9, 2017

Uh oh!

zwoop commented Jun 9, 2017

Uh oh!

jpeach left a comment

Choose a reason for hiding this comment

Uh oh!

maskit commented Jun 9, 2017

Uh oh!

zwoop commented Jun 9, 2017

Uh oh!

maskit commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maskit Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpeach commented Jun 13, 2017

maskit Jun 13, 2017 •

edited

Loading

masaori335 left a comment •

edited

Loading