Need advice on PUP-1592

[Brice Figureau]

Hi,

During (the awesome) cfgmgmtcamp Puppet Contributor Summit earlier this
week, Peter Meier and I were chasing (in fact he did all the hard work)
a couple of strange bugs, and we found while stracing the agent what
might be a performance issue.

Everything is explained in detail in PUP-1592 [1], but the tl;dr version
is we're stat(2)ing a lot of inexistent files during the transaction for
every instance of a given defined types (that might well get in the
order of 100s of stat(2) per instance of defined types).

I'd like to get the input of all the devs here on the proper way of
fixing the bug before sending a PR :)

I'll also try to do some performance tests during the week-end to see
the impact (it's not yet fully known if it really matters or not
compared to the I/O load an agent already see).

Thanks!

[1]:
https://tickets.puppetlabs.com/browse/PUP-1592
--
Brice Figureau
My Blog: http://www.masterzen.fr/

[Andy Parker]

On Thu, Feb 6, 2014 at 1:50 PM, Brice Figureau <brice-...@daysofwonder.com> wrote:

Hi,

During (the awesome) cfgmgmtcamp Puppet Contributor Summit earlier this
week, Peter Meier and I were chasing (in fact he did all the hard work)
a couple of strange bugs, and we found while stracing the agent what
might be a performance issue.

That was an absolutely incredible write up of the issue that you put in the ticket! Many thanks to both of you for tracking that down. Erik Dalen had investigated something that looks like the same problem and posted the info to PUP-751.

 

Everything is explained in detail in PUP-1592 [1], but the tl;dr version
is we're stat(2)ing a lot of inexistent files during the transaction for
every instance of a given defined types (that might well get in the
order of 100s of stat(2) per instance of defined types).

I think your idea of changing the order that puppet searches makes sense. It is much more likely that the requested name is a defined type than a custom type and so looking for defined types first short circuits the whole process more quickly.

There is maybe another issue at play, though. As Erik points out it seems like puppet will look up the same things again and again. We've done a lot of hacking around to try to put in caches and all sorts of other things, but those all end up being very fragile and we regress on the fix. The problem is that known_resource_types nearly impossible for us to manage in the system in a sane matter. I think the full fix for this is going to need to address the problem of keeping track of what is loaded, what isn't, where it was loaded from, and make a much cleaner step for when the cache is invalidated.

I recently created the start of a benchmarking system in puppet (https://github.com/puppetlabs/puppet/commit/e48157e42360aaf0c67e2ef3866b091f0a113ce8) and the profile of the one scenario added already shows an absurd number of calls to determine if any files have changed. Thankfully in this case the caching of the stat call is working, but it is still spending a huge amount of time just deciding if the filetimeout has expired.

 

I'd like to get the input of all the devs here on the proper way of
fixing the bug before sending a PR :)

I think that just swapping the search order is a good fix that should be less likely for us to regress. If that shows a performance improvement, then that should definitely go in. I think the more proper fix will require a larger redesign of how puppet loads things and tracks what it has loaded.

 

I'll also try to do some performance tests during the week-end to see
the impact (it's not yet fully known if it really matters or not
compared to the I/O load an agent already see).

Thanks!

[1]:
https://tickets.puppetlabs.com/browse/PUP-1592

--
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 puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52F403BA.7090709%40daysofwonder.com.
For more options, visit https://groups.google.com/groups/opt_out.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco - http://bit.ly/pupconf14

[Brice Figureau]

Hi Andy,

On Thu, 2014-02-06 at 15:38 -0800, Andy Parker wrote:
> On Thu, Feb 6, 2014 at 1:50 PM, Brice Figureau
> <brice-...@daysofwonder.com> wrote:
> Hi,
>
> During (the awesome) cfgmgmtcamp Puppet Contributor Summit
> earlier this
> week, Peter Meier and I were chasing (in fact he did all the
> hard work)
> a couple of strange bugs, and we found while stracing the
> agent what
> might be a performance issue.
>
>
>
> That was an absolutely incredible write up of the issue that you put
> in the ticket! Many thanks to both of you for tracking that down. Erik
> Dalen had investigated something that looks like the same problem and
> posted the info to PUP-751.

Yes, this looks the same, but Erik was finding the issue on the master,
whereas Peter and I focused mostly on the agent.

> Everything is explained in detail in PUP-1592 [1], but the
> tl;dr version
> is we're stat(2)ing a lot of inexistent files during the
> transaction for
> every instance of a given defined types (that might well get
> in the
> order of 100s of stat(2) per instance of defined types).
>
>
>
> I think your idea of changing the order that puppet searches makes
> sense. It is much more likely that the requested name is a defined
> type than a custom type and so looking for defined types first short
> circuits the whole process more quickly.

That was my gut feeling too.

> There is maybe another issue at play, though. As Erik points out it
> seems like puppet will look up the same things again and again. We've
> done a lot of hacking around to try to put in caches and all sorts of
> other things, but those all end up being very fragile and we regress
> on the fix. The problem is that known_resource_types nearly impossible
> for us to manage in the system in a sane matter. I think the full fix
> for this is going to need to address the problem of keeping track of
> what is loaded, what isn't, where it was loaded from, and make a much
> cleaner step for when the cache is invalidated.

All the problem resolves to negative caching. The problem is that Puppet
tries to validate existence of given files to load them, but doesn't
keep an eye on those that don't exist. So next lookup of the same files
will also trigger a swarm of stat call, because Puppet doesn't remember
what it already didn't find.
I do agree that negative caching (and caching in general) is very
difficult. In the agent case, the transaction now happens isolated in
its own process and is very short-lived, which means we could cache in a
global singleton for the duration of the run. The problem on the master
is a bit more complex due to the global nature of the
known_resource_types (and the autoloader).

> I recently created the start of a benchmarking system in puppet
> (https://github.com/puppetlabs/puppet/commit/e48157e42360aaf0c67e2ef3866b091f0a113ce8) and the profile of the one scenario added already shows an absurd number of calls to determine if any files have changed. Thankfully in this case the caching of the stat call is working, but it is still spending a huge amount of time just deciding if the filetimeout has expired.

Yes, I've already seen this issue.

> I'd like to get the input of all the devs here on the proper
> way of
> fixing the bug before sending a PR :)
>
>
>
> I think that just swapping the search order is a good fix that should
> be less likely for us to regress. If that shows a performance
> improvement, then that should definitely go in.

This will show an improvement on the master for sure, but as I said in
the ticket that doesn't really solve the agent issue (see the second
part of the explanation). That's where I'm unsure about the proper fix.
I'd really like your (and the others) input on the problem.

> I think the more proper fix will require a larger redesign of how
> puppet loads things and tracks what it has loaded.

It's even worst than that. We need to track things that couldn't be
loaded to not try to reload them again.

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).
If we really do, we might be able to flag those as "defined type" and
then skip compound parsing title (because compound titles can only
happen on ruby/builtins types and is the root cause of trying to find
the 'defined type' type).

This would allow us to prevent calling P::R#resource_type in the agent,
at the expense of having a new flag in the resource (which also means
more information in the transferred catalog and incompatibilities and so
on).

Any more ideas on how to solve this issue?

Anyway thanks for listening :)

[John Bollinger]

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.

 

If we really do, we might be able to flag those as "defined type" and
then skip compound parsing title (because compound titles can only
happen on ruby/builtins types and is the root cause of trying to find
the 'defined type' type).

I think marking resources of defined type is a fine idea.  I particularly like that it closes a potential vulnerability wherein Puppet could be induced to load a native plugin (or anything else if the name and location were right) instead of the intended type.  I am uncertain whether this would require an incompatible change to the catalog format.  At first I thought so, but now I'm leaning the other way.
 

This would allow us to prevent calling P::R#resource_type in the agent,
at the expense of having a new flag in the resource (which also means
more information in the transferred catalog and incompatibilities and so
on).

Any more ideas on how to solve this issue?

Even without marking defined type instances in the catalog, couldn't Puppet be made to enter each un-found type into known_resource_types as a defined type?  At least when running as the agent?  Combined with reordering the tests as suggested, that could improve the situation for the agent by avoiding searching multiple times for any type, similar to how a separate negative cache could do.

John

[Peter Meier]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

Still it shows that there is room for improvement in general within
this area that should be part of a bigger review, re-architecture.
Nevertheless, I think we should also make the tiny steps that don't
hurt but give some improvement.

~pete


[1] https://groups.google.com/forum/#!topic/puppet-dev/BPvuDlb67WE



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlL1bjwACgkQbwltcAfKi3/WRgCfUi/M9D1pebXlxs/QImwkXOV3
h9MAn1TEGLKJJvdezC1ZGZZNTxHEpVgY
=IAtQ
-----END PGP SIGNATURE-----

[Brice Figureau]

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.

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.

Thanks!

[Andy Parker]

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. 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.

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

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.

 

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 puppet-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52F646AE.302%40daysofwonder.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Brice Figureau]

Hi Andy,

This isn't directly related to this thread, but the current master is
almost 1.5 times slower (compilation, parsing wasn't taken into
consideration) than 3.4.2.

For instance those 4 consecutive requests made with puppet-load:
master:
------
4 requests finished in 35.513526 s
Time (s):
min: 8.764234 s
max: 9.056899 s
average: 8.87709875 s
median: 8.843631 s

Concurrency: 1.00
Transaction Rate (tps): 0.11 t/s


v3.4.2:
------
4 requests finished in 21.600644 s
Time (s):
min: 5.290943 s
max: 5.474927 s
average: 5.3989305 s
median: 5.414926 s

I didn't bisect yet, just wanted your input if it's a known regression,
or if I should dig further.

On 09/02/14 22:35, Andy Parker wrote:
> On Sat, Feb 8, 2014 at 7:01 AM, Brice Figureau

> <brice-...@daysofwonder.com <mailto:brice-...@daysofwonder.com>>
> wrote:
>
> snipped

[Brice Figureau]

On 10/02/14 20:46, Brice Figureau wrote:
> Hi Andy,
>
> This isn't directly related to this thread, but the current master is
> almost 1.5 times slower (compilation, parsing wasn't taken into
> consideration) than 3.4.2.
>

> [snipped]

>
> I didn't bisect yet, just wanted your input if it's a known regression,
> or if I should dig further.

The culprit seems to be:
https://github.com/puppetlabs/puppet/commit/9f00459daf809cfb782808076e61099d81ea535c

I think the call to stale? might be the reason.

[Andy Parker]

Sounds likely. I tracked down bad performance to same code and submitted PR 2340 to address it. There may be side effects of my change, though. I'm a little worried that it might change how/when puppet loads manifests in a way that breaks someone's use.

Can you try with PR 2340 applied and see if it helps?

Also, I'd like to start putting together benchmarking scenarios so that we can all work from similar baselines of performance. Do you think there is some aspect of the way your puppet system works that we could make into a benchmark scenario and add to https://github.com/puppetlabs/puppet/tree/master/benchmarks ?

 

--
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 puppet-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52F93178.4020308%40daysofwonder.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Brice Figureau]

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.

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]

On Mon, 2014-02-10 at 12:24 -0800, Andy Parker wrote:
> On Mon, Feb 10, 2014 at 12:07 PM, Brice Figureau
> <brice-...@daysofwonder.com> wrote:
> On 10/02/14 20:46, Brice Figureau wrote:
> > Hi Andy,
> >
> > This isn't directly related to this thread, but the current
> master is
> > almost 1.5 times slower (compilation, parsing wasn't taken
> into
> > consideration) than 3.4.2.
> >
>
> > [snipped]
> >
> > I didn't bisect yet, just wanted your input if it's a known
> regression,
> > or if I should dig further.
>
>
> The culprit seems to be:
> https://github.com/puppetlabs/puppet/commit/9f00459daf809cfb782808076e61099d81ea535c
>
> I think the call to stale? might be the reason.
>
>
> Sounds likely. I tracked down bad performance to same code and
> submitted PR 2340 to address it. There may be side effects of my
> change, though. I'm a little worried that it might change how/when
> puppet loads manifests in a way that breaks someone's use.
>
>
> Can you try with PR 2340 applied and see if it helps?

I will make sure to test it and report success or failure.

> Also, I'd like to start putting together benchmarking scenarios so
> that we can all work from similar baselines of performance. Do you
> think there is some aspect of the way your puppet system works that we
> could make into a benchmark scenario and add to
> https://github.com/puppetlabs/puppet/tree/master/benchmarks ?

I need to study your benchmark system. I'm currently testing with my set
of production manifests, which I can't share publicly (it's about 1500
resources).
I could probably write a kind of generator that might model the
distribution of definition/resources/class/modules, but it will be kind
of hard to get the same level of resource relationship or even language
features used (or antipatterns).

Do you have a master <-> agent benchmarks machinery that I could reuse?

For the moment I was doing microbenchmarks of the agent, with a puppet
apply and debug logging. But I'd like to conduct agent tests as more
like in real life as possible...

Thanks,

[Henrik Lindberg]

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.

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.

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.

- henrik

[Brice Figureau]

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,

[Henrik Lindberg]

I think it will.

>> 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.
>

Not sure what you are saying about known_resource_type - are you saying
it is always empty? What I am proposing will result in 1 scan of all
plugin resource types - then when a lookup of a type name is performed
it first checks if this could be a plugin type (one hash lookup), if not
then it must be a user defined type. Is there something in that you
suspect will not work on the agent side?

- henrik

[Brice Figureau]

I meant that on the agent known_resource_type is always empty. That's
the root of the issue I discovered in PUP-1592: we're constantly calling
Puppet::Type.type() because we can't cache the resource_type result.
The reason is Puppet::Type.type() returns nil (and scans all the paths
each time) because this is a definition, and the last resort check
(known_resource_type.definition) also returns nil because this hash is
valid only on the master, so ultimately we return nil (which is not an
issue in the agent) which by definition renders the cache unuseful.

So yes, your idea will make sure we don't scan the disk (except once),
but we'll still not cache the resource type and call Puppet::Type.type
everytime. If your solution makes it only a negative cache lookup, I
think this is quite safe (on a performance standpoint).

--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!

[Henrik Lindberg]

I am now looking at implementing the new cache in metatype/manager since
it seems to be the manager of the autoloader that is used. Either that,
or in the autoloader itself.

That is where Puppet::Type.type(name) ends up (i.e. metatype/manager).

- henrik

[John Bollinger]

On Tuesday, February 11, 2014 5:13:59 PM UTC-6, henrik lindberg wrote:

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.

I think separating classes from other types is a fine idea, considering that implementation notwithstanding, on the DSL side classes have a few important characteristics that distinguish them from resources.

I also think that there is some value in marking defined types to distinguish them from plugins.  Performance considerations aside, I find it troubling that the agent can be induced to load random Ruby code by dropping it in a file named after a defined type in an incoming catalog.  The issue is mitigated by the fact that Puppet's lib directory is normally subject to access controls making user privilege required to execute such an attack, but I'm nevertheless inclined to favor defense in depth.


John

[Henrik Lindberg]

So, we found a simple way to improve the performance. This has now been
merged to master for release in 3.5. Basically if the name contains ':'
it cannot be a type and the lookup for type then bails out as early as
possible.

This speeds up loading of user defined type by 2x in the benchmark
defined_types.

Brice, it would be great if you could try your scenario on master and
see what it does to your use case.

- henrik