On Wed, 2014-02-12 at 00:13 +0100, Henrik Lindberg wrote: > On 2014-11-02 17:25, Brice Figureau wrote: > > 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 :) > > > > Discussed this with zaphond42 today and I want to try an approach that > is different - since we will be visiting all directories at least once > when looking for resource type implementations in ruby, we could just as > well just cache their names up front. That way, subsequent searches are > simply a hash lookup, and we can continue to lookup ruby plugin resource > types before user defined. > > This works when we change the semantics to perform a stale check only > before each request is processed. I am counting on that there are far > fewer plugin types than there are definitions. > Since the plan is to not load all of them upfront, but only record their > existence the entire operation should be fast enough even if it will map > unused types.
That sounds great, if it solves the agent case too. IMHO we are focusing a bit too much on the master performance :) > Lookup of user defined definitions could also benefit from similar > caching of directory entries - with the goal of only visiting each > directory once, but it is a bit more complicated to implement. The current way of loading definition works fine, because it is done during parsing, so the resource lookup is just a hash lookup, it doesn't involve reparsing everything. In the agent, the known_resource_types is empty and that means we constantly hit the type loader. If the type loading scheme you propose also works in the agent, then we'll be mostly fine except we won't be able to cache the definitions resources types (and thus will hit the type loader every time). That was the root of my ideas of marking resources, because once we know what kind a resource is, everything becomes simpler, as you don't have to guess what kind of type your resource is, and it can be looked up more easily. > I am not so hot on marking if a resource reference is for a plugin type > or a user defined type - but I am for separating Class from the rest. > > The real fix is to make the implementation stop constantly looking up > the type. Hope we are able to tackle that part in time for Puppet 4. > Meanwhile, caching the plugin resource types can be made upfront in a > clean way, bound to the context at the point where it is done, and the > cache can then be used. This is something we should be able to get into > place quickly. OK, I'll stop my efforts then (my spare time is quite scarce, I don't want to spend it on things that are proved to be dead-ends :). I'll try to review and performance test whatever you can come with. Thanks, -- 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/1392194598.29754.11.camel%40arsenic.daysofwonder.com. For more options, visit https://groups.google.com/groups/opt_out.
