Full rewrite to support riak 2

danieldreier commented 9 years ago

This is effectively a new module that implements riak 2 support in place of the previous riak 1 support. It includes significant test coverage in rspec-puppet and beaker. I'm submitting this to kick off discussion. I really don't intend to just sort of take over, but since the riak 1 and 2 config files are different, we don't need riak 1 support anymore, and the new config file format is much simpler, it was more expedient to reimplement the functionality.

This replaces the current module with what I implemented in danieldreier/riak2. This is built on the puppet-module-skeleton template, so it comes with the rake tasks and layout that's become a defacto standard. Some of the files are still from that skeleton and may need to be improved before we move forward.

It also has beaker and rspec-puppet test coverage. I've run the beaker tests against:

centos-6-x86_64 centos-7-x86_64 debian-7-x86_64 ubuntu-1204-x86_64 ubuntu-1404-x86_64

At the moment, functionality is relatively basic:

(optionally, on by default) install using system packages on debian / EL 6/7
(optionally, on by default) install package repositories
run the riak service
manage any setting in riak.conf

As far as I'm aware, what this loses is:

ability to manage riak 1
ability to install via any mechanism other than packages
puppet 2.7 support
ability to manage ulimit in /etc/security/limits.conf

I'm intending to add more functionality, documentation, etc to this, but this provides a basic working version that I'd like to start getting community feedback on.

mbbroberg commented 9 years ago

This is absolutely incredible @danieldreier! I invited you to the @basho-labs/puppet-committers group because your work has been incredible in moving forward. @haf - I'd love your weigh in here as well of course! Huge respect to the both of you!

danieldreier commented 9 years ago

@haf if I understand correctly, I think that we can re-use your write_erl_config puppet function to generate advanced.config files, since they seem to be in the same format as the the old-style config files. Does that seem reasonable to you?

danieldreier commented 9 years ago

According to the puppet-dev mailing list, there's a puppet 4.0.0 release tentatively slated for next week. I'm going to make some changes to this code that will make it puppet 4 / puppet 3.7 w/ future parser only.

I'd really like to make this the first puppet 4 module -- I don't think there are any and I'm pretty sure we can get a mention in the press releases ("and there's even a puppet 4-only module already...") if we can move quickly enough.

danieldreier commented 9 years ago

okay, as of commit a4e2bd8 this passes beaker tests using puppet 4 / future parser DSL. As of https://github.com/danieldreier/puppet-riak/commit/045a240c356adbf2ee1b49f3ca384d53bdff38d8 the beaker tests pass in virtualbox as well.

Cornellio commented 9 years ago

@danieldreier Hi Daniel,

I'm getting ready to do a PR but could you take a look first and note any issues you see? This change just manages ulimits.

https://github.com/Cornellio/puppet-basho-riak

It's done via tuning.pp with a defined type. It's called from config.pp and has default values set in params, while allowing a user to pass custom values. There's a test manifest tuning_config.pp for testing custom parameters.

Beaker tests are passing so far on Debian 7 and CentOS 6. I'm still sorting out the rspec tests.

Also, I forgot to create a new branch for this.

Thanks

danieldreier commented 9 years ago

@Cornellio I'd be glad to help - thanks for working on this. You could handle the branch issue by doing a git checkout of the last point where they were the same, creating a branch from there, and then doing a git cherry-pick.

That would look something like:

git checkout bdcda1ccce6bef7230bc5ec44a95225f0b0eddb8
git checkout -b 'add_perf_tuning'
git cherry pick bdcda1ccce6bef7230bc5ec44a95225f0b0eddb8..58fcf7f31c386c727a8a7ef1deb40e3601d9d352

this will create a new branch, then cherry-pick all commits after bdcda1ccce6bef7230bc5ec44a95225f0b0eddb8 (the last common commit) to your most recent commit.

With regards to the actual content of the commit, riak::tuning::limits is a very clever way to wrap the augeas type. I'm not sure that the benefit is worth the additional complexity; I have kind of a hard time understanding what it's doing and what the failure cases might be. I'm a bit allergic to complexity in puppet code because of how limited the abstraction capabilities are. Fortunately, future parser gives us some really cool tools for this.

Given that we're ultimately only getting two things from the user (ulimits_nofile_soft, ulimits_nofile_hard), and they're de-facto namespaced together, let's pass a settings hash instead:

class { '::riak':
  ulimits => {
    soft  => 88536,
    hard => 98536,
  }
}

When we declare that in init.pp, we should take full advantage of the new type system. In this case we're expecting a hash of a string and then an integer, so we can prefix it with Hash[String,Integer] similar to the other parameters. That will cause the users to get meaningful error messages if they attempt to pass parameters of the wrong type, and prevents us from getting untyped user data that might do something unexpected.

We can then create a riak::tuning::limits class that accepts that hash and then iterates over it, something like:

class riak::tuning::ulimits {
  $settings = merge($::riak::params::ulimits,$::riak::ulimits)
  $settings.each |String[4,4] $setting, Integer $value| {
    $user = $::riak::user # don't set from params, user can't override params
    ## create augeas resource here ##
  }
}

Does that make sense? We're using String[4,4] to validate that it's a 4 character long string, and the value is (any) integer. Puppet with future parser gives much more useful error messages when you use explicit types; it's a great way to communicate what your expectations of sane input are.

My thinking is that we're going to have a lot more of these tuning settings, and this will provide a relatively flexible pattern that can be duplicated for any settings that can be provided in a list.

haf commented 9 years ago

@danieldreier yes, you can re-use it, afaik.

Cornellio commented 9 years ago

Yes, particularly I find the augeas resource complex.

I used a define since it can be evaluated multiple times with different parameters and am familiar with defines.

I'll experiment with the settings hash. I find that a bit complex so far, probably because I haven't seen it before.

Cornellio commented 9 years ago

@danieldreier Hi, I'm trying this but am having trouble. Could you take a look at this?

https://github.com/Cornellio/puppet-riak/tree/perf_tuning_b

It compiles fine but does not apply the resources in riak::tuning::ulimits

It seems to import ok:

puppet apply tests/init.pp --debug

Is showing good results, including:

Debug: Automatically imported riak::tuning::ulimits from riak/tuning into production

danieldreier commented 9 years ago

@cornellio I'd be glad to help - but will be away from computers for the weekend so I won't be getting to it until early next week. Have a good weekend and thanks for working on that, looking forward to having the functionality.

Cornellio commented 9 years ago

@danieldreier Hello, are there some docs and examples of Puppet 4 code? Your example makes sense but it would help if I could see more examples of how all the pieces connect.

basho-labs / puppet-riak

Full rewrite to support riak 2 #52