trafficserver-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Carroll <solidwallofc...@verizonmedia.com>
Subject Re: Remap plugin DSO reloading - enabling and disabling
Date Fri, 08 May 2020 18:29:14 GMT
Consider a situation with option (1) with two remap rules:

map http://example.one http://example.one @plugin=txn_box.so @reloadable=false
blah blah blah
map http://example.two http://example.two @plugin=txn_box.so @reloadable=true
blah blah

Does that DSO get reloaded on a reload of "remap.config"?


On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <sudheervinukonda@yahoo.com>
wrote:

> Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have
> reloadable flexibility per remap line either, but just per “(remap)plugin”.
>
> And the only point I was trying to make was to let that the flexibility be
> determined by the user and not implicitly by the fact that a plugin was
> used in mixed mode. And yeah sorry, I totally missed the problem with
> making it a remap level param instead of a plugin level param. So, I still
> prefer your approach 1, except it’d be clearer if it’s named something more
> obvious indicating non-reload ability than “@global” (but, naming is hard
> and I can’t think of a short/succinct better name :()
>
>
>
> On May 8, 2020, at 7:33 AM, Alan Carroll <solidwallofcode@verizonmedia.com>
> wrote:
>
> 
> Sudheer, I understand the point you are making, I just consider it
> irrelevant. Let me give Leif an example to illustrate why - TxnBox. It
> shares data between the global and remap configurations at run time via
> static variables. If you enable remap DSO reloading for TxnBox, it will
> crash on the first transaction that hits a remap rule. It doesn't matter if
> it's actually been reloaded or not. However your organization does plugin
> updates, TxnBox will still crash in that situation. Even in your example,
> Sudheer, there's no _choice_ about whether a particular plugin can be DSO
> reloaded, it's a result of the implementation. As you yourself write, you
> can't enable it for those plugins without changing the code. No
> configuration cleverness will get around that.
>
> For plugins that do support DSO reloading, the enablement is still per
> plugin, not per remap rule. Moreover, if we went with option (3) it would
> be simple to have to plugin support a configuration / load time option to
> enable or disable DSO reloading. In general, if the plugin can be DSO
> reloaded, it's unclear why it shouldn't be except in unusual circumstances
> which are depending on the plugin implementation.
>
> For Sudheer, I remain unclear on what exact flexibility you want, given
> the constraints created by a specific plugin's implementation. I've re-read
> your note and AFAICT it assumes doing DSO reload or not *per plugin*, which
> is also my point. I dislike (1) because it makes no sense to me to have
> this change between remap rules for a specific plugin. I think it's better
> to have the plugin decide if that's possible and, if needed, provide
> configuration to disable it if needed. Speaking specifically for TxnBox, I
> must forbid you from enabling DSO reloading. Even in your case, it might be
> reasonable to have this for plugins that you have not yet updated (which is
> actually the case with TxnBox - I'm limited by a requirement for ATS 7
> compatibility, so I can't change that feature at the current time).
>
> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <zwoop@apache.org> wrote:
>
>>
>>
>> On May 7, 2020, at 8:12 PM, Alan Carroll <
>> solidwallofcode@verizonmedia.com> wrote:
>>
>> Leif;
>>
>> If the plugin can be global or remap but not both, I don't see why (2)
>> limits anything. The entire issue is irrelevant for such plugins, because
>> the situation of reloading the remap DSO but not the global cannot occur,
>> In fact, option (3) or (4) would enable detecting this and issuing a
>> warning.
>>
>>
>> Ah yes, good point. However, still the same problem, one can very much
>> want to use say header_rewrite as both global and remap plugin at the same
>> time, and be fine with the fact that it doesn’t reload as a “global”, but
>> you want it to reload as a remap. We use that plugin in this way for
>> example.
>>
>> I still feel that option 2) is a bad option, but I’m ok with the others
>> (still with a preference towards #1). I think a finer granular control
>> mechanism here is a good idea.
>>
>> I’d also be curious to hear which of the core plugins are having problems
>> here, in most cases, there’s a no dependency between the global
>> instantiation, and the per remap instantiation. Sudheer and LinkedIn have
>> many internal plugins that do experience this problem however, so I’m
>> guessing that maybe you have similar custom internal plugins?
>>
>> Cheers,
>>
>> — Leif
>>
>>
>> Approach (1) was my first thought, but I think the problem there is
>> whether the plugin can work as a global and a reloadable remap is a
>> property of the plugin implementation, not any particular remap rule. That
>> is, for a specific plugin, there's really no choice about whether to use
>> "@plugin" or "@global" - the configuration must get it right or the plugin
>> crashes. Every time. Every rule. It is for this reason I disagree with
>> Leif's view the user should decide. The user's opinion is irrelevant - the
>> plugin works in this mode, or it doesn't. And as our friends at LinkedIn
>> discovered, some rather basic C++ decisions (such as using static
>> variables) will prevent a plugin from working in this mode. On the other
>> hand, if the plugin uses the "User Args" feature then it can work, in which
>> case what's the point of disabling the DSO reload? Unless the plugin
>> implementor is concerned about code skew between the global and remap
>> versions, which again the user is not qualified to decide.
>>
>> My personal preference is (3), but I suspect after mysterious crashes
>> with plugins, we will have been happier with (4).
>>
>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda <
>> sudheervinukonda@yahoo.com> wrote:
>>
>>> +1 on the general idea to make the reloadability customizable per plugin.
>>>
>>> However, I think it'd be more simple, cleaner and intuitive to not tie
>>> it to whether or not a plugin is used both as a global and remap plugin.
>>>
>>> In other words, approach (1) below but, instead of calling it "@global",
>>> we could add a param which says "@reloadable=false" (the default value for
>>> "@reloadable" can be "true").
>>>
>>> The same param can then be used, when we eventually add relodability to
>>> global plugins as well.
>>>
>>> Thoughts?
>>>
>>>
>>>
>>>
>>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll <
>>> solidwallofcode@verizonmedia.com> wrote:
>>>
>>>
>>> As part of the ATS 9 upgrade, a feature was added so that remap plugins
>>> could have their DSO reloaded. This means not just the configuration, but
>>> the implementation itself. While very useful, this has some
>>> unfortunate side effects with plugins that are used in both a global and
>>> remap context. To alleviate this, a configuration variable as added to
>>> disable the feature.
>>>
>>> Although reasonable, this is a rather heavy handed way to deal with the
>>> problem. What would be better is the ability to reload the DSO or not on a
>>> per remap plugin basis. I have a few ways this could be done:
>>>
>>> 1) Add the keyword "@global" to "remap.config". This would behave
>>> exactly as "@plugin" except it would prohibit reloading of the DSO for that
>>> plugin.
>>>
>>> 2) Have the remap reload configuration check to see if the plugin is
>>> also a global plugin and disable remap DSO reload for that plugin.
>>>
>>> 3) Add a flag to the global plugin registry information which can be set
>>> during TSPluginInit which disables DSO reloading for that plugin, should it
>>> occur in "remap.config". This is similar to (2) but requires a  plugin to
>>> prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and
>>> would only be valid when called from TSPluginInit.
>>>
>>> 4) As (3), except the flag is set by default and must be cleared to
>>> enable DSO reloading in "remap.config".
>>>
>>> I'm willing to see if I can make this work, but I would like to have
>>> some feedback on the preferred approach first.
>>>
>>>
>>

Mime
View raw message