From dcc14c8537efb5e75b201cbe5048d9d42c0021d1 Mon Sep 17 00:00:00 2001 From: Thomas Equeter Date: Tue, 10 Mar 2020 11:28:12 +0100 Subject: [PATCH] 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 | 75 +++++++++++-------- lib/pacemaker/xml/primitives.rb | 2 - .../pacemaker_resource/pacemaker_xml.rb | 17 ++--- lib/puppet/type/pacemaker_resource.rb | 13 +++- .../provider/pacemaker_resource/xml_spec.rb | 33 ++++---- .../puppet/type/pacemaker_resource_spec.rb | 29 +++++-- 6 files changed, 93 insertions(+), 76 deletions(-) diff --git a/lib/pacemaker/type.rb b/lib/pacemaker/type.rb index 275b43ab..b08c4e95 100644 --- a/lib/pacemaker/type.rb +++ b/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,48 +34,59 @@ module Pacemaker stringify_data element end elsif data.is_a? Set - new_data = Set.new - data.each do |element| - new_data.add(stringify_data element) - end - new_data + raise "unexpected Set data: #{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 + # are in Array-of-Hash format ({ 'name' => 'monitor', 'interval' => ...}), + # not { 'monitor' => {...} } ones. The unicity is done on the name and + # interval operation keys. The input is expected to have been stringified + # and munged. + # + # Modifies the operations argument and returns it. + # + # We can't use a real Set as it doesn't serialize correctly in Puppet's + # 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 + + # Munges the input into an Array of munged operations. # @param [Hash,Array] operations_input parameter value from catalog - def munge_operations(operations_input) + def munge_operations_array(operations_input) + operations_input = stringify_data(operations_input) operations_input = [operations_input] unless operations_input.is_a? Array - operations = Set.new + operations = [] operations_input.each do |operation| - # operations are an array of sets - if operation.is_a? Set - operations.merge operation - next - end - # # operations were provided as an array of hashes + # operations were provided as an array of hashes if operation.is_a? Hash and operation['name'] munge_operation operation - operations.add operation - next - end - # operations were provided as a hash of hashes - operation.each do |operation_name, operation_data| - next unless operation_data.is_a? Hash - operation = {} - if operation_name.include? ':' - operation_name_array = operation_name.split(':') - operation_name = operation_name_array[0] - if not operation_data['role'] and operation_name_array[1] - operation_data['role'] = operation_name_array[1] + add_to_operations_array(operations, operation) + elsif operation.is_a? Hash + # operations were provided as a hash of hashes + operation.each do |operation_name, operation_data| + 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(':') + operation_name = operation_name_array[0] + if not operation_data['role'] and operation_name_array[1] + operation_data['role'] = operation_name_array[1] + end end + operation['name'] = operation_name + operation.merge! operation_data + munge_operation operation + add_to_operations_array(operations, operation) if operation.any? end - operation['name'] = operation_name - operation.merge! operation_data - munge_operation operation - operations.add operation if operation.any? + 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'] diff --git a/lib/pacemaker/xml/primitives.rb b/lib/pacemaker/xml/primitives.rb index 746da607..697d40fe 100644 --- a/lib/pacemaker/xml/primitives.rb +++ b/lib/pacemaker/xml/primitives.rb @@ -1,5 +1,3 @@ -require 'set' - module Pacemaker # function related to the primitives configuration # main structure "primitives" diff --git a/lib/puppet/provider/pacemaker_resource/pacemaker_xml.rb b/lib/puppet/provider/pacemaker_resource/pacemaker_xml.rb index 8513cfa1..ceb555e8 100644 --- a/lib/puppet/provider/pacemaker_resource/pacemaker_xml.rb +++ b/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 - # operations were provided and Hash { name => { parameters } }, convert it - operation_name = operation[0] - operation = operation[1] - operation['name'] = operation_name unless operation['name'] - end + raise "expected operations members to be hashes" unless operation.is_a? Hash unless operation['id'] # there is no id provided, generate it id_components = [name, operation['name'], operation['interval']] diff --git a/lib/puppet/type/pacemaker_resource.rb b/lib/puppet/type/pacemaker_resource.rb index 6dbc7bf3..7385bb2b 100644 --- a/lib/puppet/type/pacemaker_resource.rb +++ b/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 - @should = [resource.munge_operations(@should)] + munged = resource.munge_operations_array(value) + 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) diff --git a/spec/unit/puppet/provider/pacemaker_resource/xml_spec.rb b/spec/unit/puppet/provider/pacemaker_resource/xml_spec.rb index 4420a6d7..facab4ee 100644 --- a/spec/unit/puppet/provider/pacemaker_resource/xml_spec.rb +++ b/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( - [ - {'interval' => '20', 'name' => 'monitor', 'timeout' => '10'}, - {'interval' => '0', 'name' => 'start', 'timeout' => '60'}, - {'interval' => '0', 'name' => 'stop', 'timeout' => '60'}, - ] - ) + 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,17 +229,15 @@ describe Puppet::Type.type(:pacemaker_resource).provider(:xml) do 'migration-threshold' => 'INFINITY', 'failure-timeout' => '60s' }, - operations: Set.new( - [ - {'name' => 'demote', 'timeout' => '60', 'interval' => '0'}, - {'name' => 'monitor', 'timeout' => '60', 'interval' => '27', 'role' => 'Master'}, - {'name' => 'monitor', 'timeout' => '60', 'interval' => '30'}, - {'name' => 'notify', 'timeout' => '60', 'interval' => '0'}, - {'name' => 'promote', 'timeout' => '120', 'interval' => '0'}, - {'name' => 'start', 'timeout' => '120', 'interval' => '0'}, - {'name' => 'stop', 'timeout' => '60', 'interval' => '0'}, - ] - ), + operations: [ + {'name' => 'demote', 'timeout' => '60', 'interval' => '0'}, + {'name' => 'monitor', 'timeout' => '60', 'interval' => '27', 'role' => 'Master'}, + {'name' => 'monitor', 'timeout' => '60', 'interval' => '30'}, + {'name' => 'notify', 'timeout' => '60', 'interval' => '0'}, + {'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 diff --git a/spec/unit/puppet/type/pacemaker_resource_spec.rb b/spec/unit/puppet/type/pacemaker_resource_spec.rb index 46608b1f..8e7f4370 100644 --- a/spec/unit/puppet/type/pacemaker_resource_spec.rb +++ b/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