From 59fe631609de3543cbb9082f631ea0a5a118a472 Mon Sep 17 00:00:00 2001
From: Crag Wolfe <cwolfe@redhat.com>
Date: Fri, 1 May 2015 19:46:43 -0400
Subject: [PATCH] Updates to pcmk_resource and related manifests.

* Make sure pcmk_resource supports the various invocations to "pcs
  resource create" in https://github.com/beekhof/osp-ha-deploy.
* For now, only support creating the resource and deleting it, which
  is no worse than what existed before (the properties in
  pcmk_resource in the previous iteration did not work, at best some
  properties were attempted to be updated to what they already were
  every puppet run).
---
 lib/puppet/provider/pcmk_resource/default.rb | 264 ++++++++-----------
 lib/puppet/type/pcmk_resource.rb             |  66 ++---
 manifests/resource/filesystem.pp             |  64 ++++-
 manifests/resource/ip.pp                     |  51 +++-
 manifests/resource/lsb.pp                    |  36 +--
 manifests/resource/mysql.pp                  |  30 ---
 manifests/resource/ocf.pp                    |  63 +++--
 manifests/resource/qpid_clustered.pp         |  52 ----
 manifests/resource/route.pp                  |  32 ++-
 manifests/resource/service.pp                |  66 +++--
 manifests/resource/systemd.pp                |  36 +--
 11 files changed, 387 insertions(+), 373 deletions(-)
 delete mode 100644 manifests/resource/mysql.pp
 delete mode 100644 manifests/resource/qpid_clustered.pp

diff --git a/lib/puppet/provider/pcmk_resource/default.rb b/lib/puppet/provider/pcmk_resource/default.rb
index 7d3248fa..fecff488 100644
--- a/lib/puppet/provider/pcmk_resource/default.rb
+++ b/lib/puppet/provider/pcmk_resource/default.rb
@@ -1,156 +1,126 @@
 Puppet::Type.type(:pcmk_resource).provide(:default) do
-    desc 'A base resource definition for a pacemaker resource'
+  desc 'A base resource definition for a pacemaker resource'
 
-    ### overloaded methods
-    def create
-        cmd = 'resource create ' + @resource[:name] + ' ' + @resource[:resource_type] + ' ' + @resource[:resource_params] + ' op monitor interval=' + @resource[:interval]
-        if @resource[:monitor_params] and not @resource[:monitor_params].empty?
-            cmd += hash_to_params(@resource[:monitor_params])
-        end
-        # group defaults to empty
-        if not @resource[:group].empty?
-            cmd += ' --group ' + @resource[:group]
-        end
-        # clone defaults to false
-        if @resource[:clone]
-            cmd += ' --clone'
-        end
-        # do pcs create
-        pcs('create', cmd)
+  ### overloaded methods
+  def create
+    resource_params = @resource[:resource_params]
+    meta_params = @resource[:meta_params]
+    op_params = @resource[:op_params]
+    clone_params = @resource[:clone_params]
+    group_params = @resource[:group_params]
+    master_params = @resource[:master_params]
+
+    suffixes = 0
+    if clone_params then suffixes +=1 end
+    if master_params then suffixes +=1 end
+    if group_params then suffixes +=1 end
+    if suffixes > 1
+      raise(Puppet::Error, "May only define one of clone_params, "+
+            "master_params and group_params")
     end
 
-    def destroy
-        cmd = 'resource delete ' + @resource[:name]
-        pcs('delete', cmd)
+    # Build the 'pcs resource create' command.  Check out the pcs man page :-)
+    cmd = 'resource create ' + @resource[:name]+' ' +@resource[:resource_type]
+    if not_empty_string(resource_params)
+      cmd += ' ' + resource_params
+    end
+    if not_empty_string(meta_params)
+      cmd += ' meta ' + meta_params
+    end
+    if not_empty_string(op_params)
+      cmd += ' op ' + op_params
+    end
+    if clone_params
+      cmd += ' --clone'
+      if not_empty_string(clone_params)
+        cmd += ' ' + clone_params
+      end
+    end
+    if not_empty_string(group_params)
+      cmd += ' --group ' + group_params
+    end
+    if master_params
+      cmd += ' --master'
+      if not_empty_string(master_params)
+        cmd += ' ' + master_params
+      end
     end
 
-    def exists?
-        cmd = 'resource show ' + @resource[:name] + ' > /dev/null 2>&1'
-        pcs('show', cmd)
+    # do pcs create
+    pcs('create', cmd)
+  end
+
+  def destroy
+    cmd = 'resource delete ' + @resource[:name]
+    pcs('delete', cmd)
+  end
+
+  def exists?
+    cmd = 'resource show ' + @resource[:name] + ' > /dev/null 2>&1'
+    pcs('show', cmd)
+  end
+
+
+  ### property methods
+
+  # It isn't an easy road if you want to make these true
+  # puppet-like resource properties.  Here is a start if you are feeling brave:
+  # https://github.com/cwolferh/puppet-pacemaker/blob/pcmk_resource_improvements_try0/lib/puppet/provider/pcmk_resource/default.rb#L64
+  def resource_params
+    @resource[:resource_params]
+  end
+
+  def resource_params=(value)
+  end
+
+  def op_params
+    @resource[:op_params]
+  end
+
+  def op_params=(value)
+  end
+
+  def meta_params
+    @resource[:meta_params]
+  end
+
+  def meta_params=(value)
+  end
+
+  def group_params
+    @resource[:group_params]
+  end
+
+  def group_params=(value)
+  end
+
+  def master_params
+    @resource[:master_params]
+  end
+
+  def master_params=(value)
+  end
+
+  def clone_params
+    @resource[:clone_params]
+  end
+
+  def clone_params=(value)
+  end
+
+  def not_empty_string(p)
+    p && p.kind_of?(String) && ! p.empty?
+  end
+
+  def pcs(name, cmd)
+    Puppet.debug("/usr/sbin/pcs #{cmd}")
+    pcs_out = `/usr/sbin/pcs #{cmd}`
+    if $?.exitstatus != 0 && pcs_out.lines.first && ! name.include?('show')
+      Puppet.debug("Error: #{pcs_out}")
+      raise Puppet::Error, "pcs #{name} failed: #{pcs_out.lines.first.chomp!}" if $?.exitstatus
     end
+    # return output for good exit or false for failure.
+    $?.exitstatus == 0 ? pcs_out : false
+  end
 
-
-    ### property methods
-    def resource_params
-        cmd = 'resource show ' + @resource[:name]
-        get_attrs = pcs('get interval', cmd)
-
-        # find the Attributes
-        for line in get_attrs.lines.each do
-            return (line.scan /Attributes: (.+?)$/m)[0][0].strip if line.include? 'Attributes:'
-        end
-        # return empty string if Attributes not found
-        ''
-    end
-
-    def resource_params=(value)
-        cmd = 'resource update ' + @resource[:name] + ' ' + value
-        pcs('update attributes', cmd)
-    end
-
-    def group
-        # get the list of groups and their resources
-        cmd = 'resource --groups'
-        resource_groups = pcs('group list', cmd)
-
-        # find the group that has the resource in it
-        for group in resource_groups.lines.each do
-            return group[0, /:/ =~ group] if group.include? @resource[:name]
-        end
-        # return empty string if a group wasn't found
-        # that includes the resource in it.
-        ''
-    end
-
-    def group=(value)
-        if value.empty?
-            cmd = 'resource ungroup ' + group + ' ' + @resource[:name]
-            pcs('ungroup', cmd)
-        else
-            cmd = 'resource group add ' + value + ' ' + @resource[:name]
-            pcs('group add', cmd)
-        end
-    end
-
-    def clone
-        cmd = 'resource show ' + @resource[:name] + '-clone > /dev/null 2>&1'
-        pcs('show clone', cmd) == false ? false : true
-    end
-
-    def clone=(value)
-        if not value
-            cmd = 'resource unclone ' + @resource[:name]
-            pcs('unclone', cmd)
-        else
-            cmd = 'resource clone ' + @resource[:name]
-            pcs('clone', cmd)
-        end
-    end
-
-    def interval
-        cmd = 'resource show ' + @resource[:name]
-        get_interval = pcs('get interval', cmd)
-
-        # find the interval value
-        for line in get_interval.lines.each do
-            return (line.scan /interval=(.+?) /m)[0][0] if line.include? 'interval='
-        end
-        # return empty string if an interval value wasn't found
-        ''
-    end
-
-    def interval=(value)
-        cmd = 'resource update ' + @resource[:name] + ' op monitor interval=' + value
-        pcs('update interval', cmd)
-    end
-
-    def monitor_params
-        cmd = 'resource show ' + @resource[:name]
-        pcs_output = pcs('get monitor params', cmd)
-
-        pcs_output.each_line do |line|
-            line.strip.match(/(Operations: )?monitor ([^(]+)/) do |match|
-                Puppet.debug(match.inspect)
-                return params_to_hash(match[2])
-            end
-        end
-        # return empty string if monitor params not found
-        ''
-    end
-
-    def monitor_params=(value)
-        cmd = 'resource update ' + @resource[:name] + ' op monitor ' + hash_to_params(value)
-        pcs('update interval', cmd)
-    end
-
-    private
-
-    def pcs(name, cmd)
-        Puppet.debug("/usr/sbin/pcs #{cmd}")
-        pcs_out = `/usr/sbin/pcs #{cmd}`
-        #puts name
-        #puts $?.exitstatus
-        if $?.exitstatus != 0 and pcs_out.lines.first and not name.include? 'show'
-            Puppet.debug("Error: #{pcs_out}")
-            raise Puppet::Error, "pcs #{name} failed: #{pcs_out.lines.first.chomp!}" if $?.exitstatus
-        end
-        # return output for good exit or false for failure.
-        $?.exitstatus == 0 ? pcs_out : false
-    end
-
-    def params_to_hash(str)
-        str.split.reduce({}) do |hash, param|
-            k,v = param.split '='
-            hash[k] = v
-            hash
-        end
-    end
-
-    def hash_to_params(hash)
-        params = ''
-        hash.each_pair do |k,v|
-            params += " #{k}=#{v}"
-        end
-        params
-    end
 end
diff --git a/lib/puppet/type/pcmk_resource.rb b/lib/puppet/type/pcmk_resource.rb
index 192338a3..9e14b7b2 100644
--- a/lib/puppet/type/pcmk_resource.rb
+++ b/lib/puppet/type/pcmk_resource.rb
@@ -1,44 +1,30 @@
 Puppet::Type.newtype(:pcmk_resource) do
-    @doc = "Base resource definition for a pacemaker resource"
+  @doc = "Base resource definition for a pacemaker resource"
 
-    ensurable
+  ensurable
 
-    newparam(:name) do
-        desc "A unique name for the resource"
-    end
-
-    newparam(:resource_type) do
-        desc "the pacemaker type to create"
-    end
-    newproperty(:resource_params) do
-        desc "extra parameters to the retource group"
-    end
-    newproperty(:group) do
-        desc "A resource group to put the resource in"
-    end
-    newproperty(:clone) do
-        desc "set if this is a cloned resource"
-        defaultto false
-    end
-    newproperty(:interval) do
-        desc "resource check interval"
-        defaultto "30s"
-    end
-    newproperty(:monitor_params) do
-        desc "extra parameters for monitor operation"
-
-        validate do |value|
-            unless value.is_a? Hash
-                raise ArgumentError, "monitor_params must be a hash, not #{value.inspect}"
-            end
-        end
-
-        munge do |value|
-            if resource.parameters[:interval].value
-                value.merge!({'interval' => resource.parameters[:interval].value})
-            else
-                value
-            end
-        end
-    end
+  newparam(:name) do
+    desc "A unique name for the resource"
+  end
+  newparam(:resource_type) do
+    desc "the pacemaker type to create"
+  end
+  newproperty(:op_params) do
+    desc "op parameters"
+  end
+  newproperty(:meta_params) do
+    desc "meta parameters"
+  end
+  newproperty(:resource_params) do
+    desc "resource parameters"
+  end
+  newproperty(:clone_params) do
+    desc "clone params"
+  end
+  newproperty(:group_params) do
+    desc "A resource group to put the resource in"
+  end
+  newproperty(:master_params) do
+    desc "set if this is a cloned resource"
+  end
 end
diff --git a/manifests/resource/filesystem.pp b/manifests/resource/filesystem.pp
index c8974091..3db5fecd 100644
--- a/manifests/resource/filesystem.pp
+++ b/manifests/resource/filesystem.pp
@@ -1,12 +1,48 @@
-define pacemaker::resource::filesystem($device,
-                                       $directory,
-                                       $fsoptions='',
-                                       $fstype,
-                                       $group='',
-                                       $clone=false,
-                                       $interval='30s',
-                                       $monitor_params=undef,
-                                       $ensure='present') {
+# == Define Resource Type: pacemaker::resource::filesystem
+#
+# A resource type to create pacemaker Filesystem resources, provided
+# for convenience.
+#
+# === Parameters
+#
+# [*name*]
+# The name of the pacemaker resource, i.e. as seen in "pcs status".
+#
+# [*device*]
+# The device which is being mounted.  E.g. 192.168.200.100:/export/foo.
+#
+# [*directory*]
+# Where to mount the device (the empty dir must already exist).
+#
+# [*fstype*]
+# As you would pass to mount.  E.g., nfs.
+#
+# [*meta_params*]
+# Additional meta parameters to pass to "pcs create".  Optional.
+#
+# [*op_params*]
+# Additional op parameters to pass to "pcs create".  Optional.
+#
+# [*clone_params*]
+# Additional clone parameters to pass to "pcs create".  Use '' or true
+# for to pass --clone to "pcs resource create" with no addtional clone
+# parameters.  Optional.
+#
+# [*group_params*]
+# Additional group parameters to pass to "pcs create", typically just
+# the the name of the pacemaker resource group. Optional.
+
+define pacemaker::resource::filesystem(
+  $ensure       = 'present',
+  $device       = '',
+  $directory    = '',
+  $fsoptions    = '',
+  $fstype       = '',
+  $meta_params  = undef,
+  $op_params    = '',
+  $clone_params = undef,
+  $group_params = undef,
+) {
   $resource_id = delete("fs-${directory}", '/')
 
   $resource_params = $fsoptions ? {
@@ -15,12 +51,12 @@ define pacemaker::resource::filesystem($device,
   }
 
   pcmk_resource { $resource_id:
+    ensure          => $ensure,
     resource_type   => 'Filesystem',
     resource_params => $resource_params,
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
-    ensure          => $ensure,
+    meta_params     => $meta_params,
+    op_params       => $op_params,
+    clone_params    => $clone_params,
+    group_params    => $group_params,
   }
 }
diff --git a/manifests/resource/ip.pp b/manifests/resource/ip.pp
index 282e4bf5..7dfa8ae2 100644
--- a/manifests/resource/ip.pp
+++ b/manifests/resource/ip.pp
@@ -1,23 +1,50 @@
-define pacemaker::resource::ip($ip_address,
-                               $cidr_netmask=32,
-                               $nic='',
-                               $group='',
-                               $interval='30s',
-                               $monitor_params=undef,
-                               $ensure='present') {
+# == Define Resource Type: pacemaker::resource::ip
+#
+# A resource type to create pacemaker IPaddr2 resources, provided
+# for convenience.
+#
+# === Parameters
+#
+# [*name*]
+# The name of the pacemaker resource, i.e. as seen in "pcs status".
+#
+# [*ip_address*]
+# The virtual IP address you want pacemaker to create and manage.
+#
+# [*cidr_netmask*]
+# The netmask to use in the cidr= option in the "pcs resource create"
+# command. Optional.  Default is '32'.
+#
+# [*nic*]
+# The nic to use in the nic= option in the "pcs resource create"
+# command.  Optional.
+#
+# [*group_params*]
+# Additional group parameters to pass to "pcs create", typically just
+# the the name of the pacemaker resource group. Optional.
+#
+define pacemaker::resource::ip(
+  $ensure       = 'present',
+  $ip_address   = undef,
+  $cidr_netmask = '32',
+  $nic          = '',
+  $group_params = '',
+  ) {
 
+  $cidr_option = $cidr_netmask ? {
+      ''      => '',
+      default => " cidr_netmask=${cidr_netmask}"
+  }
   $nic_option = $nic ? {
       ''      => '',
-      default => " nic=$nic"
+      default => " nic=${nic}"
   }
 
   pcmk_resource { "ip-${ip_address}":
     ensure          => $ensure,
     resource_type   => 'IPaddr2',
-    resource_params => "ip=${ip_address} cidr_netmask=${cidr_netmask}${nic_option}",
-    group           => $group,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
+    resource_params => "ip=${ip_address}${cidr_option}${nic_option}",
+    group_params    => $group_params,
   }
 
 }
diff --git a/manifests/resource/lsb.pp b/manifests/resource/lsb.pp
index a3894ef3..b2528c67 100644
--- a/manifests/resource/lsb.pp
+++ b/manifests/resource/lsb.pp
@@ -1,18 +1,24 @@
-define pacemaker::resource::lsb($group='',
-                                $clone=false,
-                                $interval='30s',
-                                $monitor_params=undef,
-                                $ensure='present',
-                                $options='') {
-
-  pcmk_resource { "${name}":
+# == Define Resource Type: pacemaker::resource::lsb
+#
+# See pacemaker::resource::service.  Typical usage is to declare
+# pacemaker::resource::service rather than this resource directly.
+#
+define pacemaker::resource::lsb(
+  $ensure          = 'present',
+  $service_name    = $name,
+  $resource_params = '',
+  $meta_params     = '',
+  $op_params       = '',
+  $clone_params    = undef,
+  $group_params    = undef,
+) {
+  pcmk_resource { $name:
     ensure          => $ensure,
-    resource_type   => "lsb:${name}",
-    resource_params => $options,
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
+    resource_type   => "lsb:${service_name}",
+    resource_params => $resource_params,
+    meta_params     => $meta_params,
+    op_params       => $op_params,
+    clone_params    => $clone_params,
+    group_params    => $group_params,
   }
-
 }
diff --git a/manifests/resource/mysql.pp b/manifests/resource/mysql.pp
deleted file mode 100644
index 4e1898f6..00000000
--- a/manifests/resource/mysql.pp
+++ /dev/null
@@ -1,30 +0,0 @@
-define pacemaker::resource::mysql($name,
-                                  $group='',
-                                  $clone=false,
-                                  $interval='30s',
-                                  $monitor_params=undef,
-                                  $stickiness=0,
-                                  $ensure='present',
-                                  $additional_params='',
-                                  $replication_user='',
-                                  $replication_passwd='', 
-                                  $max_slave_lag=0,
-                                  $evict_outdated_slaves=false,
-                                  $enable_creation=true) {
-
-  $replication_options = $replication_user ? {
-      ''      => '',
-      default => " replication_user=$replication_user replication_passwd=$replication_passwd max_slave_lag=$max_slave_lag evict_outdated_slaves=$evict_outdated_slaves"
-  }
-
-  pcmk_resource { "mysql-${name}":
-    resource_type   => 'mysql',
-    resource_params => "enable_creation=${enable_creation}${replication_options} ${additional_params}",
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
-    ensure          => $ensure,
-  }
-
-}
diff --git a/manifests/resource/ocf.pp b/manifests/resource/ocf.pp
index 6bd0395e..fd7cf632 100644
--- a/manifests/resource/ocf.pp
+++ b/manifests/resource/ocf.pp
@@ -1,19 +1,50 @@
-define pacemaker::resource::ocf($group='',
-                                $clone=false,
-                                $interval='30s',
-                                $monitor_params=undef,
-                                $ensure='present',
-                                $options='',
-                                $resource_name='') {
-
-  pcmk_resource { "${name}":
+# == Define Resource Type: pacemaker::resource::ocf
+#
+# A resource type to create pacemaker ocf resources, provided for
+# convenience.
+#
+# === Parameters
+#
+# [*name*]
+# The name of the pacemaker resource, i.e. as seen in "pcs status".
+#
+# [*ocf_agent_name*]
+# The name of the ocf resource agent.  Optional.  Defaults to *name*.
+#
+# [*resource_params*]
+# Additional resource parameters to pass to "pcs create".  Optional.
+#
+# [*op_params*]
+# Additional op parameters to pass to "pcs create".  Optional.
+#
+# [*meta_params*]
+# Additional meta parameters to pass to "pcs create".  Optional.
+#
+# [*clone_params*]
+# Additional clone parameters to pass to "pcs create".  Use '' or true
+# for to pass --clone to "pcs resource create" with no addtional clone
+# parameters.  Optional.
+#
+# [*group_params*]
+# Additional group parameters to pass to "pcs create", typically just
+# the the name of the pacemaker resource group. Optional.
+#
+define pacemaker::resource::ocf(
+  $ensure          = 'present',
+  $ocf_agent_name  = $name,
+  $resource_params = '',
+  $meta_params     = '',
+  $op_params       = '',
+  $clone_params    = undef,
+  $group_params    = undef,
+) {
+  pcmk_resource { $name:
     ensure          => $ensure,
-    resource_type   => "ocf:${resource_name}",
-    resource_params => $options,
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
+    resource_type   => "ocf:${ocf_agent_name}",
+    resource_params => $resource_params,
+    meta_params     => $meta_params,
+    op_params       => $op_params,
+    clone_params    => $clone_params,
+    group_params    => $group_params,
   }
-
 }
diff --git a/manifests/resource/qpid_clustered.pp b/manifests/resource/qpid_clustered.pp
deleted file mode 100644
index 18ce8a1f..00000000
--- a/manifests/resource/qpid_clustered.pp
+++ /dev/null
@@ -1,52 +0,0 @@
-# This class should be included on all nodes that are
-# part of a qpid cluster
-# It ensures that the qpid cluster package is installed
-# and that the nessesary configs are inplace for
-# the qpid clustering to start using the pacemaker
-# corosync instance
-# The pacemaker resource is created as a cloned resource
-# so that pacemaker starts qpid on all the cluster's nodes
-
-define pacemaker::resource::qpid_clustered($name,
-                                 $cluster_name,
-                                 $clone=true,
-                                 $group='',
-                                 $interval='30s',
-                                 $monitor_params=undef,
-                                 $stickiness=0,
-                                 $ensure='present') {
-
-  package { "qpid-cpp-server-cluster":
-    ensure => installed,
-  }
-
-  file_line { 'Set Qpid Cluster Name':
-    path    => '/etc/qpidd.conf',
-    match   => '^[ ]*cluster_name=',
-    line    => "cluster_name='${cluster_name}'",
-  }
-
-  # TODO: this should be replaced with an exec once
-  # https://bugzilla.redhat.com/show_bug.cgi?id=1019368
-  # has been completed
-  augeas { "uidgid in cluster.conf":
-    lens    => "Xml.lns",
-    incl    => "/etc/cluster/cluster.conf",
-    changes => [
-      "set cluster/uidgid/#attribute/uid qpidd",
-      "set cluster/uidgid/#attribute/gid qpidd",
-    ],
-    require => Package['qpid-cpp-server-cluster'],
-  }
-
-  pcmk_resource { "lsb-qpidd":
-    resource_type   => "lsb:qpidd",
-    resource_params => "",
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
-    ensure          => $ensure,
-  }
-
-}
diff --git a/manifests/resource/route.pp b/manifests/resource/route.pp
index b62ae2a5..d67f57a3 100644
--- a/manifests/resource/route.pp
+++ b/manifests/resource/route.pp
@@ -1,41 +1,39 @@
-define pacemaker::resource::route($src='',
-                               $dest='',
-                               $gateway='',
-                               $nic='',
-                               $clone= false,
-                               $group='',
-                               $interval='30s',
-                               $monitor_params=undef,
-                               $ensure='present') {
+define pacemaker::resource::route(
+  $ensure       = 'present'
+  $src          = '',
+  $dest         = '',
+  $gateway      = '',
+  $nic          = '',
+  $clone_params = undef,
+  $group_params = undef,
+) {
 
   $nic_option = $nic ? {
       ''      => '',
-      default => " device=$nic"
+      default => " device=${nic}"
   }
 
   $src_option = $src ? {
       ''      => '',
-      default => " source=$src"
+      default => " source=${src}"
   }
 
   $dest_option = $dest ? {
       ''      => '',
-      default => " destination=$dest"
+      default => " destination=${dest}"
   }
 
   $gw_option = $gateway ? {
       ''      => '',
-      default => " gateway=$gateway"
+      default => " gateway=${gateway}"
   }
 
   pcmk_resource { "route-${name}-${group}":
     ensure          => $ensure,
     resource_type   => 'Route',
     resource_params => "${dest_option} ${src_option} ${nic_option} ${gw_option}",
-    group           => $group,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
-    clone           => $clone,
+    group_params    => $group_params,
+    clone_params    => $clone_params,
   }
 
 }
diff --git a/manifests/resource/service.pp b/manifests/resource/service.pp
index 15f9643b..10fc4aad 100644
--- a/manifests/resource/service.pp
+++ b/manifests/resource/service.pp
@@ -1,19 +1,55 @@
-define pacemaker::resource::service($group='',
-                                    $clone=false,
-                                    $interval='30s',
-                                    $monitor_params=undef,
-                                    $ensure='present',
-                                    $options='') {
-
+# == Define Resource Type: pacemaker::resource::service
+#
+# A resource type to create pacemaker lsb or systemd resources
+# (depending on distro), provided for convenience.
+#
+# === Parameters
+#
+# [*name*]
+# The name of the pacemaker resource, i.e. as seen in "pcs status".
+#
+# [*service_name*]
+# The name of the systemd or lsb service.  Optional.  Defaults to *name*.
+#
+# [*resource_params*]
+# Additional resource parameters to pass to "pcs create".  Optional.
+#
+# [*op_params*]
+# Additional op parameters to pass to "pcs create".  Optional.
+#
+# [*meta_params*]
+# Additional meta parameters to pass to "pcs create".  Optional.
+#
+# [*clone_params*]
+# Additional clone parameters to pass to "pcs create".  Use '' or true
+# for to pass --clone to "pcs resource create" with no addtional clone
+# parameters.  Optional.
+#
+# [*group_params*]
+# Additional group parameters to pass to "pcs create", typically just
+# the the name of the pacemaker resource group. Optional.
+#
+define pacemaker::resource::service(
+  $ensure          = 'present',
+  $service_name    = $name,
+  $resource_params = '',
+  $meta_params     = '',
+  $op_params       = '',
+  $clone_params    = undef,
+  $group_params    = undef,
+) {
   include ::pacemaker::params
   $res = "pacemaker::resource::${::pacemaker::params::services_manager}"
+
   create_resources($res,
-    { "$name" => { group    => $group,
-                  clone    => $clone,
-                  interval => $interval,
-                  monitor_params => $monitor_params,
-                  ensure   => $ensure,
-                  options  => $options,
-                }
-    })
+    { "${name}" => {
+      ensure          => $ensure,
+      service_name    => $service_name,
+      resource_params => $resource_params,
+      meta_params     => $meta_params,
+      op_params       => $op_params,
+      clone_params    => $clone_params,
+      group_params    => $group_params,
+    }
+  })
 }
diff --git a/manifests/resource/systemd.pp b/manifests/resource/systemd.pp
index f24e2bf8..4da97b45 100644
--- a/manifests/resource/systemd.pp
+++ b/manifests/resource/systemd.pp
@@ -1,18 +1,24 @@
-define pacemaker::resource::systemd($group='',
-                                $clone=false,
-                                $interval='30s',
-                                $monitor_params=undef,
-                                $ensure='present',
-                                $options='') {
-
-  pcmk_resource { "${name}":
+# == Define Resource Type: pacemaker::resource::systemd
+#
+# See pacemaker::resource::service.  Typical usage is to declare
+# pacemaker::resource::service rather than this resource directly.
+#
+define pacemaker::resource::systemd(
+  $ensure          = 'present',
+  $service_name    = "{$name}",
+  $resource_params = '',
+  $meta_params     = '',
+  $op_params       = '',
+  $clone_params    = undef,
+  $group_params    = undef,
+) {
+  pcmk_resource { $name:
     ensure          => $ensure,
-    resource_type   => "systemd:${name}",
-    resource_params => $options,
-    group           => $group,
-    clone           => $clone,
-    interval        => $interval,
-    monitor_params  => $monitor_params,
+    resource_type   => "systemd:${service_name}",
+    resource_params => $resource_params,
+    meta_params     => $meta_params,
+    op_params       => $op_params,
+    clone_params    => $clone_params,
+    group_params    => $group_params,
   }
-
 }