trafficserver-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leif Hedstrom <zw...@apache.org>
Subject Re: Remap plugin DSO reloading - enabling and disabling
Date Fri, 08 May 2020 20:18:42 GMT


> On May 8, 2020, at 2:17 PM, Leif Hedstrom <zwoop@apache.org> wrote:
> 
> 
> 
>> On May 8, 2020, at 12:29 PM, Alan Carroll <solidwallofcode@verizonmedia.com <mailto:solidwallofcode@verizonmedia.com>>
wrote:
>> 
>> Consider a situation with option (1) with two remap rules:
>> 
>> map http://example.one <http://example.one/> http://example.one <http://example.one/>
@plugin=txn_box.so @reloadable=false blah blah blah
>> map http://example.two <http://example.two/> 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"?
> 
> 
> It should get reloaded for the second, not for the first. As far as I understand, this
is fine, the Gancho made the code such that as long as something uses some version of a plugin,
it will be kept forever.

Now, if this gets too complicated (but I don’t see why, the old remap is still available
when the new one is being created), I’d say produce an Error().

— Leif

> 
> — Leif
> 
> 
> 
>> 
>> 
>> On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <sudheervinukonda@yahoo.com <mailto: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
<mailto: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 <mailto:zwoop@apache.org>>
wrote:
>>> 
>>> 
>>>> On May 7, 2020, at 8:12 PM, Alan Carroll <solidwallofcode@verizonmedia.com
<mailto: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
<mailto: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
<mailto: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