Improve cs_clone

- Stop cs_clone before removing (crm provider) - Use resource name as clone id (pcs provider) - closes voxpupuli#149 - Add acceptance tests and improve unit tests Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
roidelapluie · Sep 15, 2016 · 1cc7cee · 1cc7cee
1 parent 3f56c39
commit 1cc7cee
Show file tree

Hide file tree

Showing 8 changed files with 201 additions and 138 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,6 +21,8 @@
 - Removed legacy configuration sections: amf, aisexec, logging.logger\_subsys
   (#345)
 - Fix two\_nodes behaviour with expected\_votes = 2 introduced in 3.0.0 (#246)
+- Cs\_clones are now stopped before being removed (crm provider) (#367)
+- Cs\_clones now use the resource name as clone id (pcs provider) (#149, #367)
 
 ### Deprecation notes
 

diff --git a/lib/puppet/provider/cs_clone/crm.rb b/lib/puppet/provider/cs_clone/crm.rb
@@ -15,6 +15,8 @@
   commands crm: 'crm'
   commands crm_attribute: 'crm_attribute'
 
+  mk_resource_methods
+
   def self.instances
     block_until_ready
 
@@ -66,76 +68,13 @@ def create
   # Unlike create we actually immediately delete the item.
   def destroy
     debug('Removing clone')
+    cmd = [command(:crm), '-w', 'resource', 'stop', @resource[:name]]
+    PuppetX::Voxpupuli::Corosync::Provider::Crmsh.run_command_in_cib(cmd, @resource[:cib], false)
     cmd = [command(:crm), 'configure', 'delete', @resource[:name]]
     PuppetX::Voxpupuli::Corosync::Provider::Crmsh.run_command_in_cib(cmd, @resource[:cib])
     @property_hash.clear
   end
 
-  #
-  # Getter that obtains the our service that should have been populated by
-  # prefetch or instances (depends on if your using puppet resource or not).
-  def primitive
-    @property_hash[:primitive]
-  end
-
-  # Getter that obtains the our clone_max that should have been populated by
-  # prefetch or instances (depends on if your using puppet resource or not).
-  def clone_max
-    @property_hash[:clone_max]
-  end
-
-  def clone_node_max
-    @property_hash[:clone_node_max]
-  end
-
-  def notify_clones
-    @property_hash[:notify_clones]
-  end
-
-  def globally_unique
-    @property_hash[:globally_unique]
-  end
-
-  def ordered
-    @property_hash[:ordered]
-  end
-
-  def interleave
-    @property_hash[:interleave]
-  end
-
-  # Our setters.  Setters are used when the
-  # resource already exists so we just update the current value in the property
-  # hash and doing this marks it to be flushed.
-
-  def primitive=(should)
-    @property_hash[:primitive] = should
-  end
-
-  def clone_max=(should)
-    @property_hash[:clone_max] = should
-  end
-
-  def clone_node_max=(should)
-    @property_hash[:clone_node_max] = should
-  end
-
-  def notify_clones=(should)
-    @property_hash[:notify_clones] = should
-  end
-
-  def globally_unique=(should)
-    @property_hash[:globally_unique] = should
-  end
-
-  def ordered=(should)
-    @property_hash[:ordered] = should
-  end
-
-  def interleave=(should)
-    @property_hash[:interleave] = should
-  end
-
   # Flush is triggered on anything that has been detected as being
   # modified in the property_hash.  It generates a temporary file with
   # the updates that need to be made.  The temporary file is then used

diff --git a/lib/puppet/provider/cs_clone/pcs.rb b/lib/puppet/provider/cs_clone/pcs.rb
@@ -11,9 +11,24 @@
   desc 'Provider to add, delete, manipulate primitive clones.'
 
   commands pcs: 'pcs'
+  commands cibadmin: 'cibadmin'
+
+  mk_resource_methods
 
   defaultfor operatingsystem: [:fedora, :centos, :redhat]
 
+  def change_clone_id(primitive, id, cib)
+    xpath = "/cib/configuration/resources/clone[descendant::primitive[@id='#{primitive}']]"
+    cmd = [command(:cibadmin), '--query', '--xpath', xpath]
+    raw, = PuppetX::Voxpupuli::Corosync::Provider::Pcs.run_command_in_cib(cmd, cib)
+    doc = REXML::Document.new(raw)
+    if doc.root.attributes['id'] != id
+      doc.root.attributes['id'] = id
+      cmd = [command(:cibadmin), '--replace', '--xpath', xpath, '--xml-text', doc.to_s.chop]
+      PuppetX::Voxpupuli::Corosync::Provider::Pcs.run_command_in_cib(cmd, cib)
+    end
+  end
+
   def self.instances
     block_until_ready
 
@@ -48,7 +63,7 @@ def self.instances
   # of actually doing the work.
   def create
     @property_hash = {
-      name:              @resource[:primitive] + '-clone',
+      name:              @resource[:name],
       ensure:            :present,
       primitive:         @resource[:primitive],
       clone_max:         @resource[:clone_max],
@@ -68,71 +83,6 @@ def destroy
     @property_hash.clear
   end
 
-  #
-  # Getter that obtains the our service that should have been populated by
-  # prefetch or instances (depends on if your using puppet resource or not).
-  def primitive
-    @property_hash[:primitive]
-  end
-
-  # Getter that obtains the our clone_max that should have been populated by
-  # prefetch or instances (depends on if your using puppet resource or not).
-  def clone_max
-    @property_hash[:clone_max]
-  end
-
-  def clone_node_max
-    @property_hash[:clone_node_max]
-  end
-
-  def notify_clones
-    @property_hash[:notify_clones]
-  end
-
-  def globally_unique
-    @property_hash[:globally_unique]
-  end
-
-  def ordered
-    @property_hash[:ordered]
-  end
-
-  def interleave
-    @property_hash[:interleave]
-  end
-
-  # Our setters.  Setters are used when the
-  # resource already exists so we just update the current value in the property
-  # hash and doing this marks it to be flushed.
-
-  def primitive=(should)
-    @property_hash[:primitive] = should
-  end
-
-  def clone_max=(should)
-    @property_hash[:clone_max] = should
-  end
-
-  def clone_node_max=(should)
-    @property_hash[:clone_node_max] = should
-  end
-
-  def notify_clones=(should)
-    @property_hash[:notify_clones] = should
-  end
-
-  def globally_unique=(should)
-    @property_hash[:globally_unique] = should
-  end
-
-  def ordered=(should)
-    @property_hash[:ordered] = should
-  end
-
-  def interleave=(should)
-    @property_hash[:interleave] = should
-  end
-
   def exists?
     @property_hash[:existing_resource] == :true
   end
@@ -160,6 +110,7 @@ def flush
       cmd << "ordered=#{@property_hash[:ordered]}" if @property_hash[:ordered]
       cmd << "interleave=#{@property_hash[:interleave]}" if @property_hash[:interleave]
       PuppetX::Voxpupuli::Corosync::Provider::Pcs.run_command_in_cib(cmd, @resource[:cib])
+      change_clone_id(@property_hash[:primitive], @property_hash[:name], @resource[:cib])
     end
   end
 end
diff --git a/lib/puppet/type/cs_clone.rb b/lib/puppet/type/cs_clone.rb
@@ -7,9 +7,12 @@
   ensurable
 
   newparam(:name) do
-    desc "Identifier of the location entry. This value needs to be unique
+    desc "Identifier of the clone entry. This value needs to be unique
       across the entire Corosync/Pacemaker configuration since it doesn't have
-      the concept of name spaces per type."
+      the concept of name spaces per type.
+
+      For provider that do not support arbitrary names for clones, this property is
+      the name of the primitive that will be cloned."
 
     isnamevar
   end
@@ -90,4 +93,9 @@ def unmunge_cs_primitive(name)
 
     name
   end
+
+  validate do
+    return if self[:ensure] == :absent
+    raise Puppet::Error, 'primitive is mandatory' unless self[:primitive]
+  end
 end
diff --git a/spec/acceptance/cs_clone_spec.rb b/spec/acceptance/cs_clone_spec.rb
@@ -0,0 +1,90 @@
+#! /usr/bin/env ruby -S rspec
+require 'spec_helper_acceptance'
+
+describe 'corosync' do
+  cert = '-----BEGIN CERTIFICATE-----
+MIIDVzCCAj+gAwIBAgIJAJNCo5ZPmKegMA0GCSqGSIb3DQEBBQUAMEIxCzAJBgNV
+BAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQg
+Q29tcGFueSBMdGQwHhcNMTUwMjI2MjI1MjU5WhcNMTUwMzI4MjI1MjU5WjBCMQsw
+CQYDVQQGEwJYWDEVMBMGA1UEBwwMRGVmYXVsdCBDaXR5MRwwGgYDVQQKDBNEZWZh
+dWx0IENvbXBhbnkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA
+uCPPbDgErGUVs1pKqv59OatjCEU4P9QcmhDYFR7RBN8m08mIqd+RTuiHUKj6C9Rk
+vWQ5bYrGQo/+4E0ziAUuUzzITlpIYLVltca6eBhKUqO3Cd0NMRVc2k4nx5948nwv
+9FVOIfOOY6BN2ALglfBfLnhObbzJjs6OSZ7bUCpXVPV01t/61Jj3jQ3+R8b7AaoR
+mw7j0uWaFimKt/uag1qqKGw3ilieMhHlG0Da5x9WLi+5VIM0t1rcpR58LLXVvXZB
+CrQBucm2xhZsz7R76Ai+NL8zhhyzCZidZ2NtJ3E1wzppcSDAfNrru+rcFSlZ4YG+
+lMCqZ1aqKWVXmb8+Vg7IkQIDAQABo1AwTjAdBgNVHQ4EFgQULxI68KhZwEF5Q9al
+xZmFDR+Beu4wHwYDVR0jBBgwFoAULxI68KhZwEF5Q9alxZmFDR+Beu4wDAYDVR0T
+BAUwAwEB/zANBgkqhkiG9w0BAQUFAAOCAQEAsa0YKPixD6VmDo3pal2qqichHbdT
+hUONk2ozzRoaibVocqKx2T6Ho23wb/lDlRUu4K4DMO663uumzI9lNoOewa0MuW1D
+J52cejAMVsP3ROOdxBv0HZIVVJ8NLBHNLFOHJEDtvzogLVplzmo59vPAdmQo6eIV
+japvs+0tdy9iwHj3z1ZME2Ntm/5TzG537e7Hb2zogatM9aBTUAWlZ1tpoaXuTH52
+J76GtqoIOh+CTeY/BMwBotdQdgeR0zvjE9FuLWkhTmRtVFhbVIzJbFlFuYq5d3LH
+NWyN0RsTXFaqowV1/HSyvfD7LoF/CrmN5gOAM3Ierv/Ti9uqGVhdGBd/kw=='
+  File.open('/tmp/ca.pem', 'w') { |f| f.write(cert) }
+  it 'with defaults' do
+    pp = <<-EOS
+      file { '/tmp/ca.pem':
+        ensure  => file,
+        content => '#{cert}'
+      } ->
+      class { 'corosync':
+        multicast_address => '224.0.0.1',
+        authkey           => '/tmp/ca.pem',
+        bind_address      => '127.0.0.1',
+        set_votequorum    => true,
+        quorum_members    => ['127.0.0.1'],
+      }
+      cs_property { 'stonith-enabled' :
+        value   => 'false',
+      } ->
+      cs_primitive { 'duncan_vip':
+        primitive_class => 'ocf',
+        primitive_type  => 'IPaddr2',
+        provided_by     => 'heartbeat',
+        parameters      => { 'ip' => '172.16.210.101', 'cidr_netmask' => '24' },
+        operations      => { 'monitor' => { 'interval' => '10s' } },
+      }
+    EOS
+
+    apply_manifest(pp, catch_failures: true, debug: true, trace: true)
+    apply_manifest(pp, catch_changes: true, debug: true, trace: true)
+  end
+
+  describe service('corosync') do
+    it { is_expected.to be_running }
+  end
+
+  it 'creates a clone' do
+    pp = <<-EOS
+         cs_clone { 'duncan_vip_clone':
+           ensure => present,
+           primitive => 'duncan_vip',
+         }
+         EOS
+    apply_manifest(pp, catch_failures: true, debug: true, trace: true)
+    apply_manifest(pp, catch_changes: true, debug: true, trace: true)
+    command = 'cibadmin --query | grep duncan_vip_clone'
+    shell(command) do |r|
+      expect(r.stdout).to match(%r{<clone})
+    end
+  end
+
+  it 'deletes a clone' do
+    pp = <<-EOS
+         cs_clone { 'duncan_vip_clone':
+           ensure => absent,
+         }
+         EOS
+    apply_manifest(pp, catch_failures: true, debug: true, trace: true)
+    apply_manifest(pp, catch_changes: true, debug: true, trace: true)
+    command = 'cibadmin --query | grep duncan_vip_clone'
+    assert_raises(Beaker::Host::CommandFailure) do
+      shell(command)
+    end
+  end
+
+  after :all do
+    cleanup_cs_resources
+  end
+end
diff --git a/spec/spec_helper_corosync.rb b/spec/spec_helper_corosync.rb
@@ -24,6 +24,7 @@ def not_expect_commands(patterns)
 shared_context 'pcs' do
   before do
     described_class.stubs(:command).with(:pcs).returns 'pcs'
+    described_class.stubs(:command).with(:cibadmin).returns 'cibadmin'
     described_class.expects(:block_until_ready).returns(nil).at_most(1)
   end
 end