On Sun, 2014-02-09 at 13:35 -0800, Andy Parker wrote: > On Sat, Feb 8, 2014 at 7:01 AM, Brice Figureau > <[email protected]> wrote: > On 08/02/14 00:37, Peter Meier wrote: > > On 02/07/2014 08:26 PM, John Bollinger wrote: > > > > > >> On Friday, February 7, 2014 3:57:00 AM UTC-6, Brice > Figureau > >> wrote: > > > >> In this specific bug, I'm wondering if we really need to > have > >> those defined type as resources in the RAL catalog (I > believe it > >> might be used for relationship or event propagation). > > > > > > > >> I suspect that defined type instances could be removed from > the > >> catalog by replacing them with the collections of resources > the > >> represent (recursively). However, I would expect that to > cause an > >> explosion in the number of relationships (both signaling > and > >> non-signaling) that must appear in the catalog. > > > > > > "Flattening" the catalog is something that we have discussed > before > > [1] and which would give imho quite some improvements. > > > > However, we would likely loose important context > information, that > > should then be preserved differently. > > > > Coming back to the "Bug": I think swapping the statement > would give > > already some improvement and is a fix that could go into the > next > > (bugfix)-release. Given that there are no concerns regarding > possible > > side effects. > > > Unfortunately there's a side-effect. The problem is when you > start the > master, it creates a settings catalog with file resources in > it. Those > will be then subject to the to_hash method, which calls > parse_title > which calls in turn resource_type. With the swapped > statements, this > will call known_resource_types. Since it's the first time, it > will > initialize itself and will trigger a full parse of the > manifests. > Beforehand, it would end-up in the Puppet::Type.type which > would return > true and completely skip this initial parsing until a catalog > gets compiled. > > > > That is a similar problem that pops up all over the place. Joshua > Partlow and I struggled with it a lot while we were working on > PUP-1118. The real solution to this is to get the code to stop > assuming what the environment is and either have it passed in to it > via a parameter, or use the new current environment. Right now there > are places throughout the code that assumes all sorts of things.
Indeed. > During PUP-1118 we tried to change some of these, but in a lot of > places we were a bit more conservative. Maybe we shouldn't have been. I think this might deserve more refactoring in this respect. > When the settings catalog is being applied, there should be a specific > environment, not tied to any of the real environments, that has no > manifests to parse. In the system that we have on master, this could > be done by having all of the relevant code paths use > Puppet.lookup(:current_environment) and use > Puppet.override(:current_environment => fake_settings_environment) > when creating and applying the settings catalog. > > I have an interim patch that instead mark defined type > resources as > defined types, I'll update the bug report with my findings. > The drawback of course is that it adds a property that gets > serialized > in the catalog. If the property is not present (like an older > master to > a newer agent), the agent resorts to the old behavior of > testing first > for a builtin type then a definition. > > I'll polish the patch, and make sure there are tests for it > and will > push a PR as soon as I can for review. > > > > I've also been whittling away at some of this. I haven't tried to > tackle the agent side, but on the master side I've got a couple > patches that show some good speedups in my benchmarks. > > > https://github.com/puppetlabs/puppet/pull/2341 > > https://github.com/puppetlabs/puppet/pull/2340 I will try both when I'll have a bit more hacking time on Puppet. > 2341 also shows another way of approaching the settings catalog > problem. I added a setter for the resource_type to the > Puppet::Resource class, which lets us short circuit the type lookups. > This could be used as a hack in the settings catalog. I'm currently working on marking all resources with a kind attribute that can be either node, class, definition or builtin. This property gets sent over the catalog (which requires a schema modification, so might not be mergeable in a 3.x release). This allows the client to not try to guess a given resource type. I'm marking those resources at creation time (so the settings resource for instance get marked as builtin), and try to lookup the type of the resource accordingly (defaulting to the previous way of doing things if the resource is not marked). Unfortunately, I discovered that Puppet::Resource was so central in Puppet that everyone creates new instances of those for various completely different task, and most of the time it's not possible to know in advance the resource kind. The last one I found is when looking up a resource in a given catalog, we create a blank resource of the given title to be able to get a properly munged title back to perform the look up. I find that particularly bad performance-wise, creating so many transient objects (might be better with latest ruby GC though). I hope to have a PR ready by the end of the next week end, but at least you know what I'm trying to do :) -- Brice Figureau My Blog: http://www.masterzen.fr/ -- 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/1392135910.421.12.camel%40arsenic.daysofwonder.com. For more options, visit https://groups.google.com/groups/opt_out.
