Re: [apache/trafficserver] HTTP2 drain (#1710)

[James Peach <jp...@apache.org>]

On 9 May 2017, at 0:02, Masakazu Kitajo wrote:

> Hi,
>
> Don't worry guys. I'm working on it.
>
> I merged the PR to have a draining (shedding) logic for HTTP/2 (Double
> GOAWAY frames), and now I'm generalizing the flag so that we can 
> invoke
> draining on both HTTP/1 and HTTP/2. I'm going to add "Connection: 
> close"
> header automatically for HTTP/1. So, 
> "proxy.config.stop.shutdown_timeout"
> will be available for both versions. The PR was step 1, and it will be 
> step
> 2.

“proxy.config.stop.shutdown_timeout” is still a mis-feature. What 
really matters for shutting down is how much traffic you are still 
serving, which is expressed by 
“proxy.config.restart.active_client_threshold”. I think you might 
also want to express a maximum time you are willing to wait, but just 
having a fixed time is not what we should be doing.

> "traffic_ctl server stop --drain" is also in my mind. Once we have one
> generalized way for draining in traffic_server, we can invoke it from
> anywhere we want (e.g. from traffic_manager, TS API, metric). 
> Currently it
> is invoked using the flag, but eventually, we should be able to 
> decouple
> draining and shutdown, and should also be able to remove the flag and
> sleep().
>
> Doing these all at once is heavy and the PR would be big, and it slows
> review and progress, IMO. You might want full featured graceful 
> shutdown
> but this is a reason why I didn't ask to do it.

I’m fine with (and even prefer) doing this in pieces, but we need to 
agree on the direction and that the end result will be what we want.

>
> Thanks,
> Masakazu
>
>
>
> On Tue, May 9, 2017 at 2:40 PM, CrazyCow <zh...@gmail.com> 
> wrote:
>
>> I don't disagree. The reasons I chose this way are:
>> 1. We are using other stuff in my team instead of traffic_ctl to 
>> manage the
>> process and do the upgrade.
>> 2. traffic_ctl --drain can only support HTTP and it can only be used 
>> when
>> restarting ATS. That makes it hardly useful in our use case.
>>
>> 2017-05-08 22:16 GMT-07:00 James Peach <jp...@apache.org>:
>>
>>>
>>>
>>> On 8 May 2017, at 21:48, Miles Libbey wrote:
>>>
>>> We'd also like a bit more fine grained control in the process -- we
>>>> frequently want to perform maintenance on a server (upgrading ATS;
>>>> upgrading the OS; performing hardware changes, etc) after draining 
>>>> but
>>>> before restarting ATS. I suppose this would mean allowing the 
>>>> --drain
>>>> option to apply to traffic_Ctl server stop.
>>>>
>>>
>>> Yes I agree that draining make sense for stop as well as restart.
>>>
>>>
>>>
>>>> miles
>>>>
>>>> On Mon, May 8, 2017 at 8:19 PM, James Peach <jp...@apache.org> 
>>>> wrote:
>>>>
>>>>> This patch adds another separate shutdown mechanism that only 
>>>>> works for
>>>>> HTTP/2. I think that we really ought to have a single, 
>>>>> well-defined
>>>>> graceful
>>>>> shutdown that works for all protocols.
>>>>>
>>>>> In HTTP/1.1 it works like this:
>>>>>         - You (possibly dynamically) set
>>>>> proxy.config.restart.active_client_threshold
>>>>>         - You run “traffic_ctl server restart —drain”
>>>>>
>>>>> Then, once client connections have been drained to the threshold,
>>>>> traffic_server restarts. Note that this assumes that you have some
>>>>> additional orchestration that triggers header_rewrite to inject a
>>>>> “Connection: close” header, and tell the GSLB to stop sending 
>>>>> new
>>>>> connections.
>>>>>
>>>>> After this HTTP/2 change, there is a new graceful shutdown path
>>>>>         - You (at startup only) set 
>>>>> proxy.config.stop.shutdown_timeout
>>>>>         - You send a signal to traffic_server
>>>>>
>>>>> This flips a global variable to the “drain” state, then sleeps 
>>>>> in the
>>>>> signal
>>>>> handler until the timeout is reached. In HTTP/2 only, any new
>> connections
>>>>> will be accepted and then immediately closed.
>>>>>
>>>>> To rationalize these disparate approaches, I suggest that we go 
>>>>> back to
>>>>> the
>>>>> traffic_ctl methodology, and enhance it so that it sends a message 
>>>>> to
>>>>> traffic_server that puts it into a known draining state. This 
>>>>> should be
>>>>> published in a metric and should be reversible so you can abort 
>>>>> the
>>>>> drain.
>>>>> The metric can be observed by HTTP/1.1 and HTTP/2 to take 
>>>>> appropriate
>>>>> action. We should also add a new setting
>>>>> “proxy.config.restart.active_client_timeout” (or something 
>>>>> like that)
>> to
>>>>> handle the maximum time to wait for traffic to drain.
>>>>>
>>>>> I’m not sure whether it is a good idea to close connections in 
>>>>> HTTP/2
>>>>> while
>>>>> we are in draining state. If there is a desire for this, I would 
>>>>> like
>> it
>>>>> to
>>>>> be configurable (defaulting to off).
>>>>>
>>>>> On 8 May 2017, at 18:17, Masakazu Kitajo wrote:
>>>>>
>>>>> Merged #1710.
>>>>>>
>>>>>> --
>>>>>> You are receiving this because you are subscribed to this thread.
>>>>>> Reply to this email directly or view it on GitHub:
>>>>>> https://github.com/apache/trafficserver/pull/1710#event-1073784726
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>