-
Notifications
You must be signed in to change notification settings - Fork 844
Support graceful shutdown on both HTTP/1.1 and HTTP/2 #2106
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,8 @@ | |
| #include "InkAPIInternal.h" | ||
| #include "http/HttpServerSession.h" | ||
|
|
||
| extern bool http_client_session_draining; | ||
|
|
||
| // Emit a debug message conditional on whether this particular client session | ||
| // has debugging enabled. This should only be called from within a client session | ||
| // member function. | ||
|
|
@@ -120,6 +122,12 @@ class ProxyClientSession : public VConnection | |
| return m_active; | ||
| } | ||
|
|
||
| bool | ||
| is_draining() const | ||
| { | ||
| return http_client_session_draining; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's any value in disguising the global variable here. Just use it directly where you need it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrapped it because the variable will be replaced with a metric you suggested. |
||
|
|
||
| // Initiate an API hook invocation. | ||
| void do_api_callout(TSHttpHookID id); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -335,3 +335,10 @@ start_HttpProxyServerBackDoor(int port, int accept_threads) | |
| // The backdoor only binds the loopback interface | ||
| netProcessor.main_accept(new HttpSessionAccept(ha_opt), NO_FD, opt); | ||
| } | ||
|
|
||
| void | ||
| stop_HttpProxyServer() | ||
| { | ||
| sslNetProcessor.stop_accept(); | ||
| netProcessor.stop_accept(); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work correctly. If you are running under 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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right. Is it OK if I close sockets only if traffic_manager isn't used ? This PR doesn't include support for traffic_manager.
It was not clear. I mean another Pull Request.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for the assert, it seems the NetVConnection has to be closed with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What if you aren't? |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Will do.