Jira (PUP-6964) Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

[Nick Walker (JIRA)]

Nick Walker created an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Issue Type: Improvement Assignee: Unassigned Created: 2016/12/01 1:47 PM Labels: tcse Priority: Normal Reporter: Nick Walker The Problem See PUP-6952 and PUP-6953 for backgound but essentially, puppet can't find puppet 4.x functions in other modules unless they are declared as a dependency of the module trying to use the function. OR if the module just doesn't declare any dependencies. The Change The behavior of finding functions should not be dependent on the modules dependencies as defined in metadata.json for the module. We should search the entire modulepath for a function and not just in the declared dependencies. We can retain some of the optimization by searching in declared dependencies first and then if not found extend the search to all modules. This makes the optimization opt-in and not required. Add Comment

This message was sent by Atlassian JIRA (v6.4.14#64029-sha1:ae256fe)

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency The behavior is there to keep everyone honest. What is the point of having metadata if it not enforced? You can lie as much as you like without any consequences. Where should a user be looking for the functions and possibly other constructs named in the global namespace? If strict module namespacing was enforced it would never be a problem. The case I am thinking about is the user that consumes a module. That module makes use of a function foo(). Where did it come from? At least with having to declare module dependencies the list of modules to explore is greatly reduced. Also, the author of a module should be very much aware of making the module depend on another. It is a conscious design and maintenance decision. The current design of allowing no dependencies in metadata.json to mean "look everywhere" is because we interpret that as the module author saying "I did not bother helping you - go fish". I think that is bad.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

I am ambivalent though since right now it is only 4.x functions (ruby or puppet) that requires the dependencies. I also fear that the decision to allow no-dependencies to mean "all/any" has the effect that people remove rather than correct the list of dependencies. I would however like us to move in the opposite more strict direction and rather evolve the metadata.json capabilities with things like optional dependencies. But maybe the route there is to take a step back first, and increase the strictness in sync with better capabilities in metadata.json. Here I am not sure what the right balance between strictness/quality and usability is. Ping Eric Sorenson (in response to your 0.02$ on PUP-6953). Also note - as long as we allow any module to contain top scope named definitions (functions as well as other) there is always the issue of potential clashes. Strictly using metadata.json gives modules a chance to tell which thing they actually want rather than having to rely on the undefined (somewhat random) order to search modules that is obtained by stepping through modulepath. The current implementation is deterministic because it searches in the order the dependencies are listed.

Add Comment

[Maggie Dreyer (JIRA)]

Maggie Dreyer updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Maggie Dreyer Labels: tcse  triaged

Add Comment

[Moses Mendoza (JIRA)]

Moses Mendoza updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Moses Mendoza Team: Agent

Add Comment

[Nate McCurdy (JIRA)]

Nate McCurdy commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency I'd like to point out that this affects custom types from a module as well. For example, Stdlib::Absolutepath from the stdlib module. This was entirely unexpected behavior and a breaking change that affected many nodes. The fix (removing the empty array of dependencies in metadata.json) was very much not-obvious to find.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Added this to PUP-8072 epic "ScriptMerge" as we there want to break out the bolt functions from puppet to bolt and it will be unfriendly to then require that every module with plans need to make a dependency on the bolt supplied "special internal" module.

Add Comment

[Eric Sorenson (JIRA)]

Eric Sorenson commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Yes, that would be completely insane. The current behavior w/ normal 4.x functions is also insane, and if we can fix both at once so much the better.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg assigned an issue to Thomas Hallgren

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Henrik Lindberg Assignee: Thomas Hallgren Release Notes Summary: It is no longer required to have a dependency listed in a module's metadata.json on another module (b) in order to use functions or data types from module b. Sprint: Tasks Kanban Release Notes: New Feature Fix Version/s: PUP 5.4.0

Add Comment

[John Duarte (JIRA)]

John Duarte updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: John Duarte QA Risk Assessment: Needs Assessment Automate

Add Comment

[Thomas Hallgren (JIRA)]

Thomas Hallgren assigned an issue to Unassigned

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Thomas Hallgren Assignee: Thomas Hallgren

Add Comment

[Thomas Hallgren (JIRA)]

Thomas Hallgren commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Merged to master at c2d6476.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Closed PUP-6953 as a duplicate.

Add Comment

[Craig Gomes (JIRA)]

Craig Gomes updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Craig Gomes Priority: Normal Blocker

Add Comment

This message was sent by Atlassian JIRA (v7.5.1#75006-sha1:7df2574)

[Craig Gomes (JIRA)]

Craig Gomes commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency This ticket has been linked to PE-23417 - performance regression See Patrick's comments here

Add Comment

[Kenn Hussey (JIRA)]

Kenn Hussey updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Kenn Hussey Flagged: Impediment

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Josh Cooper Fix Version/s: PUP 5.4.0 Fix Version/s: PUP 5.y

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Revert merged to master in https://github.com/puppetlabs/puppet/commit/3d6023bde9a55081e57ddaec35c4db5e01ec0f87. Removing 5.4.0 fix version.

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Josh Cooper Sub-team: Language

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency I have a new PR up for this that places the modules from a module's dependencies first in the loaders to search. That means that it should in theory have the same performance as before except that it would reach "not found" in slightly longer time. It would be great to get a gatling run on that PR - possibly getting the feature in (even if it is the 11th hour to making it for 5.4.0).

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Henrik Lindberg Fix Version/s: PUP 5.y Fix Version/s: PUP 5.5.0

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Henrik Lindberg Sprint: Platform Core KANBAN, Tasks Kanban

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Josh Cooper Flagged: Impediment

Add Comment

[Christopher Wood (JIRA)]

Christopher Wood commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency I found this ticket after running face-first into this issue yesterday. A component of the behaviour in puppet 5.4.0 is that when I copied the function-using code from a component module class into a profile module class, puppet was then able to find my function. Of course the control repo lacks a metadata.json, and now that I've added my module dependency everything works, so not chasing too hard on whether function lookup works for component modules without metadata.json. (Also interested in whether the eventual error message will provide a list of places searched for functions, as that would have shed some light at my level of puppet skill.)

Add Comment

This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)

[Reid Vandewiele (JIRA)]

Reid Vandewiele commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Christopher Wood makes an excellent point about visibility. If, upon not finding a function, Puppet made how function loading works naturally discoverable, this wouldn't be nearly as painful a problem.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Henrik Lindberg Fix Version/s: PUP 5.5.0 Fix Version/s: PUP 5.y

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency I pulled this from the PUP 5.5.0 release as we are not quite there yet with the implementation as there are concerns about performance that we can only speculate about. Seems a bit risky to merge based on speculation alone. We are also still debating what the a more optimal solution would be - if the individual caches per module are positive (trading memory for speed) or if they are negative (takes both memory and unnecessary cycles). We may also have to look at 3.x function loading since the current behavior is try 4.x load first, if that fails try 3.x. That happens on every call to a 3.x function (with the 4.x lookup being optimized/cached).

Add Comment

[Eric Sorenson (JIRA)]

Eric Sorenson commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

This needs to get fixed in the 5.5.x series, or no one will be able to reasonably use 4.x functions in modules.

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

The most recent PR was rejected as it would have introduced a perf regression. I moved this back to ready for engineering to reflect reality. /cc Eric Sorenson

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper assigned an issue to Henrik Lindberg

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Josh Cooper Assignee: Henrik Lindberg

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Henrik Lindberg We will also need this functionality in order to unvendor types and providers from puppet, as previously modules didn't need to declare "module" dependencies on core T&P in puppet, or the loaders need to special case what were previously builtin types?

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Josh Cooper Types and providers are loaded using the 3.x autoloader - it does not have any restrictions on module visibility.

Add Comment

[Josh Cooper (JIRA)]

Josh Cooper commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Ah right, thanks

Add Comment

[Charlie Sharpsteen (JIRA)]

Charlie Sharpsteen updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Charlie Sharpsteen CS Priority: Needs Priority

Add Comment

[Owen Rodabaugh (JIRA)]

Owen Rodabaugh updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Owen Rodabaugh CS Priority: Needs Priority Normal CS Impact: Users writing modules can run into this and end up thrashing around trying to understand why it's not working. Anything that makes writing modules more accessible/less confusing to users is a good thing. Given that the initial attempts at this included a significant performance impact, perhaps there is a middle ground of having a flag that tells puppet whether to rely on the dependencies entry in metadata.json or instead go for "slow mode", perhaps as a new metadata.json entry, where it scans the environment for functions. CS Severity: 3 - Serious CS Business Value: 4 - $$$$$ CS Frequency: 3 - 25-50% of Customers

Add Comment

[Nick Walker (JIRA)]

Nick Walker updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Nick Walker CS Priority: Normal Needs Priority

Add Comment

[Adam Bottchen (JIRA)]

Adam Bottchen updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Adam Bottchen CS Priority: Needs Priority Major CS Severity: 3 4 - Serious Major

Add Comment

[Eric Sorenson (JIRA)]

Eric Sorenson updated an issue

Puppet / PUP-6964 Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Eric Sorenson Fix Version/s: PUP 5.y Fix Version/s: PUP 6.0.0 Fix Version/s: PUP 5.5.7

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency I continued the work on this and got some good results. I integrated 3.x function loading into the 4.x loaders such that the loaders will find and load 3x functions with lower precedence than 4.x functions as before, but now per module only - thus a module with both 3.x and 4.x can no longer have its 3x functions overridden by 4.x functions in modules coming after it on the module path. With the new approach the 3.x functions are still loaded the old way to allow code that calls scope.function_xxx to work as before. (The actual 3x loading code is unchanged to reduce risk). Instead, the 4.x loader creates a wrapper 4.x function that delegates the call using the scope.function_xxx calling convention (again to reduce risk). I also included the rewrite of returned values from 3.x functions to remove the problem of returning 4.x runtime incompatible :undef instead of nil (this is a separate ticket as well). Needed to do this in the new wrapper since the calling side does not know this needs to be done. First run of benchmarking this (10 iterations on my machine) showed promising results as the baseline was 16.05s before the change and 15.64s after (both single runs, but still indicative of a possible wash in terms of performance). There may be an opportunity to shave a little more off since a 3.x function is actually two methods; one dispatcher (the function_xxx method) that checks the argument constraints and raises errors, and the actual function (the real_function_xxx method). Since the 4x function wrapper also checks arguments it may be possible to short circuit one level of indirection per call. Impact of this is probably small in this particular benchmark. Work remains to write tests for this before putting up a PR.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg assigned an issue to Josh Cooper

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Henrik Lindberg Assignee: Henrik Lindberg Josh Cooper

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency After running the existing tests, I found that they already covered the altered behavior because the test suite found problems in the implementation (that I subsequently fixed). I think this is now good to merge. See PR with benchmark figures from running the benchmark on my machine. The new code seems to be slightly faster than the old way of loading (I measured aprox 10%).

Add Comment

[Kenn Hussey (JIRA)]

Kenn Hussey updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Kenn Hussey Flagged: Impediment

Add Comment

[Kris Bosland (JIRA)]

Kris Bosland commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency Merged to 5.5.x at e0b7dcc.

Add Comment

[Kenn Hussey (JIRA)]

Kenn Hussey updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Kenn Hussey Flagged: Impediment

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency The work in this ticket is the cause for the problem reported in PUP-9268. A 3.x (legacy API) function that has illegal method definitions in its body will fail as those methods are no longer available where they used to be.

Add Comment

[Henrik Lindberg (JIRA)]

Henrik Lindberg commented on PUP-6964

Re: Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

This was reverted for Puppet 5.5.8, but is in effect for Puppet >= 6.0.0.

Add Comment

[Austin Boyd (JIRA)]

Austin Boyd updated an issue

Puppet / PUP-6964

Puppet 4.x functions should be available to all modules not just those that declare the containing module as a dependency

Change By: Austin Boyd Zendesk Ticket IDs: 32385 Zendesk Ticket Count: 1

Add Comment

[Austin Boyd (JIRA)]