dalen / puppet-puppetdbquery

Query functions for PuppetDB
Apache License 2.0
198 stars 71 forks source link

Support multi environment setups #101

Closed coder-hugo closed 7 years ago

coder-hugo commented 7 years ago

The puppet master caches files in the lib directory of a puppet module globally not per environment. If there are multiple environments on the puppet master which are using the module in a different version it's not guaranteed that the puppet master uses the correct version of a file in the lib dir. Therefore this PR suffixes the puppetdb directory with -2.3 to get no conflicts with older versions. This suffix should be changed also in the future for breaking changes.

dalen commented 7 years ago

I'm not sure this would actually fix the issue with environment isolation as ruby would only load each file once, and they create the same constants (PuppetDB::Parser etc).

So if you would have env22 with puppetdbquery 2.2 and env23 with puppetdbquery 2.3 I think the following would happen:

  1. Client1 requests catalog for env22 -> env22/puppetdbquery/lib/puppetdb-2.2/parser is loaded and creates PuppetDB::Parser
  2. Client2 requests catalog for env23 -> env23/puppetdbquery/lib/puppetdb-2.3/parser is loaded and replaces PuppetDB::Parser with the 2.3 version
  3. Client1 requests a new catalog for env22 -> Ruby sees that it has already loaded env22/puppetdbquery/lib/puppetdb-2.2/parser, so just uses the existing PuppetDB::Parser which is from the the 2.3 version

For each file that is loaded the absolute path is added to $LOADED_FEATURES and then it will skip loading the file if the file is already there, and that already includes the environment name. So the flow above would look pretty much the same even without the version in the file paths (as the env is also in the path making them unique).

One possible solution would be to replace require with load as that will always load the file, even if it is already loaded (but it would not clear the existing constants, so the loading would be additive, not remove what's already loaded if it isn't there in the version being loaded). It would also slow things down, preferably that should only be done once per compile, not for each invocation of the function.

The proper fix would of course be in puppet itself, not sure if they are working on that though.

dalen commented 7 years ago

Another possible fix that works in the 4.x function API is to embed the parser code into the functions as a helper method instead of loading it from a separate file. According to https://tickets.puppetlabs.com/browse/PUP-731 that should work in Puppet > 4.6.2 at least.

It would be a little bit ugly to embed it, but I could separate the real code out and assemble the final function file with the parser embedded as a build step.

coder-hugo commented 7 years ago

Thanks for your reply. I can confirm that the environment isolation is working using the 4.x function API. In the end this was the reason for this PR. On our puppet master their are many environments and in some of them an older version of this module is installed that have a different method signature for the facts_hash. As the function of the module version in our environment requires a different method signature than the one that was present the master failed to compile the catalog.

So I would prefer your second solution although I haven't completely understood what you want to do. Are you already working on this solution? If not maybe you could explain it a bit more detailed so that I could implement this solution within this PR. Otherwise you can just close the PR.

dalen commented 7 years ago

Basically the generated code in parser.rb and lexer.rb would have to be embedded into the functions themselves as helper methods. The ASTNode class would also have to be switched into some sort of helper methods that operate on data structures instead of a class.

By build step I meant that we could likely keep these in separate files, but have a rake task that assembles them into the final function files for distribution.

I looked into what would be needed, but haven't taken a stab at doing it. If you want to give it a try it would be appreciated :)