The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #12567

Class parameter default values are evaluated in a seemingly random order

Added by Ben Marcotte about 2 years ago. Updated over 1 year ago.

Status:DuplicateStart date:02/10/2012
Priority:HighDue date:
Assignee:-% Done:

0%

Category:parameterized classes
Target version:-
Affected Puppet version:2.7.9 Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

Current puppet syntax allows the default value of one class parameter to be set using the value of another class parameter. However, the order in which the parameters are evaluated appears to be somewhat random. Given the following class:

class foo(
    $foo_root = '/var/foo',
    $a_path   = "${foo_root}/a",
    $b_path   = "${foo_root}/b",
    $c_path   = "${foo_root}/c",
    $d_path   = "${foo_root}/d",
    $e_path   = "${foo_root}/e",
    $f_path   = "${foo_root}/f",
    $g_path   = "${foo_root}/g",
    $h_path   = "${foo_root}/h",
    $i_path   = "${foo_root}/i",
    $j_path   = "${foo_root}/j",
    $k_path   = "${foo_root}/k",
    $l_path   = "${foo_root}/l",
    $m_path   = "${foo_root}/m",
    $n_path   = "${foo_root}/n",
    $o_path   = "${foo_root}/o",
    $p_path   = "${foo_root}/p",
    $q_path   = "${foo_root}/q",
    $r_path   = "${foo_root}/r",
    $s_path   = "${foo_root}/s",
    $t_path   = "${foo_root}/t",
    $u_path   = "${foo_root}/u",
    $v_path   = "${foo_root}/v",
    $w_path   = "${foo_root}/w",
    $x_path   = "${foo_root}/x",
    $y_path   = "${foo_root}/y",
    $z_path   = "${foo_root}/z",
) {
    notify{ "foo_root = '${foo_root}'": }
    notify{ "a_path   = '${a_path}'": }
    notify{ "b_path   = '${b_path}'": }
    notify{ "c_path   = '${c_path}'": }
    notify{ "d_path   = '${d_path}'": }
    notify{ "e_path   = '${e_path}'": }
    notify{ "f_path   = '${f_path}'": }
    notify{ "g_path   = '${g_path}'": }
    notify{ "h_path   = '${h_path}'": }
    notify{ "i_path   = '${i_path}'": }
    notify{ "j_path   = '${j_path}'": }
    notify{ "k_path   = '${k_path}'": }
    notify{ "l_path   = '${l_path}'": }
    notify{ "m_path   = '${m_path}'": }
    notify{ "n_path   = '${n_path}'": }
    notify{ "o_path   = '${o_path}'": }
    notify{ "p_path   = '${p_path}'": }
    notify{ "q_path   = '${q_path}'": }
    notify{ "r_path   = '${r_path}'": }
    notify{ "s_path   = '${s_path}'": }
    notify{ "t_path   = '${t_path}'": }
    notify{ "u_path   = '${u_path}'": }
    notify{ "v_path   = '${v_path}'": }
    notify{ "w_path   = '${w_path}'": }
    notify{ "x_path   = '${x_path}'": }
    notify{ "y_path   = '${y_path}'": }
    notify{ "z_path   = '${z_path}'": }
}

I get the following output in a test run:

info: Applying configuration version '1328902697'
notice: /Stage[main]/Foo/Notify[s_path   = '/var/foo/s']/message: current_value absent, should be s_path   = '/var/foo/s' (noop)
notice: /Stage[main]/Foo/Notify[foo_root = '/var/foo']/message: current_value absent, should be foo_root = '/var/foo' (noop)
notice: /Stage[main]/Foo/Notify[l_path   = '/l']/message: current_value absent, should be l_path   = '/l' (noop)
notice: /Stage[main]/Foo/Notify[d_path   = '/d']/message: current_value absent, should be d_path   = '/d' (noop)
notice: /Stage[main]/Foo/Notify[n_path   = '/n']/message: current_value absent, should be n_path   = '/n' (noop)
notice: /Stage[main]/Foo/Notify[q_path   = '/q']/message: current_value absent, should be q_path   = '/q' (noop)
notice: /Stage[main]/Foo/Notify[y_path   = '/y']/message: current_value absent, should be y_path   = '/y' (noop)
notice: /Stage[main]/Foo/Notify[o_path   = '/var/foo/o']/message: current_value absent, should be o_path   = '/var/foo/o' (noop)
notice: /Stage[main]/Foo/Notify[a_path   = '/a']/message: current_value absent, should be a_path   = '/a' (noop)
notice: /Stage[main]/Foo/Notify[e_path   = '/e']/message: current_value absent, should be e_path   = '/e' (noop)
notice: /Stage[main]/Foo/Notify[t_path   = '/t']/message: current_value absent, should be t_path   = '/t' (noop)
notice: /Stage[main]/Foo/Notify[f_path   = '/f']/message: current_value absent, should be f_path   = '/f' (noop)
notice: /Stage[main]/Foo/Notify[j_path   = '/j']/message: current_value absent, should be j_path   = '/j' (noop)
notice: /Stage[main]/Foo/Notify[m_path   = '/m']/message: current_value absent, should be m_path   = '/m' (noop)
notice: /Stage[main]/Foo/Notify[z_path   = '/var/foo/z']/message: current_value absent, should be z_path   = '/var/foo/z' (noop)
notice: /Stage[main]/Foo/Notify[r_path   = '/r']/message: current_value absent, should be r_path   = '/r' (noop)
notice: /Stage[main]/Foo/Notify[c_path   = '/c']/message: current_value absent, should be c_path   = '/c' (noop)
notice: /Stage[main]/Foo/Notify[w_path   = '/w']/message: current_value absent, should be w_path   = '/w' (noop)
notice: /Stage[main]/Foo/Notify[h_path   = '/var/foo/h']/message: current_value absent, should be h_path   = '/var/foo/h' (noop)
notice: /Stage[main]/Foo/Notify[u_path   = '/u']/message: current_value absent, should be u_path   = '/u' (noop)
notice: /Stage[main]/Foo/Notify[i_path   = '/i']/message: current_value absent, should be i_path   = '/i' (noop)
notice: /Stage[main]/Foo/Notify[v_path   = '/v']/message: current_value absent, should be v_path   = '/v' (noop)
notice: /Stage[main]/Foo/Notify[k_path   = '/k']/message: current_value absent, should be k_path   = '/k' (noop)
notice: /Stage[main]/Foo/Notify[g_path   = '/g']/message: current_value absent, should be g_path   = '/g' (noop)
notice: /Stage[main]/Foo/Notify[p_path   = '/p']/message: current_value absent, should be p_path   = '/p' (noop)
notice: /Stage[main]/Foo/Notify[x_path   = '/x']/message: current_value absent, should be x_path   = '/x' (noop)
notice: /Stage[main]/Foo/Notify[b_path   = '/b']/message: current_value absent, should be b_path   = '/b' (noop)
notice: Class[Foo]: Would have triggered 'refresh' from 27 events
notice: Stage[main]: Would have triggered 'refresh' from 1 events
notice: Finished catalog run in 0.16 seconds

So, even though all of the parameters that depend of $foo_root are listed after its default has been set, only 4 out of 26 of the latter variables get a value based on $foo_root’s default.

I expected that parameter values would be evaluated in the order in which they appear in the class declaration. This issue came up while trying to convert a number of modules away from using global variables to override defaults in advance of globals being deprecated in 2.8. Not being able to write parameterized classes of the style above will hinder that conversion process and our ability to be ready for that deprecation.

12567.pp (1.95 KB) Chris Price, 02/14/2012 05:08 pm


Related issues

Duplicates Puppet - Bug #9848: Variable interpolation in class parameters seems random Accepted 10/01/2011

History

#1 Updated by Chris Price about 2 years ago

  • Status changed from Unreviewed to Investigating
  • Assignee set to Chris Price

Confirmed this behavior on 2.7.x and master. Right now, we do not guarantee the order in which parameters will be assigned; however, this smells like a simple case of relying on the order of iteration over a hash. Investigating to see how complex a fix might be.

#2 Updated by Chris Price about 2 years ago

  • File 12567.pp added
  • Priority changed from Normal to High

After some investigation, it seems like there are 2 potential issues here. They are both visible in the method Puppet::Resource.set_default_parameters (lib/puppet/resource.rb:295 in master at the time of this writing).

Issue 1:

resource_type.arguments.each do |param, default|

resource_type.arguments is a hash, and we’re iterating it with “each”. The iteration order can vary based on what version of ruby you are using. In order to provide reliable ordering here (which I think is desirable, and would be required in order to make the variable interpolation correspond to the lexical ordering of the parameter definitions), I think we’d need to change this data structure to use an Array there. It seems like use an Array of pairs would be feasible, but I can’t tell yet how far-reaching of a change that might be.

Issue 2:

self[param] = default.safeevaluate(scope)

This statement evaluates a parameter using the passed scope, but then stores the result only in “self”. This does not appear to affect the “scope” object, so during the next iteration the scope doesn’t contain the previously evaluated variables. To me, this seems to imply that we will never interpolate the previously evaluated variables. And this is indeed the behavior that I’m seeing on the master branch… with ruby 1.8.7-p334, 1.8.7-p357, and 1.9.2-p290. NONE of the parameters see any substitution.

In 2.7.9, things work a little differently: with ruby 1.8.7-p334, all parameters get interpolated as the user would expect with ruby 1.9.2-p290, all parameters get interpolated as the user would expect *with ruby 1.8.7-p357, the behavior is unpredictable (this seems to match up with the original bug report).

There are known differences in hash iteration order between these ruby versions; it generally seems to be predictable in 1.8.7-p334 and 1.9.2, but unpredictable in 1.8.7-p357… so these results somewhat match my expectations.

However, it also seems that sometime in between 2.7.9 and master, we stopped supporting the ability to interpolate parameter values into other parameters altogether. Not sure if this was intentional or not.

I’ve attached the manifest that I’ve been using to reproduce this. The command I’ve been running is simply:

puppet apply ./12567.pp

#3 Updated by Chris Price about 2 years ago

  • Status changed from Investigating to Needs Decision
  • Assignee changed from Chris Price to Nigel Kersten

Hi Nigel,

Since this is a behavioral change between 2.7.x and master, Daniel suggested that we get input from you as to what the “correct” behavior should be before we decide how to proceed on this one.

#4 Updated by Nigel Kersten about 2 years ago

  • Assignee changed from Nigel Kersten to Chris Price

“However, it also seems that sometime in between 2.7.9 and master, we stopped supported the ability to interpolate parameter values into other parameters altogether. Not sure if this was intentional or not.”

I’m not aware of any intentional decision here, and it’s something that I thought we’d always supported.

“In order to provide reliable ordering here (which I think is desirable, and would be required in order to make the variable interpolation correspond to the lexical ordering of the parameter definitions), ”

We should absolutely do that, I agree.

#5 Updated by Chris Price about 2 years ago

  • Status changed from Needs Decision to Accepted

OK, so it sounds like there is a regression here. I will try to spend some time on this next week, or if it gets bumped up in priority over roadmap stuff.

#6 Updated by Chris Price about 2 years ago

  • Target version set to 3.x

#7 Updated by Ben Marcotte about 2 years ago

Out of the two issues that Chris raised in comment 2 above, which is being targeted to Telly? Although it may be appropriate to target issue 2 (the regression) to that version, I feel that issue 1 (investigate migrating from a hash to an array for evaluating the parameters) could be needed before then. If Telly becomes 2.8, and if global variables are still planned to be deprecated in that version, then it would be prudent to have a feature like this, that would help users like me migrate our existing modules that rely on globals to parameterized classes, available well in advance of 2.8 landing.

As such, perhaps Chris’s two items should be split into separate tickets with different target versions?

#8 Updated by Chris Price about 2 years ago

Your suggestion of separating it into two tickets is a valid one, depending on how this ends up getting prioritized. Let me follow up on prioritization and scheduling and I’ll split it out if necessary.

#9 Updated by Chris Price almost 2 years ago

  • Assignee deleted (Chris Price)

#10 Updated by Nick Fagerlund over 1 year ago

  • Status changed from Accepted to Duplicate

This is the same thing as #9848.

#11 Updated by Andrew Parker over 1 year ago

  • Target version deleted (3.x)

Also available in: Atom PDF