Branan - I read through this and appreciate the depth of your sleuthing! Good stuff. One thing I didn't get from this write-up (and I know I sound like a broken record on this front) is an understanding of what problems the current behavior causes. Aside from the narrow issue fixed in PUP-1963, what problems do users — who I would categorize into distinct groups of "end users who experience bugs traceable to this" and "developers writing plugins that use these API calls" — suffer from today? And to what extent would the suggestions you're proposing eliminate those problems?
Cheers --eric0 On Apr 20, 2016, at 1:22 PM, Branan Riley <[email protected]> wrote: > I’ve been working on understanding and fixing client-side resource > generation. Below is a summary of what I’ve found and where I think we should > go from here. > > Keep in mind that while I’ve chatted with other PL engineers about most of > this, it’s not yet real roadmap. I’m taking it to the list to get feedback on > these changes before we move forward with actually planning this work. In > other words, “These are my views and don’t necessarily reflect the views of > my employer”. > > I’m mostly looking for feedback on > Does the “semantics and capabilities” section seem sane and correct from your > experiences, and is there anything I missed? > Do my proposed changes to resource generation make sense? > > But of course any response is welcome. > Background > In Puppet 4.2.0 and earlier (I don’t know how far back this behavior applies) > there weren’t a lot of differences between generate and eval_generate. Both > happened after the catalog had been converted into the resource graph. > Neither could participate in autorequire or dependency metaparams. By virtue > of happening later in the catalog application, eval_generate could rely on > earlier resources in the catalog to set up the system for it. The only > advantage to using generate in this setup was that it worked for depthfirst > resources. > > Starting in Puppet 4.3.0, due to PUP-1963 > <https://tickets.puppetlabs.com/browse/PUP-1963>, generate and eval_generate > are a bit more distinct than they used to be. generate has been moved to > happen earlier in the catalog application so that it can participate in > autorequires and dependency metaparam expansion. This was mostly done to > support the module team’s efforts around the native-type version of the > Concat module. > Semantics and capabilities of the resource generation functions > AFAICT, the capabilities listed here match the current code, although it’s > not the most efficient or clean part of puppet’s codebase. Any changes that I > think are necessary are listed in the following section. > > generate: Semantically, this is “I’m adding some resources to the catalog”. > Capability-wise, that means: > Generated resources are not contained by the parent > The generated resources participate in metaparam expansion and autorequires > We can place the new resources relatively “before” or “after” the parent in > the catalog (this is the depthfirst type option) > Resources are added to the catalog before anything is evaluated > > eval_generate: Semantically, this is “I require additional instances of > resources to do my work”. Capability-wise, that means: > Generated resources are contained by their parent. > The generated resources can have no additional dependency edges added > Resources can only be evaluated immediately after the parent (since they are > created as part of the parent evaluation, and are contained) > Dependency resources have already been evaluated by the time these resources > are created > Proposed Changes to Resource Generation > In Puppet 5: > Remove the depthfirst option on types. This is only used by the Tidy type > internally, and has no uses on the forge. It makes our code around resource > generation more complex, but more importantly it relies on the assumption > that the generate function should automatically create edges between the > generated resources and their parent. (see below) > > In Puppet 6+: > Remove the automatic parenting between generated and generating resources. > This automatic parenting makes sense for the eval_generate case where the > resources are necessarily closely related, but does not necessarily make > sense for resources generated before catalog application has started. > Additionally, now that relationship metaparams work for generated resources, > it’s possible for the generation method to do this directly, instead of > relying on the generation mechanisms. To quote the Zen of Python, “Explicit > is better than Implicit”. > Rename generate and eval_generate to something that better matches their > actual capabilities and use cases. > > It’s worth noting that the two items in “6+” may become requirements for a > future type/provider V2 API, since they’d be fairly large breakages to > current types. All of this is far enough out that it is essentially “wishful > thinking”. > Proposed Changes to Puppet Types > As far as I can tell, all core Puppet types should be using eval_generate. > They do not require the metaparam expansion that generate affords them, and > moving generation as late as possible protects against possible race > conditions (The most well known one is requiring a package to be installed > before you can purge a custom resource type, but my favorite is using a Mount > resource to ruin Tidy’s day). I believe this is a non-breaking change that we > could do in Puppet 4.5, as a bugfix for the above race conditions. > > The exception to that is Tidy, which uses depthfirst so it can provide some > fancy reporting on how many files have been tidied. I’m inclined to just rip > that reporting out in Puppet 5, along with the depthfirst option. > Recommendations for Custom Types > My general recommendation is “use eval_generate unless you absolutely need to > use generate”. This avoids the race conditions mentioned above, and ensures > that the work done by generated resources is “close” (in terms of graph > traversal) to the generating resource. > > It’s unfortunate that eval_generate and generate are named the way they were. > If I had my way they’d be generate and early_generate, or something along > those lines. We won’t be able to change this for a long time (probably never > in the current type/provider API), so we’re going to need to live with it and > document best practices for now. > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected] > <mailto:[email protected]>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/puppet-dev/CADWDnrmQyW80yaUcvHAqtPcX3-i_S7WuuM9tsQaRJzKC9JhDdQ%40mail.gmail.com > > <https://groups.google.com/d/msgid/puppet-dev/CADWDnrmQyW80yaUcvHAqtPcX3-i_S7WuuM9tsQaRJzKC9JhDdQ%40mail.gmail.com?utm_medium=email&utm_source=footer>. > For more options, visit https://groups.google.com/d/optout > <https://groups.google.com/d/optout>. Eric Sorenson - [email protected] - freenode #puppet: eric0 puppet platform // coffee // techno // bicycles -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/34529586-0B68-4633-B631-5C5B7902EDB9%40puppet.com. For more options, visit https://groups.google.com/d/optout.
