Slashbunny / puppet-phpfpm

Manages php-fpm daemon and pool configuration
GNU General Public License v3.0
10 stars 29 forks source link

parameter errors in pool.pp with the future parser #13

Closed timmooney closed 8 years ago

timmooney commented 8 years ago

We've been using your phpfpm module for more than a year on RHEL 6 and RHEL 7, with good success. We're now on puppet 3.8.7 (both client and server) using the traditional, open source puppet stack. We're in the process of testing the modules we use to see if they'll work with puppet 4.x.

Two of the steps in that testing process are to set stringify_facts = false and to set parser = future, as described here:

https://docs.puppet.com/puppet/latest/reference/upgrade_major_pre.html

The future parser seems to object to the sanity checking for the pool min/max/spare settings:

Error: Evaluation Error: Error while evaluating a Function Call, pm_start_servers(5) must not be less than pm_min_spare_servers(5) and not greater than pm_max_spare_servers(35) at /etc/puppet/environments/production/forge-modules/phpfpm/manifests/pool.pp:63:5 on node somenode.ndsu.edu

This is phpfm::pool code in our module that's triggering that error:

  $pool_name    = 'www'
  $pool_port    = 9000
  $pool_status  = "/${pool_name}-status"
  $pool_ping    = "/${pool_name}-ping"
  $pool_timeout = '120'
  phpfpm::pool { $pool_name:
    ensure                 => present,
    listen                 => "127.0.0.1:${pool_port}",
    listen_allowed_clients => '127.0.0.1',
    pm_max_children        => 50,
    pm_start_servers       => 5,
    pm_min_spare_servers   => 5,
    pm_max_spare_servers   => 35,
    pm_status_path         => $pool_status,
    ping_path              => $pool_ping,
    chdir                  => false,
    slowlog                => '/var/log/php-fpm/www-slow.log',
    php_admin_value        => {
      'error_log'      => '/var/log/php-fpm/www-error.log',
      'include_path'   => '/var/local/php/include/apps:/var/local/php/include/global',
      'log_errors'     => 'on',
      'open_basedir'   => '/var/local/php/include/apps:/var/local/php/include/global:/var/www/html:/var/local/php/upload',
      'upload_tmp_dir' => '/var/local/php/upload',
    },
    php_value              => {
      'session.save_handler'  => 'files',
      'session.save_path'     => '/opt/rh/php54/root/var/lib/php/session',
    },
    service_name           => 'php54-php-fpm',
    pool_dir               => '/opt/rh/php54/root/etc/php-fpm.d',
    pool_template_file     => 'site/phpfpm/pool-redhat.conf.erb',
  }

It's only under parser = future that we see the error. It works fine using the traditional parser.

Anyone else run into issues when using phpfpm with the future parser?

Slashbunny commented 8 years ago

I am running Puppet 4.5.2 and I can't seem to reproduce this.

This is the if statement that is failing in pool.pp and triggering the error you're getting:

  if $pm_start_servers < $pm_min_spare_servers or
  $pm_start_servers > $pm_max_spare_servers {
    fail("pm_start_servers(${pm_start_servers}) must not be less than \
      pm_min_spare_servers(${pm_min_spare_servers}) and not greater than \
      pm_max_spare_servers(${pm_max_spare_servers})")
  }

However, we can clearly see in your error message that all 3 variables being checked are the correct values (5 is not less than 5, nor greater than 35). The only part of the docs that would explain this is:

https://docs.puppet.com/puppet/3.8/reference/future_lang_expressions.html#string-encoding-and-comparisons

Values are only considered equal if they have the same data type. Notably, this means that 1 == "1" is false, and "true" == true is false.

Which would seem to imply that one of those variables is a quoted string somewhere in your code and it's comparing it to an integer, which evaluates to false, triggering the error. In your above example, they aren't quoted, but maybe there is another pool definition somewhere else?

On Puppet 4.5.2, when I quote one of the variables, I get an error:

Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Comparison of: String < Integer, is not possible. Caused by 'A String is not comparable to a non String'.

I don't know if ~3.8 just silently evaluates to false instead or maybe there is something else going on- I just don't know what else could be causing this.

timmooney commented 8 years ago

Thanks for confirming that phpfpm works great with puppet 4.x. That's great news.

In all the other cases I've seen in 3.8.x since setting stringify_facts = true and parser = future, puppet has given very obvious message about string to integer comparison. This is the first case where it's not absolutely clear what the problem is.

I'll keep digging and see if I can figure out what it might be. I'll update the ticket if and when I find it.

timmooney commented 8 years ago

Of course @Slashbunny was correct; the error was coming from another pool definition.

Sorry for the noise, and very happy for the confirmation that phpfpm is being used already on puppet 4.x.