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 <bra...@puppet.com> 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 puppet-dev+unsubscr...@googlegroups.com > <mailto:puppet-dev+unsubscr...@googlegroups.com>. > 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 - eric.soren...@puppet.com - 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 puppet-dev+unsubscr...@googlegroups.com. 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.
