Make pacemaker_resource serializable

Change the internal storage of resource operations from a Set to an
Array of Hashes. This prevents an error when a Puppet Agent tries to
serialize the data to its transaction store and fails to re-read it
with:

Error: Transaction store file
/opt/puppetlabs/puppet/cache/state/transactionstore.yaml is corrupt
((/opt/puppetlabs/puppet/cache/state/transactionstore.yaml): Tried to
load unspecified class: Set); replacing

To support the change, the handling of operations is generally stricter,
preferring to raise an error on unexpected input instead of silently
dropping invalid entries or attempting to convert them.

Change-Id: I4bee307c81d606981574cd0004150f6f3f31f81e

lib/pacemaker/type.rb

@@ -1,5 +1,3 @@
-require 'set'
-
 module Pacemaker
   # contains functions that can be included to the pacemaker types
   module Type
@@ -36,36 +34,44 @@ module Pacemaker
           stringify_data element
         end
       elsif data.is_a? Set
-        new_data = Set.new
+        raise "unexpected Set data: #{data}"
-        data.each do |element|
-          new_data.add(stringify_data element)
-        end
-        new_data
       else
         data.to_s
       end
     end
 
-    # modify provided operations data
+    # Maintains an array of operation hashes as if it was a sorted set. These
-    # @param [Hash,Array] operations_input parameter value from catalog
+    # are in Array-of-Hash format ({ 'name' => 'monitor', 'interval' => ...}),
-    def munge_operations(operations_input)
+    # not { 'monitor' => {...} } ones. The unicity is done on the name and
-      operations_input = [operations_input] unless operations_input.is_a? Array
+    # interval operation keys. The input is expected to have been stringified
-      operations = Set.new
+    # and munged.
-      operations_input.each do |operation|
+    #
-        # operations are an array of sets
+    # Modifies the operations argument and returns it.
-        if operation.is_a? Set
+    #
-          operations.merge operation
+    # We can't use a real Set as it doesn't serialize correctly in Puppet's
-          next
+    # transaction store. This datastructure is always small, so performance
+    # is irrelevant.
+    def add_to_operations_array(operations, new_op)
+      operations.delete_if { |op| op['name'] == new_op['name'] && op['interval'] == new_op['interval'] }
+      operations << new_op
+      operations.sort_by! { |op| "#{op['name']} #{op['interval']}" }
     end
-        # # operations were provided as an array of hashes
+
+    # Munges the input into an Array of munged operations.
+    # @param [Hash,Array] operations_input parameter value from catalog
+    def munge_operations_array(operations_input)
+      operations_input = stringify_data(operations_input)
+      operations_input = [operations_input] unless operations_input.is_a? Array
+      operations = []
+      operations_input.each do |operation|
+        # operations were provided as an array of hashes
         if operation.is_a? Hash and operation['name']
           munge_operation operation
-          operations.add operation
+          add_to_operations_array(operations, operation)
-          next
+        elsif operation.is_a? Hash
-        end
           # operations were provided as a hash of hashes
           operation.each do |operation_name, operation_data|
-          next unless operation_data.is_a? Hash
+            raise "invalid operation in a hash of hashes: #{operation_data}" unless operation_data.is_a? Hash
             operation = {}
             if operation_name.include? ':'
               operation_name_array = operation_name.split(':')
@@ -77,7 +83,10 @@ module Pacemaker
             operation['name'] = operation_name
             operation.merge! operation_data
             munge_operation operation
-          operations.add operation if operation.any?
+            add_to_operations_array(operations, operation) if operation.any?
+          end
+        else
+          raise "invalid pacemaker_resource.operations input: #{operations_input}"
         end
       end
       operations
@@ -86,7 +95,7 @@ module Pacemaker
     # munge a single operations hash
     # @param [Hash] operation
     def munge_operation(operation)
-      return unless operation.is_a? Hash
+      raise "invalid pacemaker_resource.operations element: #{operation}" unless operation.is_a? Hash
       operation['name'] = 'monitor' unless operation['name']
       operation['interval'] = '0' unless operation['name'] == 'monitor'
       operation['interval'] = '0' unless operation['interval']

lib/pacemaker/xml/primitives.rb

@@ -1,5 +1,3 @@
-require 'set'
-
 module Pacemaker
   # function related to the primitives configuration
   # main structure "primitives"

lib/puppet/provider/pacemaker_resource/pacemaker_xml.rb

@@ -1,5 +1,4 @@
 require_relative '../pacemaker_xml'
-require 'set'
 
 Puppet::Type.type(:pacemaker_resource).provide(:xml, parent: Puppet::Provider::PacemakerXML) do
   desc <<-eof
@@ -83,12 +82,13 @@ better model since these values can be almost anything.'
     end
 
     if data['operations']
-      operations_set = Set.new
+      operations_data = []
       data['operations'].each do |_id, operation|
         operation.delete 'id'
-        operations_set.add operation
+        operation = munge_operation(operation)
+        add_to_operations_array(operations_data, operation)
       end
-      target_structure[:operations] = operations_set
+      target_structure[:operations] = operations_data
     end
   end
 
@@ -232,7 +232,6 @@ better model since these values can be almost anything.'
   end
 
   def operations=(should)
-    should = should.first if should.is_a? Array
     property_hash[:operations] = should
   end
 
@@ -305,14 +304,10 @@ better model since these values can be almost anything.'
 
     # operations structure
     if operations && operations.any?
+      raise "expected operations to be an array" unless operations.is_a? Array
       primitive_structure['operations'] = {}
       operations.each do |operation|
-        if operation.is_a?(Array) && operation.length == 2
+        raise "expected operations members to be hashes" unless operation.is_a? Hash
-          # operations were provided and Hash { name => { parameters } }, convert it
-          operation_name = operation[0]
-          operation = operation[1]
-          operation['name'] = operation_name unless operation['name']
-        end
         unless operation['id']
           # there is no id provided, generate it
           id_components = [name, operation['name'], operation['interval']]

lib/puppet/type/pacemaker_resource.rb

@@ -127,17 +127,22 @@ is valid.
     eof
 
     validate do |value|
-      raise "Operations property must be an Hash or a Set. Got: #{value.inspect}" unless value.is_a? Hash or value.is_a? Set
+      raise "Operations property must be an Hash. Got: #{value.inspect}" unless value.is_a? Hash
     end
 
+    # Puppet calls this for individual operations inside the Array
     munge do |value|
+      raise "expected to munge a single operation" if value.is_a? Array
       value = resource.stringify_data value
-      resource.munge_operations(value)
+      resource.munge_operation(value)
     end
 
     def should=(value)
-      super
+      munged = resource.munge_operations_array(value)
-      @should = [resource.munge_operations(@should)]
+      super(munged)
+      # @shouldorig is supposed to hold the original value, but super will
+      # stored munged, not the original it didn't receive.
+      @shouldorig = value
     end
 
     def is_to_s(is)

spec/unit/puppet/provider/pacemaker_resource/xml_spec.rb

@@ -1,5 +1,4 @@
 require 'spec_helper'
-require 'set'
 
 describe Puppet::Type.type(:pacemaker_resource).provider(:xml) do
   let(:resource) do
@@ -196,13 +195,11 @@ describe Puppet::Type.type(:pacemaker_resource).provider(:xml) do
           metadata: {
               'resource-stickiness' => '1'
           },
-          operations: Set.new(
+          operations: [
-              [
               {'interval' => '20', 'name' => 'monitor', 'timeout' => '10'},
               {'interval' => '0', 'name' => 'start', 'timeout' => '60'},
               {'interval' => '0', 'name' => 'stop', 'timeout' => '60'},
-              ]
+          ],
-          )
       }
       expect(provider.property_hash).to eq data
     end
@@ -232,8 +229,7 @@ describe Puppet::Type.type(:pacemaker_resource).provider(:xml) do
               'migration-threshold' => 'INFINITY',
               'failure-timeout' => '60s'
           },
-          operations: Set.new(
+          operations: [
-              [
               {'name' => 'demote', 'timeout' => '60', 'interval' => '0'},
               {'name' => 'monitor', 'timeout' => '60', 'interval' => '27', 'role' => 'Master'},
               {'name' => 'monitor', 'timeout' => '60', 'interval' => '30'},
@@ -241,8 +237,7 @@ describe Puppet::Type.type(:pacemaker_resource).provider(:xml) do
               {'name' => 'promote', 'timeout' => '120', 'interval' => '0'},
               {'name' => 'start', 'timeout' => '120', 'interval' => '0'},
               {'name' => 'stop', 'timeout' => '60', 'interval' => '0'},
-              ]
+          ],
-          ),
       }
       expect(provider.property_hash).to eq data
     end

spec/unit/puppet/type/pacemaker_resource_spec.rb

@@ -1,5 +1,4 @@
 require 'spec_helper'
-require 'set'
 
 describe Puppet::Type.type(:pacemaker_resource) do
   subject do
@@ -82,46 +81,62 @@ describe Puppet::Type.type(:pacemaker_resource) do
     end
 
     context 'on operations' do
+      it 'should maintain a set-like operations array' do
+        ops = []
+        instance.add_to_operations_array(ops, { 'name' => 'foo', 'interval' => 10, })
+        instance.add_to_operations_array(ops, { 'name' => 'foo', 'interval' => 20, 'timeout' => 4, })
+        instance.add_to_operations_array(ops, { 'name' => 'foo', 'interval' => 20, })
+        instance.add_to_operations_array(ops, { 'name' => 'bar', 'interval' => 20, })
+
+        ops2 = [
+          { 'name' => 'bar', 'interval' => 20, },
+          { 'name' => 'foo', 'interval' => 10, },
+          { 'name' => 'foo', 'interval' => 20, },
+        ]
+
+        expect(ops).to eq ops2
+      end
+
       it 'should change operations format if provided as hash' do
         data_from = {'start' => {'timeout' => '20', 'interval' => '0'}, 'monitor' => {'interval' => '10'}}
         data_to = [{'interval' => '10', 'name' => 'monitor'}, {'timeout' => '20', 'name' => 'start', 'interval' => '0'}]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
 
       it 'should support several monitor operations' do
         data_from = [{'interval' => '10', 'name' => 'monitor'}, {'interval' => '20', 'name' => 'monitor'}]
         data_to = [{'interval' => '10', 'name' => 'monitor'}, {'interval' => '20', 'name' => 'monitor'}]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
 
       it 'should reset non-monitor operation interval to 0' do
         data_from = {'start' => {'timeout' => '20', 'interval' => '10'}, 'stop' => {'interval' => '20', 'timeout' => '20'}}
         data_to = [{'timeout' => '20', 'name' => 'start', 'interval' => '0'}, {'interval' => '0', 'name' => 'stop', 'timeout' => '20', }]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
 
       it 'should add missing interval values' do
         data_from = [{'interval' => '10', 'name' => 'monitor'}, {'timeout' => '20', 'name' => 'start'}]
         data_to = [{'interval' => '10', 'name' => 'monitor'}, {'timeout' => '20', 'name' => 'start', 'interval' => '0'}]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
 
       it 'should capitalize role value' do
         data_from = [{'interval' => '10', 'name' => 'monitor', 'role' => 'master'}]
         data_to = [{'interval' => '10', 'name' => 'monitor', 'role' => 'Master'}]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
 
       it 'should stringify operations structure' do
         data_from = {'interval' => 10, :name => 'monitor'}
         data_to = [{'interval' => '10', 'name' => 'monitor'}]
         instance[:operations] = data_from
-        expect(instance[:operations]).to eq [Set.new(data_to)]
+        expect(instance[:operations]).to eq data_to
       end
     end