PL RFC-1: Semantics of autoloaded classes
Markus Roberts
Authors: Paul & Markus
Title: Semantics of autoloaded classes
Contents:
Abstract
Problem specification
Proposed solutions, with pros and cons
Abstract
--------
Autoloading files on the (unverified) assumption that they contain the
class (and only the class) corresponding to the file's name presents a
number of semantic issues which can result in difficult to diagnose
problems with complex manifests. The results of a compilation can
depend on such factors as:
* Which nodes have been previously compiled in this environment on this master
* Which order classes are autoloaded in
* Which classes are (or have been) compiled by other threads on this master,
* etc.
We have several ideas for resolving this problem. Since this is a
fairly major issue touching on architecture / principles we don't want
to just choose a solution without getting feedback / design discussion.
Problem Specification
---------------------
Puppet principles include:
* Consistency: software behavior shouldn't depend on factors that seem
irrelevant to an end user.
* Order independence: manifests should be declarative--their meaning
should not depend on the order in which their contents are
evaluated.
* Performance: software should execute quickly, particulary on the
master when serving a large number of nodes based on the same set of
manifest files.
* Predictability / principle of "least surprise": a user should be
able to achieve their desired effect without reading the source code
or having a deep understanding of the subtleties of Puppet's
internal design.
The current behavior of autoloading (in Statler) breaks at least three
of these principles.
It introduces potential inconsistencies, since it is possible that
while servicing node A, the puppet master will autoload some manifest
files what wouldn't have been loaded while servicing node B. If the
master later services node B, those files will remain loaded and may
affect the catalog that gets sent to node B (issue #4656).
It introduces a potential order dependency, because the order in which
manifests are evaluated determines the order in which autoloading is
attempted, and the behavior of autoloading depends on what classes
have already been loaded. For example, if a user's site.pp refers
to autoloaded classes foo::bar and foo::baz, both of which exist in
foo.pp, and foo::bar also exists in foo/bar.pp, then the behavior
depends on whether the reference to foo::bar or foo::baz is evaluated
first.
It hurts performance, because it sometimes has to access the
filesystem in order to resolve a name even if that name already
exists. For example, if a user has classes foo and bar, and tries
to refer to bar within namespace foo, Puppet first has to go to the
filesystem to ensure that foo::bar doesn't exist before it can resolve
the reference to the toplevel bar. (Note: this behavior is new to
Statler--it was introduced in commit 6b1dd81).
Of course, a user who is aware of these issues can take steps to
avoid them. For example, to avoid inconsistencies and order
dependence, a user can follow these principles
1. Ensure that each autoloaded file only contains classes,
definitions, and nodes whose names match the filename (for example,
foo/bar.pp should only contain classes such as foo::bar and
foo::bar::baz).
2. Ensure that each autoloaded class is defined in only one file.
3. Ensure that autoloaded files do not contain toplevel resources
(resources declared outside of any class).
However, in accordance with the principle of least surprise, we would
rather not to force users to be aware of these principles in order
to write good manifests.
Note that we're initially focussing on the autoloading of .pp files;
some of the proposed solutions may need extention / modification to
account for internal DSL (.rb) files.
Proposed Solution I: rebuild known_resource_types on each compile
-----------------------------------------------------------------
Each time an agent contacts the master, rebuild known_resource_types
from scratch (using cached copies of the ASTs for the relevant files).
Pros:
- This eliminates inconsistencies caused by servicing previous nodes.
- Minimal user impact. Manifests that worked consistantly in 0.25 and
2.6 will continue to work without modification.
- It will cause manifests which depended on the node-order issues (and
thus failed occasionaly) to fail consistently, making them much easier
to diagnose.
Cons:
- This does nothing to address order dependencies or the existing
performance issues with autoloading.
- It introduces a slight additional performance hit, since
known_resource_types must be rebuilt from scratch with each compile.
However, we estimate this performance hit to be very small in
practice (<1% of total compilation time spent on the master).
Proposed Solution II: restrict the contents of autoloaded files
---------------------------------------------------------------
When autoloading a file, check that it only declares things
within a namespace corresponding to its filename. For example,
the file $module_path/foo/manifests/init.pp may declare a
class (or definition or node) foo, any class within the namespace
foo (e.g. foo::bar), and any resources within those classes. It
may not declare any toplevel resources or any other toplevel
classes. These same restrictions apply to a file ./foo.pp
autoloaded from the current working directory.
In a similar vein, a file $module_path/foo/manifests/bar.pp (or
./foo/bar.pp) may only declare things within the namespace
foo::bar. Exception: it may also declare the class foo, but it
may not declare anything inside it other than bar. These rules
are extended in the obvious way for more deeply nested
files (e.g. ./foo/bar/baz.pp).
Also, modify the search order of autoloading so that it looks in
the order of general to specific. For example, when trying to
autoload foo::bar::baz, if foo is a module, it looks in the order
$module_path/foo/manifests/init.pp, then
$module_path/foo/manifests/bar.pp, then
$module_path/foo/manifests/bar/baz.pp.
If foo is not a module, then it looks in the order
./foo.pp, then
./foo/bar.pp, then
./foo/bar/baz.pp.
In either case, it stops as soon as it finds
the thing being autoloaded.
Pros:
- This eliminates most sources of inconsistencies and order
dependencies by formalizing a relationship between files and
classes that most users are probably following anyway.
- It eliminates the remaining sources of inconsistencies and
order dependencies by making a small change to search order
that is unlikely to affect most users.
- It forces users to follow a naming convention that will help
them to organize their manifests well.
Cons:
- This does not address any performance issues with autoloading.
- Potentially large user impact. Unconventionally structured
manifests that worked in 0.25 and 2.6 may require substantial
renaming / relocation of classes in order to meet the new file
organization requirements. (However, users can work around
this using explicit imports.)
Proposed Solution III: eliminate the autoloading feature
--------------------------------------------------------
Do not allow autoloading. Require the user to explicitly import all
necessary manifest files either directly or indirectly from site.pp.
Pros:
- This eliminates inconsistencies and order dependencies because all
files are imported before evaluation of any nodes begins.
- It improves performance, because it eliminates the need to go to the
filesystem when resolving names.
- It improves predictability, because it always makes it explicitly
clear which manifest files can contribute to a catalog.
- Minimal user impact. Manifests that worked in 0.25 and 2.6 may
require explicit imports in order to work properly without autoloading,
however it will be easy to tell from the error messages what imports
to add.
Cons:
- It eliminates a feature which users may perceive to be of high value.
R.I.Pienaar
Thanks for sending these proposals to the list, already finding it useful.
----- "Markus Roberts" <mar...@puppetlabs.com> wrote:
> Proposed Solution I: rebuild known_resource_types on each compile
> -----------------------------------------------------------------
>
> Each time an agent contacts the master, rebuild known_resource_types
> from scratch (using cached copies of the ASTs for the relevant files).
>
> Pros:
>
> - This eliminates inconsistencies caused by servicing previous nodes.
>
> - Minimal user impact. Manifests that worked consistantly in 0.25 and
> 2.6 will continue to work without modification.
>
> - It will cause manifests which depended on the node-order issues (and
> thus failed occasionaly) to fail consistently, making them much easier
> to diagnose.
>
> Cons:
>
> - This does nothing to address order dependencies or the existing
> performance issues with autoloading.
>
> - It introduces a slight additional performance hit, since
> known_resource_types must be rebuilt from scratch with each compile.
> However, we estimate this performance hit to be very small in
> practice (<1% of total compilation time spent on the master).
I don't know the internals in this area but it sounds clean.
> Proposed Solution II: restrict the contents of autoloaded files
> ---------------------------------------------------------------
>
> When autoloading a file, check that it only declares things
> within a namespace corresponding to its filename. For example,
> the file $module_path/foo/manifests/init.pp may declare a
> class (or definition or node) foo, any class within the namespace
> foo (e.g. foo::bar), and any resources within those classes. It
> may not declare any toplevel resources or any other toplevel
> classes. These same restrictions apply to a file ./foo.pp
> autoloaded from the current working directory.
>
> In a similar vein, a file $module_path/foo/manifests/bar.pp (or
> ./foo/bar.pp) may only declare things within the namespace
> foo::bar. Exception: it may also declare the class foo, but it
> may not declare anything inside it other than bar. These rules
> are extended in the obvious way for more deeply nested
> files (e.g. ./foo/bar/baz.pp).
>
> Also, modify the search order of autoloading so that it looks in
> the order of general to specific. For example, when trying to
> autoload foo::bar::baz, if foo is a module, it looks in the order
>
> $module_path/foo/manifests/init.pp, then
> $module_path/foo/manifests/bar.pp, then
> $module_path/foo/manifests/bar/baz.pp.
I'd like to add to this foo/manifests/bar/baz/init.pp see #2636 for my motivation
>
> If foo is not a module, then it looks in the order
>
> ./foo.pp, then
> ./foo/bar.pp, then
> ./foo/bar/baz.pp.
>
> In either case, it stops as soon as it finds
> the thing being autoloaded.
>
> Pros:
>
> - This eliminates most sources of inconsistencies and order
> dependencies by formalizing a relationship between files and
> classes that most users are probably following anyway.
>
> - It eliminates the remaining sources of inconsistencies and
> order dependencies by making a small change to search order
> that is unlikely to affect most users.
>
> - It forces users to follow a naming convention that will help
> them to organize their manifests well.
>
> Cons:
>
> - This does not address any performance issues with autoloading.
>
> - Potentially large user impact. Unconventionally structured
> manifests that worked in 0.25 and 2.6 may require substantial
> renaming / relocation of classes in order to meet the new file
> organization requirements. (However, users can work around
> this using explicit imports.)
I like this too, I think there was a bug earlier on in the 2.6
series that actually did this. People hated it but I think in
the end it will be better. It might take us a long time to get
there as we might need to start logging deprecation notices but
I think this should be the eventual goal.
We should do better with publishing a set of best practices and
I think they should heavily rely on autoloading semantics so I
think this RFC is extremely important.
If we go half way and say we can have multiple classes in a file
we should still ensure people dont put any code outside of class
boundaries in these files, any code they want there should be in
site.pp or somewhere specifically obvious as 'if you put it here
it can/will apply to all hosts' it's a common rookie mistake we
often see on the IRC. I remember lots of broken Solaris machines
on my first week with puppet due to this as well.
You think foo.pp is class foo surely, anything I put in it belongs
to that class. We should help remove that confusion.
Further source of confusion is the import/include thing, people
just don't get the difference. I think a target to have might be
to totally illuminate the import function from the users view in
the long run.
We might consider adding a special place to where node blocks will
be auto imported from as well, /etc/puppet/nodes or whatever, so
that people don't need to mess around with import at all should they
follow convention. I know you guys think node blocks should best
die and be replaced with classifiers, I find them useful and they
are good for starting out, so having a known place to just put .pp
files for them would be nice and smooth the learning curve
>
>
> Proposed Solution III: eliminate the autoloading feature
> --------------------------------------------------------
strongly against this one :)
Peter Meier
Hash: SHA1
> Proposed Solution II: restrict the contents of autoloaded files
> ---------------------------------------------------------------
>
> [...]
>
> Pros:
>
> - This eliminates most sources of inconsistencies and order
> dependencies by formalizing a relationship between files and
> classes that most users are probably following anyway.
>
> - It eliminates the remaining sources of inconsistencies and
> order dependencies by making a small change to search order
> that is unlikely to affect most users.
>
> - It forces users to follow a naming convention that will help
> them to organize their manifests well.
This is something that a few people would see as a con, but I would say
it's a big plus.
> Cons:
>
> - This does not address any performance issues with autoloading.
>
> - Potentially large user impact. Unconventionally structured
> manifests that worked in 0.25 and 2.6 may require substantial
> renaming / relocation of classes in order to meet the new file
> organization requirements. (However, users can work around
> this using explicit imports.)
Given that this is already now an unpredictable behavior of puppet and
as I think it is generally already now a good idea to get rid of global
resources in files that are autoloaded I would say that we could live
with that cons. There is no global resource definition that couldn't be
refactored in a class. Furthermore, if there is a big warning that this
won't anymore work on the next version I think we are also fine to do that.
> Proposed Solution III: eliminate the autoloading feature
> --------------------------------------------------------
no way.
I didn't say anything to solution I as I think II is my preferred
solution hence I would prefer choosing II over I.
thanks for the proposal.
pete
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkyR344ACgkQbwltcAfKi38FWwCeI2Uq5eMGEub7xZIffg1upH6P
DiAAn0MavpJzvXAzo5268wUv04Vsj3Ps
=q4zU
-----END PGP SIGNATURE-----
Daniel Pittman
[...]
> 1. Ensure that each autoloaded file only contains classes,
> definitions, and nodes whose names match the filename (for example,
> foo/bar.pp should only contain classes such as foo::bar and
> foo::bar::baz).
I would support this, and it keeps the legitimate uses of these non-matching
manifest names working for me. (By which I mean hacks like the following.)
class foo {
# The array is usually split($random_fact, /:/) or something.
foo::helper { ["foo", "bar", "baz"]: }
}
define foo::helper () {
# Do something for each item in the array that isn't easy to express in
# the common bit above.
}
> Proposed Solution III: eliminate the autoloading feature
> --------------------------------------------------------
>
> Do not allow autoloading. Require the user to explicitly import all
> necessary manifest files either directly or indirectly from site.pp.
I would not be happy with this. It would make it crazy-hard to manage a large
scale puppet system.
What looks sensible to me would be to implement option one as a "one major
release" feature, during which time an implementation of option two as a
warning rather than an error.
Then on the next major release crank over to an error, ditch the work-around,
and everyone wins: you can reject the problems (if I understand option 2
right) without having to pay the extra cost to check them every time.
Daniel
--
✣ Daniel Pittman ✉ dan...@rimspace.net ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons
Markus Roberts
What looks sensible to me would be to implement option one as a "one major
release" feature, during which time an implementation of option two as a
warning rather than an error.
Then on the next major release crank over to an error, ditch the work-around,
and everyone wins: you can reject the problems (if I understand option 2
right) without having to pay the extra cost to check them every time.
+1
That should not, from what I can see, be significantly harder than option II (which seems to be the current favorite) and greatly reduces its sting. I like.
-----------------------------------------------------------
The power of accurate observation is
commonly called cynicism by those
who have not got it. ~George Bernard Shaw
------------------------------------------------------------
Brice Figureau
> PuppetLabs Request For Comments: 1
> Authors: Paul & Markus
> Title: Semantics of autoloaded classes
> Proposed Solution I: rebuild known_resource_types on each compile
> -----------------------------------------------------------------
>
> Each time an agent contacts the master, rebuild known_resource_types
> from scratch (using cached copies of the ASTs for the relevant files).
>
> Pros:
>
> - This eliminates inconsistencies caused by servicing previous nodes.
>
> - Minimal user impact. Manifests that worked consistantly in 0.25
> and
> 2.6 will continue to work without modification.
>
> - It will cause manifests which depended on the node-order issues (and
> thus failed occasionaly) to fail consistently, making them much
> easier
> to diagnose.
This is good, failing early is better.
> Cons:
>
> - This does nothing to address order dependencies or the existing
> performance issues with autoloading.
>
> - It introduces a slight additional performance hit, since
> known_resource_types must be rebuilt from scratch with each compile.
> However, we estimate this performance hit to be very small in
> practice (<1% of total compilation time spent on the master).
There's another con, you'll keep in memory n copies of the AST where n
is your max concurrency (thanks to the way the GC works).
I don't really know how much memory we're talking about, maybe it's
something we should not care about.
I like this. Enforcing this can be a pain, but in the end the manifest
architecture would be way better (ie cleaner code).
I think lot of people will not like this, though :(
> Cons:
>
> - This does not address any performance issues with autoloading.
Which is not good. I do agree that we should aim for the correct result
before talking about performance, but if in Statler we lose 50%
performance in compilation a lot of people will get angry :)
> - Potentially large user impact. Unconventionally structured
> manifests that worked in 0.25 and 2.6 may require substantial
> renaming / relocation of classes in order to meet the new file
> organization requirements. (However, users can work around
> this using explicit imports.)
If there is a (well) documented work-around, it should be better.
> Proposed Solution III: eliminate the autoloading feature
> --------------------------------------------------------
>
> Do not allow autoloading. Require the user to explicitly import all
> necessary manifest files either directly or indirectly from site.pp.
>
> Pros:
>
> - This eliminates inconsistencies and order dependencies because all
> files are imported before evaluation of any nodes begins.
>
> - It improves performance, because it eliminates the need to go to the
> filesystem when resolving names.
>
> - It improves predictability, because it always makes it explicitly
> clear which manifest files can contribute to a catalog.
>
> - Minimal user impact. Manifests that worked in 0.25 and 2.6 may
> require explicit imports in order to work properly without
> autoloading,
> however it will be easy to tell from the error messages what
> imports
> to add.
>
> Cons:
>
> - It eliminates a feature which users may perceive to be of high
> value.
Even though I use a mix of manual import and auto-loading, I still think
auto-loading is really nice.
In a few words, I'd really like to have both the performance and the
nice features. For this reason, I'd vote if feasible, for #1 with the
strict checking of #2 if possible :)
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!
Paul Berry
--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppe...@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
Markus Roberts
>>
> Each time an agent contacts the master, rebuild known_resource_types
> from scratch (using cached copies of the ASTs for the relevant files).
>
There's another con, you'll keep in memory n copies of the AST where n
is your max concurrency (thanks to the way the GC works).
I don't really know how much memory we're talking about, maybe it's
something we should not care about.
Is this necessarily the case? If the ASTs are read-only (as they should be, though possibly aren't) they could be shared, no?
Paul wrote:
> It sounds to me like we have a decent consensus for Proposed Solution II
> (restrict the contents of autoloaded files) with the caveat that if we're going
> to make the changes in version n, it would be nice to issue deprecation
> warnings in version n-1 for instances where the classes inside a file don't
> fit the naming requirements.
> Would anyone object if we slid the deprecation warnings into the next 2.6
> point release (probably 2.6.3 I'm guessing), so that we can do the full
> solution in 2.7?
-- M
Paul Berry
Paul wrote:
> It sounds to me like we have a decent consensus for Proposed Solution II> warnings in version n-1 for instances where the classes inside a file don't
> (restrict the contents of autoloaded files) with the caveat that if we're going
> to make the changes in version n, it would be nice to issue deprecation
> fit the naming requirements.
The consensus position also had option I (rebuild the TypeCollection for each compile) as an adjunct.
Luke Kanies
Nick Lewis
Not that I know of, but those performance differences are likely gigantic - at least doubling compile times in some cases.
Luke Kanies
>
> Not that I know of, but those performance differences are likely gigantic - at least doubling compile times in some cases.
>
>
> The only extra work with option I is rebuilding known_resource_types, which simply requires walking the ASTs (it doesn't involve re-parsing any files). That doesn't seem like it would be particularly slow, and doesn't seem to be based on our admittedly-contrived performance testing. Is there something I'm misunderstanding with the compilation process?
Ah, this must be something that Paul has changed recently. What holds
a reference to the AST now, since it's not the known resource types?
Nick Lewis
Ah, this must be something that Paul has changed recently. What holds
a reference to the AST now, since it's not the known resource types?
Peter Meier
Hash: SHA1
> Would anyone object if we slid the deprecation warnings into the next 2.6
> point release (probably 2.6.3 I'm guessing), so that we can do the full
> solution in 2.7? That would be my preference, since the autoloading issues
> are frustrating my efforts to work on other 2.7 features, and it would be
> nice not to have to wait until 2.8 on all of this stuff.
I'm fine with that and I think it's a good idea to introduce that
towards 2.7.
One note: Don't flood the logs with deprecation warnings as it have been
done in the past. Once one note at the first load should be sufficient.
cheers pete
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkycb5IACgkQbwltcAfKi3/BKgCgtXxvYR94wQwszGF/ciyrA/vX
bXYAn3/a5bn74Vr5zK2ddLpVv0tRqorg
=ifoU
-----END PGP SIGNATURE-----