[Puppet Users] node inheritance, variable scope, and pain.

[Daniel Pittman]

G'day.

It was, until yesterday, my naive expectation that this would work:

node default { include broken }
node "krosp" inherits "default" { $value = "not " }
class broken { notice("This is ${value}broken") }

Specifically, I expected this would output 'This is not broken', but instead
we get the counter-intuitive and *very* annoying 'This is broken'.

As far as I can tell this is because puppet treats $value as out of scope to
the classes included from default: the order of processing is:

- node 'default', or any other inherited node.
- any classes included are processed.
- node $hostname is processed.

This means our practice of included universal features into the default node,
then inheriting that into other nodes, also makes it impossible for us to do
things like configure features of those universal classes.

I really do *NOT* want to go down the path of having to repeat the same set of
classes in every node; that invites disaster, by forcing me to repeat myself.

I suspect we will turn 'node default' into 'class default', then include that
rather than inheriting that, since that does fix the scope issue.


Anyway, my two questions:

Is this actually the correct behaviour as of 0.25.4?

Is this every likely to change to something that doesn't suck?

Daniel
--
✣ Daniel Pittman ✉ dan...@rimspace.net ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons

--
You received this message because you are subscribed to the Google Groups "Puppet Users" group.
To post to this group, send email to puppet...@googlegroups.com.
To unsubscribe from this group, send email to puppet-users...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-users?hl=en.

[Russ Allbery]

Daniel Pittman <dan...@rimspace.net> writes:

> It was, until yesterday, my naive expectation that this would work:

> node default { include broken }
> node "krosp" inherits "default" { $value = "not " }
> class broken { notice("This is ${value}broken") }

> Specifically, I expected this would output 'This is not broken', but instead
> we get the counter-intuitive and *very* annoying 'This is broken'.

> As far as I can tell this is because puppet treats $value as out of scope to
> the classes included from default: the order of processing is:

> - node 'default', or any other inherited node.
> - any classes included are processed.
> - node $hostname is processed.

> This means our practice of included universal features into the default
> node, then inheriting that into other nodes, also makes it impossible
> for us to do things like configure features of those universal classes.

> I really do *NOT* want to go down the path of having to repeat the same
> set of classes in every node; that invites disaster, by forcing me to
> repeat myself.

Our rule of thumb is that any time one is tempted to set variables and use
those values in included and inherited classes, that's a red flag that you
actually wanted a define instead. If you had instead written this as:

node default { broken { "message": value => "" } }
node "krosp" inherits "default" { Broken["message"] { value => "not " } }
define broken($value) { notice("This is ${value}broken") }

I'm pretty sure you'd get the behavior you expected.

--
Russ Allbery (r...@stanford.edu) <http://www.eyrie.org/~eagle/>

[Daniel Pittman]

Russ Allbery <r...@stanford.edu> writes:
> Daniel Pittman <dan...@rimspace.net> writes:
>
>> It was, until yesterday, my naive expectation that this would work:
>
>> node default { include broken }
>> node "krosp" inherits "default" { $value = "not " }
>> class broken { notice("This is ${value}broken") }
>
>> Specifically, I expected this would output 'This is not broken', but instead
>> we get the counter-intuitive and *very* annoying 'This is broken'.

[...]

> Our rule of thumb is that any time one is tempted to set variables and use
> those values in included and inherited classes, that's a red flag that you
> actually wanted a define instead. If you had instead written this as:
>
> node default { broken { "message": value => "" } }
> node "krosp" inherits "default" { Broken["message"] { value => "not " } }
> define broken($value) { notice("This is ${value}broken") }
>
> I'm pretty sure you'd get the behavior you expected.

I do, indeed. I don't much like it, as it feels the same sort of work-around
as turning "default" into a class — especially because I don't want a
define, I want a class with a tiny bit of configuration.[1]

Still, it is probably less bad than the alternatives, and I do agree that
using a define (or, next release, a parametrized class) is a better solution
than action-at-a-distance variables.

Daniel

Footnotes:
[1] In this case, one bit worth of configuration, dammit.

--
✣ Daniel Pittman ✉ dan...@rimspace.net ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons

[Peter Meier]

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

> I suspect we will turn 'node default' into 'class default', then include that
> rather than inheriting that, since that does fix the scope issue.

this is imho the way to go if you don't want to switch over to an
external node tool.

For each node I set some variables and then include exactly one class,
which might a class similar to your default or a subclass of that one.
And then we do all the overriding and setting default variables (if not
set in the node) stuff within these (sub-)classes.

This works pretty well and doesn't give you overloaded node statements.

cheers pete
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvidiYACgkQbwltcAfKi388PwCeIRoT2KWdVLhJy6gv1Psu/ZS2
q9wAnRM2A38ZsOinLLtgzJPQ4rI5Kltk
=SxhE
-----END PGP SIGNATURE-----

[David Schmitt]

On 5/6/2010 9:56 AM, Peter Meier wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>> I suspect we will turn 'node default' into 'class default', then include that
>> rather than inheriting that, since that does fix the scope issue.
>
> this is imho the way to go if you don't want to switch over to an
> external node tool.
>
> For each node I set some variables and then include exactly one class,
> which might a class similar to your default or a subclass of that one.
> And then we do all the overriding and setting default variables (if not
> set in the node) stuff within these (sub-)classes.
>
> This works pretty well and doesn't give you overloaded node statements.

Indeed, that's exactly what I would have suggested too and for the same
reasons.


Best Regards, David
--
dasz.at OG Tel: +43 (0)664 2602670 Web: http://dasz.at
Klosterneuburg UID: ATU64260999

FB-Nr.: FN 309285 g FB-Gericht: LG Korneuburg

[Robert]

On May 6, 9:56 am, Peter Meier <peter.me...@immerda.ch> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> > I suspect we will turn 'node default' into 'class default', then include that
> > rather than inheriting that, since that does fix the scope issue.
>
> this is imho the way to go if you don't want to switch over to an
> external node tool.
>
> For each node I set some variables and then include exactly one class,
> which might a class similar to your default or a subclass of that one.
> And then we do all the overriding and setting default variables (if not
> set in the node) stuff within these (sub-)classes.
>
> This works pretty well and doesn't give you overloaded node statements.

That is one possible solution to the problem Daniel describes.
I'm having similar problems finding a good (and good-looking)
solution to group nodes and use node inheritance.

Suppose there are some different clusters with identical servers,
and a bunch of other, different servers. A lot of our modules
have classes that do:

class someclass {
file { "/some/configfile":
source => [
"puppet:///module/configfile.$hostname",
"puppet:///module/configfile.$cluster",
"puppet:///module/configfile",
]
}
}

Now suppose there are the following nodes:

node default {
include someclass
include someotherclass
include anotherclass
(...)
}
node /^imap\d+/ inherits default {
$cluster = "imap"
include dovecot
}
node /^webmail\d+/ inherits default { $cluster = "webmail" }
node /^mysql\d+/ inherits default {
$cluster = "mysql"
include databases
}
node /^webdisk\d+/ inherits default {
$cluster = "webdisk"
include apache
include ldap
}

This is the way I'd like it to work, but is doesn't, because the
parent code is executed before the child code.

One solution is indeed to, as you say, put everything a single class
or
subclass, to not use inheritance. I don't like that solution very
much,
because that removes the complete configuration for any node from
sight,
while I've gone through great lengths to make the node-configuration
as
explicit as possible: node default for example, lists each class
included
class seperately. In a working environment with 20+ people managing
400+
hosts, you have to be as explicit as possible. But without repeating.

That is why I'd like to keep using node inheritance.
Is there no other way to accomplish this?

I've thought of another solution which allows for that, but it is
somewhat
of a kludge: I could create a define setlocal($cluster) and instead of
$cluster = "imap", I could say setlocal(cluster => "imap"), which
would
put this into a file on the node that is being processed. On the next
run of puppetd, a facter plugin would read that file and make $cluster
available in puppet. I haven't tried this yet, but I don't see why it
wouldn't work.

Any thoughts on this method?

[Daniel Pittman]

Robert <robert...@gmail.com> writes:
> On May 6, 9:56 am, Peter Meier <peter.me...@immerda.ch> wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>
>> > I suspect we will turn 'node default' into 'class default', then include
>> > that rather than inheriting that, since that does fix the scope issue.
>> this is imho the way to go if you don't want to switch over to an external
>> node tool.
>> For each node I set some variables and then include exactly one class, which
>> might a class similar to your default or a subclass of that one. And then
>> we do all the overriding and setting default variables (if not set in the
>> node) stuff within these (sub-)classes.
>> This works pretty well and doesn't give you overloaded node statements.
>
> That is one possible solution to the problem Daniel describes. I'm having
> similar problems finding a good (and good-looking) solution to group nodes
> and use node inheritance.

[...]

> One solution is indeed to, as you say, put everything a single class or
> subclass, to not use inheritance. I don't like that solution very much,
> because that removes the complete configuration for any node from sight, while
> I've gone through great lengths to make the node-configuration as explicit as
> possible: node default for example, lists each class included class
> seperately.

I elected to use a define, not a class, because that way I can have mandatory
configuration as well as optional configuration, for each node, FWIW.

> In a working environment with 20+ people managing 400+ hosts, you have to be
> as explicit as possible. But without repeating.

*nod* My conclusion, on thinking about the same issue, was that it really
didn't change much when I moved from 'inherits blah' to 'blah { $fqdn: ... }'
in that I was still writing that basic stuff in one place — it just had a
different syntax from inheritance.

(You could even write it exactly where 'node default' was defined if you
want. :)