Use crm_diff for pcmk resource deep_compare

Detect if crm_diff is fixed and if it is favour the crm_diff resource
comparison. Also introduce a 'update_settle_secs' parameter
for resources that can be used to override the amount of time (s)
to wait for the cluster to settle after updating a resource.

Tested this as follows:
1) Normal deploys work fine (the code never gets triggered anyway)
2) Redeploy with no changes -> pcmk resources are untouched
   (the code does not get triggered anyway)
3) Redeploy by adding the following hiera keys:
   ExtraConfig:
     pacemaker::resource::bundle::deep_compare: true
     pacemaker::resource::ip::deep_compare: true
     pacemaker::resource::ocf::deep_compare: true
   -> pcmk resources are untouched
4) Redeploy with keys at 3) but with changes that trigger a restart
   -> pcmk resources are restarted and cluster waits for the
      restart before continuing

Change-Id: I62d5662327333e17e1c32a029695b3d3a904ca10

lib/puppet/provider/pcmk_bundle/default.rb

@@ -64,7 +64,7 @@ Puppet::Type.type(:pcmk_bundle).provide(:default) do
   def create_bundle_and_location(location_rule, needs_update=false)
     cmd = build_pcs_bundle_cmd()
     if needs_update then
-      pcmk_update_resource(@resource, cmd)
+      pcmk_update_resource(@resource, cmd, @resource[:update_settle_secs])
     else
       if location_rule then
         pcs('create', @resource[:name], "#{cmd} --disabled", @resource[:tries],

lib/puppet/provider/pcmk_common.rb

@@ -278,6 +278,32 @@ def is_crm_diff_buggy?
   raise Puppet::Error, "#{cmd} failed with (#{ret}): #{cmd_out}"
 end
 
+# same as pcmk_restart_resource? but using crm_diff
+def pcmk_restart_resource_ng?(resource_name, cib)
+  cmd = "/usr/sbin/crm_diff --cib -o #{cib}.orig -n #{cib}"
+  cmd_out = `#{cmd}`
+  ret = $?.exitstatus
+  # crm_diff returns 0 for no differences, 1 for differences, other return codes
+  # for errors
+  if not [0, 1].include? ret
+    delete_cib(cib)
+    raise Puppet::Error, "#{cmd} failed with (#{ret}): #{cmd_out}"
+  end
+  # If crm_diff says there are no differences (ret code 0), we can just
+  # exit and state that nothing needs restarting
+  return false if ret == 0
+  # In case the return code is 1 we will need to make sure that the resource
+  # we were passed is indeed involved in the change detected by crm_diff
+  graph_doc = REXML::Document.new cmd_out
+  # crm_diff --cib -o cib-orig.xml -n cib-vip-update.xml | \
+  #   xmllint --xpath '/diff/change[@operation and contains(@path, "ip-192.168.24.6")]/change-result' -
+  xpath_query = "/diff/change[@operation and contains(@path, \"#{resource_name}\")]/change-result"
+  REXML::XPath.each(graph_doc, xpath_query) do |element|
+    return true
+  end
+  return false
+end
+
 # This given a cib and a resource name, this method returns true if pacemaker
 # will restart the resource false if no action will be taken by pacemaker
 # Note that we need to leverage crm_simulate instead of crm_diff due to:
@@ -315,8 +341,13 @@ def pcmk_resource_has_changed?(resource, cmd_create, is_bundle=false)
     delete_cib(cib)
     raise Puppet::Error, "#{cmd_create} returned error. This should never happen."
   end
-  ret = pcmk_restart_resource?(resource[:name], cib, is_bundle)
-  Puppet.debug("pcmk_resource_has_changed returned #{ret}")
+  if is_crm_diff_buggy?
+    ret = pcmk_restart_resource?(resource[:name], cib, is_bundle)
+    Puppet.debug("pcmk_resource_has_changed returned #{ret}")
+  else
+    ret = pcmk_restart_resource_ng?(resource[:name], cib)
+    Puppet.debug("pcmk_resource_has_changed (ng version) returned #{ret}")
+  end
   delete_cib(cib)
   return ret
 end
@@ -324,7 +355,7 @@ end
 # This function will update a resource by making a cib backup
 # removing the resource and readding it and the push the CIB
 # to the cluster
-def pcmk_update_resource(resource, cmd_create)
+def pcmk_update_resource(resource, cmd_create, settle_timeout_secs=600)
   cib = backup_cib()
   cmd_delete = "resource delete #{resource[:name]}"
   ret = pcs_offline(cmd_delete, cib)
@@ -346,4 +377,9 @@ def pcmk_update_resource(resource, cmd_create)
     end
   end
   push_cib_offline(cib, resource[:tries], resource[:try_sleep], resource[:post_success_sleep])
+  cmd = "/usr/bin/timeout #{settle_timeout_secs} /usr/sbin/crm_resource --wait"
+  cmd_out = `#{cmd}`
+  ret = $?.exitstatus
+  Puppet.debug("pcmk_update_resource: #{cmd} returned (#{ret}): #{cmd_out}")
+  delete_cib(cib)
 end

lib/puppet/provider/pcmk_resource/default.rb

@@ -72,7 +72,7 @@ Puppet::Type.type(:pcmk_resource).provide(:default) do
   def create_resource_and_location(location_rule, needs_update=false)
     cmd = build_pcs_resource_cmd()
     if needs_update then
-      pcmk_update_resource(@resource, cmd)
+      pcmk_update_resource(@resource, cmd, @resource[:update_settle_secs])
     else
       if location_rule then
         pcs('create', @resource[:name], "#{cmd} --disabled", @resource[:tries],

lib/puppet/type/pcmk_bundle.rb

@@ -134,4 +134,22 @@ Puppet::Type.newtype(:pcmk_bundle) do
 
     defaultto false
   end
+
+  newparam(:update_settle_secs) do
+    desc "The time in seconds to wait for the cluster to settle after resource has been updated
+          when :deep_compare kicked in.  Defaults to '600'."
+
+    munge do |value|
+      if value.is_a?(String)
+        unless value =~ /^[-\d.]+$/
+          raise ArgumentError, "update_settle_secs must be a number"
+        end
+        value = Float(value)
+      end
+      raise ArgumentError, "update_settle_secs cannot be a negative number" if value < 0
+      value
+    end
+
+    defaultto 600
+  end
 end

lib/puppet/type/pcmk_resource.rb

@@ -128,4 +128,22 @@ Puppet::Type.newtype(:pcmk_resource) do
 
     defaultto false
   end
+
+  newparam(:update_settle_secs) do
+    desc "The time in seconds to wait for the cluster to settle after resource has been updated
+          when :deep_compare kicked in.  Defaults to '600'."
+
+    munge do |value|
+      if value.is_a?(String)
+        unless value =~ /^[-\d.]+$/
+          raise ArgumentError, "update_settle_secs must be a number"
+        end
+        value = Float(value)
+      end
+      raise ArgumentError, "update_settle_secs cannot be a negative number" if value < 0
+      value
+    end
+
+    defaultto 600
+  end
 end

manifests/resource/bundle.pp

@@ -86,6 +86,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::bundle::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -126,6 +132,7 @@ define pacemaker::resource::bundle(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = hiera('pacemaker::resource::bundle::deep_compare', false),
+  $update_settle_secs = hiera('pacemaker::resource::bundle::update_settle_secs', 600),
   ) {
   if $image == undef {
     fail("Cannot create bundle ${name} without specifying an image")
@@ -153,5 +160,6 @@ define pacemaker::resource::bundle(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

manifests/resource/filesystem.pp

@@ -85,6 +85,13 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::filesystem::update_settle_secs', 600) (seconds)
+#
+#
 # === Dependencies
 #
 #  None
@@ -127,6 +134,7 @@ define pacemaker::resource::filesystem(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::filesystem::update_settle_secs', 600),
 ) {
   $resource_id = delete("fs-${directory}", '/')
 
@@ -150,5 +158,6 @@ define pacemaker::resource::filesystem(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

manifests/resource/ip.pp

@@ -78,7 +78,13 @@
 #   (optional) Enable deep comparing of resources and bundles
 #   When set to true a resource will be compared in full (options, meta parameters,..)
 #   to the existing one and in case of difference it will be repushed to the CIB
-#   Defaults to false
+#   Defaults to hiera('pacemaker::resource::ip::deep_compare', false)
+#
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::ip::update_settle_secs', 600) (seconds)
 #
 # === Dependencies
 #
@@ -121,6 +127,7 @@ define pacemaker::resource::ip(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = hiera('pacemaker::resource::ip::deep_compare', false),
+  $update_settle_secs = hiera('pacemaker::resource::ip::update_settle_secs', 600),
   ) {
   if !is_ipv6_address($ip_address) and $ipv6_addrlabel != '' {
     fail("ipv6_addrlabel ${ipv6_addrlabel} was specified, but ${ip_address} is not an ipv6 address")
@@ -157,6 +164,7 @@ define pacemaker::resource::ip(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 
 }

manifests/resource/lsb.pp

@@ -76,6 +76,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::lsb::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -116,6 +122,7 @@ define pacemaker::resource::lsb(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::lsb::update_settle_secs', 600),
 ) {
   pcmk_resource { $name:
     ensure             => $ensure,
@@ -132,5 +139,6 @@ define pacemaker::resource::lsb(
     verify_on_create   => $verify_on_create,
     location           => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

manifests/resource/ocf.pp

@@ -80,6 +80,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::ocf::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -121,6 +127,7 @@ define pacemaker::resource::ocf(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = hiera('pacemaker::resource::ocf::deep_compare', false),
+  $update_settle_secs = hiera('pacemaker::resource::ocf::update_settle_secs', 600),
 ) {
   pcmk_resource { $name:
     ensure             => $ensure,
@@ -138,5 +145,6 @@ define pacemaker::resource::ocf(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

manifests/resource/remote.pp

@@ -65,6 +65,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::remote::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -102,6 +108,7 @@ define pacemaker::resource::remote(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::remote::update_settle_secs', 600),
 ) {
   pcmk_resource { $name:
     ensure             => $ensure,
@@ -117,5 +124,6 @@ define pacemaker::resource::remote(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

manifests/resource/route.pp

@@ -75,6 +75,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::route::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -115,6 +121,7 @@ define pacemaker::resource::route(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::route::update_settle_secs', 600),
 ) {
 
   $nic_option = $nic ? {
@@ -150,6 +157,7 @@ define pacemaker::resource::route(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 
 }

manifests/resource/service.pp

@@ -76,6 +76,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::service::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -116,6 +122,7 @@ define pacemaker::resource::service(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::service::update_settle_secs', 600),
 ) {
   include ::pacemaker::params
   $res = "pacemaker::resource::${::pacemaker::params::services_manager}"
@@ -136,6 +143,7 @@ define pacemaker::resource::service(
       verify_on_create   => $verify_on_create,
       location_rule      => $location_rule,
       deep_compare       => $deep_compare,
+      update_settle_secs => $update_settle_secs,
     }
   })
 }

manifests/resource/systemd.pp

@@ -76,6 +76,12 @@
 #   to the existing one and in case of difference it will be repushed to the CIB
 #   Defaults to false
 #
+# [*update_settle_secs*]
+#   (optional) When deep_compare is enabled and puppet updates a resource, this
+#   parameter represents the number (in seconds) to wait for the cluster to settle
+#   after the resource update.
+#   Defaults to hiera('pacemaker::resource::systemd::update_settle_secs', 600) (seconds)
+#
 # === Dependencies
 #
 #  None
@@ -116,6 +122,7 @@ define pacemaker::resource::systemd(
   $verify_on_create   = false,
   $location_rule      = undef,
   $deep_compare       = false,
+  $update_settle_secs = hiera('pacemaker::resource::systemd::update_settle_secs', 600),
 ) {
   pcmk_resource { $name:
     ensure             => $ensure,
@@ -132,5 +139,6 @@ define pacemaker::resource::systemd(
     verify_on_create   => $verify_on_create,
     location_rule      => $location_rule,
     deep_compare       => $deep_compare,
+    update_settle_secs => $update_settle_secs,
   }
 }

spec/unit/puppet/provider/pcmk_common_spec.rb

@@ -10,6 +10,9 @@ describe "pcmk_common functions" do
     FileUtils.cp orig_cib, "cib-noop.xml"
     FileUtils.cp orig_cib, "cib-resource.xml"
     FileUtils.cp orig_cib, "cib-bundle.xml"
+    FileUtils.cp orig_cib, "cib-noop.xml.orig"
+    FileUtils.cp orig_cib, "cib-resource.xml.orig"
+    FileUtils.cp orig_cib, "cib-bundle.xml.orig"
   end
 
   it "pcmk_graph_contain_id? raises proper exception" do
@@ -23,7 +26,6 @@ describe "pcmk_common functions" do
     expect(pcmk_restart_resource?('foo', "cib-noop.xml")).to eq false
     expect(pcmk_restart_resource?('ip-172.16.11.97', "cib-noop.xml")).to eq false
   end
-
   it "pcs_offline update to resource definition" do
     expect(pcs_offline('resource update ip-172.16.11.97 cidr_netmask=31', 'cib-resource.xml')).to eq ""
   end
@@ -56,4 +58,19 @@ describe "pcmk_common functions" do
     expect(pcmk_restart_resource?('foo', "cib-bundle.xml", true)).to eq false
     expect(pcmk_restart_resource?('test_bundle', "cib-bundle.xml", true)).to eq true
   end
+
+  context 'when crm_diff is not buggy', if: is_crm_diff_buggy?() == false do
+    it "pcmk_restart_resource_ng? noop" do
+      expect(pcmk_restart_resource_ng?('foo', "cib-noop.xml")).to eq false
+      expect(pcmk_restart_resource_ng?('ip-172.16.11.97', "cib-noop.xml")).to eq false
+    end
+    it "pcmk_restart_resource_ng? vip resource" do
+      expect(pcmk_restart_resource_ng?('foo', "cib-resource.xml")).to eq false
+      expect(pcmk_restart_resource_ng?('ip-172.16.11.97', "cib-resource.xml")).to eq true
+    end
+    it "pcmk_restart_resource_ng? bundle resource" do
+      expect(pcmk_restart_resource_ng?('foo', "cib-bundle.xml")).to eq false
+      expect(pcmk_restart_resource_ng?('test_bundle', "cib-bundle.xml")).to eq true
+    end
+  end
 end