Do a deep merge on fpm lookup

[sigv]

Pull Request (PR) description

When allowing configuring php::fpm::pools as hashes in Hiera, the unique ("array merge") behavior is currently used. Documentation states this will return a merged array even though we are interested in a hash. Instead of doing this manual lookup, we can switch to auto lookups.

This Pull Request (PR) fixes the following issues

Fixes #536

[bastelfreak]

Hi @sigv, thanks for the fix. Can you add a test for it?

[sigv]

The real_fpm_pools variable from init class is assigned as pools variable on fpm class. That is a fallback for real_pools variable in fpm class. That is:

class php (
  Hash $fpm_pools                                 = {},
) {
  $real_fpm_pools = $fpm_pools
}

class php::fpm (
  Hash $pools                   = $php::real_fpm_pools,
) {
  $real_pools = lookup('php::fpm::pools', Hash, {'strategy' => 'deep'}, $pools)
  create_resources(::php::fpm::pool, $real_pools, $real_global_pool_settings)
}

Documentation suggests setting php::fpm::pools directly from Hiera, which then overrides the init fallback using lookup. My proposal then is to test default.yaml with a configuration based on the documentation.

[c33s]

@bastelfreak please don't merge, because we should go in the oposite direction and get rid of ALL hardcoded lookups. the hardcoded lookups break puppets ability to define the merge behavior in hiera files (Automatic class parameter lookup).

@sigv you can define the merge behavior you want in your hiera. in your case simply add this to your hiera file (you can add it to your global hiera file and you can also change the lookup behavior from your global hiera file on any other hiera level.

lookup_options:
    php::fpm::pools: { merge: {strategy: deep} }

also see my issue #434

[sigv]

@c33s I do agree that variables should be pulled in auto-magically without using the lookup function, however the current merge strategy does not work outright. Documentation explicitly states unique combines arrays and scalar values returning a merged, flattened array with duplicate values removed, meanwhile deep combines keys and values of hashes returning a merged hash. I do not think it makes sense for me to rework the two individual variables that are affected here into automatically being looked up without the use of the explicit function as the whole project utilizes that methodology anyhow and am instead proposing patching the merge behavior. This does not in of itself block an upcoming rewrite, but does fix a current bug.

[sigv]

I could switch it out to be a first lookup instead if that makes more sense as a default.

[bastelfreak]

My personal opinion is to get rid of all those lookup calls and use automatic class parameter lookup. That's also the prefered way in the puppet style guide.

[sigv]

Is the default merge behavior change a no-go for deep and no-go for first?

[sigv]

The current state of PR should have the same behavior (first) that automatic lookup has. That should mean a smooth migration down the line. I do agree that making it automatic lookup is nicer, but is that really a reason for holding a small bugfix patch back?

[Dan33l]

@sigv i don't understand why defining merging behavior in your hiera data doesn't it fit your needs?

You can try this syntax also:

lookup_options:
  php::fpm::pools:
    merge: deep

[sigv]

Of course I could add this and I get the default auto lookup behavior as well:

lookup_options:
  php::fpm::pools:
    merge: first

At the current state, if someone follows documentation on how to configure via Hiera, they add a php::fpm::pools Hash in their Hiera. That does not work on its own as of right now without forcing lookup_options to be something else. This PR fixes that problem.

If the php::fpm::pools is not expected to work on its own, documentation needs be updated to reflect the requirement on lookup_options.

[c33s]

@sigv

however the current merge strategy does not work outright. Documentation explicitly states unique combines arrays and scalar values returning a merged, flattened array with duplicate values removed, meanwhile deep combines keys and values of hashes returning a merged hash.

this is a good catch, thank you very much for that.

I do not think it makes sense for me to rework the two individual variables that are affected here into automatically being looked up without the use of the explicit function as the whole project utilizes that methodology anyhow and am instead proposing patching the merge behavior.

my opinion is different here, if we touch something, it should be better afterwards. for me the argument does not count to say "it makes no sense to clean a room because the whole flat is dirty".

as far as i can see its just:

puppet-php/manifests/fpm.pp

Line 102 in d711fc7

$real_global_pool_settings = lookup('php::fpm::global_pool_settings', Hash, {'strategy' => 'first'}, $global_pool_settings)

$real_global_pool_settings = lookup('php::fpm::global_pool_settings', Hash, {'strategy' => 'first'}, $global_pool_settings)
$real_pools = lookup('php::fpm::pools', Hash, {'strategy' => 'first'}, $pools)

->

$real_global_pool_settings = $global_pool_settings
$real_pools = $pools

this solution does not need much effort. the cleaner way would be to refactor the used variable names.

i already fixed that in my fork quite a while ago for init.pp c33s@0363ab3 and the othe fix was merged here https://github.com/voxpupuli/puppet-php/pull/435/files

@sigv & @Dan33l i am not sure (can't remember) if it simply works by adding the lookup_options does they also change the behavior for the lookup() function with hardcoded options? the reason for my fork was that with the hardcoded lookup functions it was not possible for me to change the merge behavior for these functions, it was only for the automatic parameter lookup. maybe this has changed in higher puppet versions?

for me using the lookup function inline breaks puppet. the remove of this function would be the right way for me.

[sigv]

@c33s Fair. Dropped lookup function entirely for the fpm here.

[Dan33l]

@sigv & @Dan33l i am not sure (can't remember) if it simply works by adding the lookup_options does they also change the behavior for the lookup() function with hardcoded options? the reason for my fork was that with the hardcoded lookup functions it was not possible for me to change the merge behavior for these functions, it was only for the automatic parameter lookup. maybe this has changed in higher puppet versions?

My understanding is also that lookup_options key in hiera works only with automatic parameter lookup.

I apologize, i though we were talking about automatic parameter lookup. I am not a user of this module. So, now i read the code, i am seeing that it is hard coded lookup.

I agree that a clean way is to find a way to not hard code the lookup, if possible.

[sigv]

I agree that a clean way is to find a way to not hard code the lookup, if possible.

That is indeed the current state of the PR - to switch the FPM pools variables over to automatic lookup.

[sigv]

Is this still waiting for further approval after the LGTM? I don't have push/merge rights.

[c33s]

Is this still waiting for further approval after the LGTM? I don't have push/merge rights.

we are waiting for @bastelfreak s approval. he's quite busy right now. i have the permission to merge, but it is my first merge here, so i wait for his decision. after he is ok with my reviews i will merge such requests faster in future.

[sigv]

Got it - no worries. Just wanted to hear what's going on.

[bastelfreak]

Hi people, sorry for the delay here.

[phyber]

What's the actual fix here? It's not very clear at all, and the suggestion in the documentation still results in this error.

Edit: I see a new release wasn't made yet, which is why the behaviour is still observed.

[c33s]

@phyber what exactly are you trying to do? can you show me some code snippets?

you only have to add the following code to your hiera data for example common.yaml

lookup_options:
  php::fpm::pools:
    merge: hash

my hieradata/common.yaml looks like that:

lookup_options:
  profile::nginx::servers: { merge: {strategy: deep, merge_hash_arrays: true, knockout_prefix: "--", sort_merged_arrays: true}}
  profile::nginx::upstreams: { merge: {strategy: deep, merge_hash_arrays: true, knockout_prefix: "--", sort_merged_arrays: true}}
  profile::nginx::mailhost: { merge: {strategy: deep, merge_hash_arrays: true, knockout_prefix: "--", sort_merged_arrays: true}}
  sysctl::values: { merge: first }

[phyber]

The version of Puppet here is 5.5.10 (Debian stable), the PHP module is 7.0.0.

I have php::fpm::pools defined for a single node under its own branch in hiera, nodes/%{::hostname}.yaml. I only have a php::fpm::pools defined for this single node and nowhere else in the hiera tree. The hash is pretty standard, with no lookups or anything. There is only a single Puppet environment defined (production).

php::fpm::pools:
    www:
        user: 'www-data'
        group: 'www-data'
        listen: '/run/php/php7.3-fpm.sock'
        etc: ...

The lookup options are specified in my top level common.yaml in long form.

lookup_options:
    php::fpm::pools:
        merge:
            strategy: 'hash'

We can see this working in the puppet lookup --explain:

# puppet lookup --explain php::fpm::pools
<some truncated output>
Using merge options from "lookup_options" hash
Searching for "php::fpm::pools"
  Merge strategy hash
<truncated output>
    Merged result: {
      "www" => {
        "user" => "www-data",
        "group" => "www-data",
        "listen" => "/run/php/php7.3-fpm.sock",
        "etc" => ...,
      }
    }

Running apply or noop results in the following:

# make noop
/usr/bin/puppet apply --noop /etc/puppet/code/environments/production/manifests/site.pp
Error: Evaluation Error: Error while evaluating a Function Call, Found value has wrong type, expects a Hash value, got Tuple (file: /etc/puppet/code/environments/production/vendor/modules/php/manifests/fpm.pp, line: 103, column: 17) on node <hostname>

Everything looks like it should be working.

[phyber]

After playing around with this, it feels like what's happening here is that it doesn't matter that you passed strategy: hash via lookup_options in hiera.

This appears to be because the merge strategy in the lookup function call (strategy: unique) is invalid for the type of values being passed (hashes) at the time the function call is made.
The overriding of the lookup type via hiera lookup_options probably happens within the lookup function call later on, but by then, the function has already errored out. So no matter what, this appears to be broken until the fix is released.

Fortunately, it looks like 7.0.1 is happening soon (#563), which should fix the issue thanks to this PR.