Jira (PUP-5971) Define evaluation should be evaluated depth-first, not breadth-first

0 views
Skip to first unread message

Hunter Haugen (JIRA)

unread,
Feb 24, 2016, 6:28:05 PM2/24/16
to puppe...@googlegroups.com
Hunter Haugen created an issue
 
Puppet / Bug PUP-5971
Define evaluation should be evaluated depth-first, not breadth-first
Issue Type: Bug Bug
Assignee: Unassigned
Created: 2016/02/24 3:27 PM
Priority: Normal Normal
Reporter: Hunter Haugen

For a long time, a little loop in the compiler has called evaluate_definitions after the current scope is evaluated (https://github.com/puppetlabs/puppet/blob/8a82d7d7c4d8254177e93a392607385755095cf4/lib/puppet/parser/compiler.rb#L609-L625).

This means that declaration evaluation is breadth-first instead of depth-first. However when writing manifests one often sees the pattern of if defined(Some['resource']) which depends on parse order to work. Using defined() with definitions that declare definitions such as `some::sub

{'hi': }

; if defined(Some::Sub::Define['aoeu'])` when combined with the breadth-first evaluation described violates parse order, as the defined resource will not exist until after the function call as their evaluation is delayed until the end of the current scope.

If defined resources were evaluated at parse time instead of being delayed until evaluate_definitions then parse order would be maintained.

Also, I believe that this is different than collectors as collectors affect resources across the entire catalog, whereas defined resources are only evaluated a single time and will not generate additional resources on further evaluation.

Add Comment Add Comment
 
This message was sent by Atlassian JIRA (v6.4.12#64027-sha1:e3691cc)
Atlassian logo

Henrik Lindberg (JIRA)

unread,
Feb 25, 2016, 8:46:02 AM2/25/16
to puppe...@googlegroups.com
Henrik Lindberg updated an issue
Change By: Henrik Lindberg
For a long time, a little loop in the compiler has called {{evaluate_definitions}} after the current scope is evaluated (https://github.com/puppetlabs/puppet/blob/8a82d7d7c4d8254177e93a392607385755095cf4/lib/puppet/parser/compiler.rb#L609-L625).

This means that declaration evaluation  (i.e instantiation of resources)  is breadth-first instead of depth-first. However when writing manifests one often sees the pattern of {{if defined(Some \ ['resource'])}} which depends on parse order to work. Using {{defined()}} with definitions that declare definitions such as  `
{code:puppet}
some::sub {'hi': } ; if defined(Some::Sub::Define['aoeu']) `

{code}
 when combined with the breadth-first evaluation  described  violates parse order, as the defined resource will not exist until after the function call as their evaluation is delayed until  the end of the current scope  all other unevaluated resource declarations waiting to be instantiated have been instantiated .

If
 defined  resources  declarations  were  directly instantiated when  evaluated  at parse time  instead of being delayed until {{evaluate_definitions}} then parse order would be maintained.


Also, I believe that this is different than collectors as collectors affect resources across the entire catalog, whereas defined resources are only evaluated a single time and will not generate additional resources on further evaluation.

Henrik Lindberg (JIRA)

unread,
Feb 25, 2016, 8:55:04 AM2/25/16
to puppe...@googlegroups.com
Henrik Lindberg commented on Bug PUP-5971
 
Re: Define evaluation should be evaluated depth-first, not breadth-first

Andy and I asked Luke for the design rationale for making resource instantiation lazy. There was none.

It is hard to predict how much breakage a change like this would create without first changing the implementation. There may be nasty side effects since with the current implementation an author of manifests that have a resource (A) that declares another (B) is assured that A is fully instantiated before A, such that defined(A) is true in B. After the change that would not be true since A is in the process of being defined when B is instantiated.

There are complicated issues to sort out wrt. handling of default resource expressions and their applicability. Changing the order of instantiation will also change the order in which resource defaults are evaluated (become effective).

I am +100 on making this change as it takes a lot of mystery out of puppet evaluation. It is lazy/strange for no good reason other than to "confuse a cat" [M. Python].

Josh Cooper (JIRA)

unread,
Mar 1, 2016, 1:36:03 AM3/1/16
to puppe...@googlegroups.com

Hunter Haugen (JIRA)

unread,
Mar 1, 2016, 8:12:03 PM3/1/16
to puppe...@googlegroups.com
Hunter Haugen commented on Bug PUP-5971
 
Re: Define evaluation should be evaluated depth-first, not breadth-first

Keeping lazy evalutation pointers on defined() functions such that it is neither depth nor breadth evalutaion sounds like a ball of spaghetti to me, but I'd love for you to prove me wrong.

In the mean time, I'll keep my eye open for sub defines that would assume the parent would be fully evaluated.

Henrik Lindberg (JIRA)

unread,
Mar 1, 2016, 8:16:05 PM3/1/16
to puppe...@googlegroups.com

Keeping lazy evalutation pointers on defined() functions such that it is neither depth nor breadth evaluation sounds like a ball of spaghetti to me, but I'd love for you to prove me wrong.

Indeed - but I am not suggesting that should be done. The defined function would simply look at what is (completely) defined, just like now. The point I made was just that what is already defined at any given point would change somewhat.

Maybe we should start a branch where puppet has depth-first behavior to see how much breakage there is.

Hunter Haugen (JIRA)

unread,
Mar 2, 2016, 12:55:03 PM3/2/16
to puppe...@googlegroups.com
Hunter Haugen commented on Bug PUP-5971

When changing the variable lookup scope, each variable lookup would do two lookups (the new lookup and the old lookup, except it was broken for a while) and if they were different then it raised a warning.

Two different compile paths on every run would introduce a lot of overhead, but perhaps the "puppet 4 future parser" could have depth-first and be under evaluation of users who attempt early migration, as the puppet 3 future parser did.

Most likely the issue we'd run into is that roles and profiles would declare lots of parameterized classes, and various components include those same classes farther down. Under breadth-first search the roles & profiles would be generally evaluated first and thus the parameterized classes would be fine, but depth-first risks delving into the non-parameterized declaration of classes by defined types which then conflict with the parameterized versions later on... bummer.

Henrik Lindberg (JIRA)

unread,
Mar 2, 2016, 1:44:04 PM3/2/16
to puppe...@googlegroups.com

That is not what I meant - rather a forked compiler that could be tried when using a git branch - if it is not horrible it could be introduced behind a feature switch. The catalog preview could compare two compilations regular compiler, new "depth first" compiler.

Henrik Lindberg (JIRA)

unread,
Mar 2, 2016, 1:50:03 PM3/2/16
to puppe...@googlegroups.com

Hunter Haugen yep, things like what you describe makes it difficult. What I think we need to do is to also merge the desires of different expressions. Still makes it hard as the meaning of an include (which will lookup defaults for parameters via data binding) may be different than what is stated in the manifests. We need precedence rules to determine which value wins. I think we need to consider that anyway to allow the same parameterized class to be declared more than once, to be able to have sane default attribute expressions, (and a couple of other use cases).

Hunter Haugen (JIRA)

unread,
Sep 1, 2016, 8:12:06 PM9/1/16
to puppe...@googlegroups.com
Hunter Haugen commented on Bug PUP-5971

Ran into this again with the tomcat module. Here is a simple minimal case:

define foo {
  foo::foo { $name: }
}
define foo::foo {
  ensure_resource('user', 'tomcat8', { 'ensure' => 'present' })
  file { '/test':
    owner => 'tomcat8',
  }
}
define bar {
  ensure_resource('user', 'tomcat8', { 'ensure' => 'present' })
}
foo { 'hi': } -> bar { 'hi': }

And the result

Error: Failed to apply catalog: Found 1 dependency cycle:
(File[/test] => Foo::Foo[hi] => Foo[hi] => Bar[hi] => User[tomcat8] => File[/test])

And I can't just test the bar an extra level deeper, since foo and bar may be declared at who-knows-what-depth by our users.

This message was sent by Atlassian JIRA (v6.4.14#64029-sha1:ae256fe)
Atlassian logo

Henrik Lindberg (JIRA)

unread,
Sep 7, 2016, 6:14:56 PM9/7/16
to puppe...@googlegroups.com

Henrik Lindberg (JIRA)

unread,
May 15, 2017, 2:32:03 PM5/15/17
to puppe...@googlegroups.com

Josh Cooper (Jira)

unread,
Mar 5, 2020, 1:45:03 AM3/5/20
to puppe...@googlegroups.com
This message was sent by Atlassian Jira (v8.5.2#805002-sha1:a66f935)
Atlassian logo
Reply all
Reply to author
Forward
0 new messages