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.
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
--
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/ldeara%24nn4%241%40ger.gmane.org.
For more options, visit https://groups.google.com/groups/opt_out.
