Dynamic loading breaks behavior for a plugin who is both remap and global.

[zizhong]

While migrating to ATS 9, we found an issue with Plugin Factory.
We have a bunch of plugins, where TSPluginInit() and TSRemapNewInstance(). In TSPluginInit(), we initialize global configs.
In ATS 9, for every remap (re)load, it makes a copy of the shared objects and dlopen it one more time. So, in memory, there will be multiple shared object context. Since the new context doesn’t have the config initialization, and other setups in TSPluginInit(), the code is broken.
I know the loading plugins multiple times is needed for dynamically loading the remap plugins. It breaks the behavior for any plugin both a global plugin and a remap plugin.
Could we add an option to let the user disable the dynamic loading of the plugin?
@gtenev @bryancall

[sudheerv]

@gtenev I don’t believe this is the intended behavior? We shouldn’t allow two active copies of a given plugin be active (even if they are loaded in a mixed mode), no?

Also for the record, @15ljindal found this problem while migrating to ATS9.

[shinrich]

What is the exhibited behavior in this case? Crash? Actually I think we are running with one of the dynamic reload commits removed. Let me look up the issue we filed for that.

[shinrich]

We reverted a remap commit to avoid crashes on start up due to misconfigured remap.config files as described in issue #6301. Probably unrelated except for both touching the new dynamic remap load logic.

[sudheerv]

@shinrich We don't have ats9 in prod yet, but, Lokesh caught this issue with our functional tests for our plugins. The plugin initializes global config variables via TSPluginInit(), but that state was lost during TSRemapDoRemap() since TSRemapNewInstance() loads a duplicate copy of the .so for the same plugin. I suspect this will likely also crash if we put it in prod.

[bryancall]

@zizhong I am having problem with getting a plugin to reload in a test case using this plugin for some reason. I am touching the .so and even rebuilding it and it won't reload.

Example plugin:

#include <ts/ts.h>
#include <ts/remap.h>
#include <stdio.h>

#define PLUGIN_NAME "hello_world"

void *some_global_var = nullptr;

void
TSPluginInit (int argc, const char * argv[])
{
  TSPluginRegistrationInfo info;
  info.plugin_name = PLUGIN_NAME;
  info.vendor_name = "MyCompany";
  info.support_email = "ts-api-support@MyCompany.com";

  if (TSPluginRegister(&info) != TS_SUCCESS) {
    TSError("[%s] Plugin registration failed", PLUGIN_NAME);
  }
  TSDebug("bcall", "TSPluginInit: %p", &some_global_var);
}

TSReturnCode TSRemapNewInstance(int argc, char * argv[], void ** ih, char * errbuf, int errbuf_size) {
  TSDebug("bcall", "TSRemapNewInstance: %p", &some_global_var);
  return TS_SUCCESS;
}

void TSRemapDeleteInstance(void *foo) {
}

TSReturnCode TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size) {
  TSDebug("bcall", "TSRemapInit: %p", &some_global_var);

  return TS_SUCCESS;
}

TSRemapStatus TSRemapDoRemap(void * ih, TSHttpTxn rh, TSRemapRequestInfo * rri) {
  return TSREMAP_DID_REMAP;
}

This is the output I get when running with the debug tag "bcall":

traffic.out:[Feb  6 09:55:03.374] traffic_server DIAG: (bcall) TSPluginInit: 0x7fef38023040
traffic.out:[Feb  6 09:55:03.384] traffic_server DIAG: (bcall) TSRemapInit: 0x7fef3801e040
traffic.out:[Feb  6 09:55:03.384] traffic_server DIAG: (bcall) TSRemapNewInstance: 0x7fef3801e040
traffic.out:[Feb  6 09:55:03.384] traffic_server DIAG: (bcall) TSRemapNewInstance: 0x7fef3801e040

[zizhong]

@bryancall In @15ljindal's test, he didn't reload the remap. Merely loading it could dlopen twice for the plugin and create two copies of shared objects in the memory, which makes the states created in TSPluginInit leaked.

[sudheerv]

FYI -
authproxy seems to have this issue as well. I haven't tested it but just looking at the code, it has both TSPluginInit and TSRemapNewInstance and it seems to have shared state initialized in TSPluginInit and accessed in later hooks.

And authproxy is a stable plugin in the repo.

[zwoop]

First off, to make sure we don't muddle the waters, I'm 98% certain that authproxy does not fall into this category. You would simply not use authproxy as both a global and a remap plugin at the same time. They would conflict (as a global, it always applies, so moot point in trying to use it as a remap).

Second, I'd really like to understand what the actual reasoning here is for sharing data between the global portion of your plugins, and remap plugins In all cases I can think of, where we have plugins being both global and remap plugins, they don't share any state or data between them. E.g. header_rewrite is commonly used with one global configuration ("apply this rule to all requests") and then a bunch of per-remap instances ("only apply these rules to requests matching this remap rule"). Yes, the global version will not be reloaded, but this is expected, for now, but it will continue to run just fine with the old .so code even during remap plugin reload.

[gtenev]

What stopped working

Let’s say that a plugin supports running as a global plugin calling the global plugin API (TSPluginInit) and as a remap plugin calling remap plugin API (TSRemapInit and TSRemapNewInstance).

In ATSv8 the following use cases are supported:

• running the plugin as remap plugin
• running the plugin as global plugin
• running the plugin as both global plugin and remap plugin.

In ATSv9 all of the above works but sharing context (global objects) between global plugins and remap plugins is no longer supported (global objects end up on different addresses).

Sharing context between remap plugins across config reloads still works but only if plugins are of the same version (having same DSO file path and timestamp).

Why did it stop working?

In ATSv9 having multiple copies of the DSO in memory is a requirement and not a bug. This is necessary in order to keep the code and access to data structures consistent across multiple configuration / plugin reloads. New HTTP transactions would pickup the new version of the plugin code but old transactions and long lasting continuations would use the old one in order to finish their work safely. Sharing global objects made sense only between DSO memory copies of the same plugin version.

Since global plugins are not reloadable yet (also would not fit in 9.x release schedule) we decided that most of the cases would be OK with having only remap plugins reloadable for now. But then not having global plugin reloadable would leave us with not being able to share global objects between global and remap plugins.

What was discussed and what’s next?

Great care and lots of design and implementation discussions took place before implementing the new plugin reload feature (discussed at 2 ATS summits, online and offline for several moths). It is also worth mentioning that to be able to load multiple DSOs at different addresses on all supported OSs we had to design/implement new infrastructure on top of dlopen because features like dlmopen were only supported on Linux.

Having the option not to support (or disable) plugin reload was never discussed. Quite the opposite - long term we were planning to make global plugins reloadable too. Making global plugins reloable could require more design discussions (the current issue is a good example, probably there will be more).

In general using the same “reload” infrastructure for global and remap plugins seems the best way to go long term, which would mean that we would share global objects between global and remap plugins in the future the same way we currently do between remap plugins in ATSv9.

Plugin reload could expose old problems

The impact of having problems with the feature can be huge. I can imagine the pain. I have been addressing diligently all problems and concerns so far.

But at the same time this new feature could expose old design and implementation problems (even unrelated to the plugin reloading mechanism itself) and we will have to deal with that.

Consider changing the plugin first

In ATSv8 and below I could not see strong signs that the global and the remap plugin mechanisms were meant by design to work together or to complement each other and speculated that it just happened to work that way and we went with it (remap plugins were added much later then global plugins). Whether we all agree with this speculation is not important. I was using this point to (hopefully) get people unstuck from their old expectations.

Regardless, I would like to ask that we consider changing the plugin design or implementation first, hoping to avoid making core design changes in ATSv9.

Based on our discussion on slack so far I have not heard a strong enough argument (other than avoiding plugin changes) to support changing the core design. One-time initialization and avoiding unnecessary plugin reloads still seem possible with ATSv9 core behavior.

I think it is ultimately up to the LinkedIn team to judge the feasibility of changing the plugin but my opinion is that avoiding plugin changes should not be the only reason.

[sudheerv]

OK, it turns out this was an agreed upon design change in ATS9 which I didn't have context on. Apologies for the rather long debate without that context.

We will work on changing our plugins, but that’ll take quite a bit of time as our plugins are owned by multiple teams across the org and changing some aspects of it isn’t as straightforward as making the config init one time. My vote would be to make this configurable similar to how we’ve always rolled out major new features. I don’t really see why this feature alone needs to be special in terms of not making it configurable. Especially, when it is incremental in nature and breaks preexisting things.

If it’s not very easy to make it configurable per plugin, I don’t mind if we make it compile time configurable like @randall suggested. That’ll still give us time to turn this off while we get our plugins updated and eventually enable this. I’d really hate for us to write a local patch just to turn this off and would like us to keep our code with as little custom patches as we can.