From 78337c2270eb62962640da307f4e13c169d92d26 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Wed, 30 Mar 2016 16:25:59 +0100 Subject: [PATCH] fixes #14447 - add cache around parser using YAML file A cache of parsed modules can be configured with `:parser_cache_path` to skip calls out to kafo_parsers when loading Puppet modules. This helps when no parser is available for the current Puppet installation (e.g. under Puppet 4 with AIO at the time of writing), and may provide a small peformance benefit. The cache can be generated with `kafo-export-params -f parsercache --no-parser-cache`. The cache is skipped if the mtime of the manifest is greater than the mtime when the cache was generated, causing Kafo to use kafo_parsers as normal. --- README.md | 16 ++++++++ bin/kafo-export-params | 56 ++++++++++++++++++-------- config/kafo.yaml.example | 2 + lib/kafo/configuration.rb | 9 ++++- lib/kafo/param_builder.rb | 2 +- lib/kafo/parser_cache_reader.rb | 46 +++++++++++++++++++++ lib/kafo/parser_cache_writer.rb | 14 +++++++ lib/kafo/puppet_module.rb | 11 +++-- test/kafo/parser_cache_reader_test.rb | 58 +++++++++++++++++++++++++++ test/kafo/parser_cache_writer_test.rb | 32 +++++++++++++++ test/kafo/puppet_module_test.rb | 25 ++++++++++-- test/parser_cache_factory.rb | 20 +++++++++ test/test_helper.rb | 1 + test/test_parser.rb | 8 ++-- 14 files changed, 269 insertions(+), 31 deletions(-) create mode 100644 lib/kafo/parser_cache_reader.rb create mode 100644 lib/kafo/parser_cache_writer.rb create mode 100644 test/kafo/parser_cache_reader_test.rb create mode 100644 test/kafo/parser_cache_writer_test.rb create mode 100644 test/parser_cache_factory.rb diff --git a/README.md b/README.md index e99777e0..71cf8e19 100644 --- a/README.md +++ b/README.md @@ -908,6 +908,22 @@ parameter `--skip-checks-i-know-better` (or `-s`). This will completely disable running all system check scripts. Note that this option is not persisted between runs. +## Parser cache + +A cache of parsed Puppet modules and manifests can be created to skip the use +of kafo_parsers at runtime. This is useful when kafo_parsers doesn't support the +version of Puppet in use, and may also provide a small performance benefit. + +Create the cache with `kafo-export-params -f parsercache --no-parser-cache` and +configure it in config/kafo.yaml with: + +```yaml +:parser_cache_path: ./parser_cache.yaml +``` + +The cache will be skipped if the file modification time of the manifest is +greater than the mtime recorded in the cache. + ## Exit code Kafo can terminate either before or after puppet is run. If it is run with diff --git a/bin/kafo-export-params b/bin/kafo-export-params index 6b8a4401..35655e07 100755 --- a/bin/kafo-export-params +++ b/bin/kafo-export-params @@ -5,8 +5,10 @@ require 'clamp' require 'logging' require 'kafo/configuration' require 'kafo/exceptions' +require 'kafo/parser_cache_writer' require 'kafo/string_helper' require 'logger' +require 'yaml' KafoConfigure = OpenStruct.new def KafoConfigure.exit(code) @@ -15,36 +17,54 @@ end module Kafo class KafoExportParams < Clamp::Command - TYPES = %w(md html asciidoc) + TYPES = %w(md html asciidoc parsercache) option ['-c', '--config'], 'FILE', 'Config file for which should we generate params', :required => true option ['-f', '--format'], 'FORMAT', - 'Config file for which should we generate params', :default => 'md' do |format| + "Format parameters will be written in, valid options: #{TYPES.join(',')}", :default => 'md' do |format| format = format.downcase raise ArgumentError unless TYPES.include?(format) format end + option ['-o', '--output'], 'FILE', 'Output file to write parameters into', :default => '-' + + option '--[no-]parser-cache', :flag, 'Enable or disable the parser cache, disable for fresh results', :default => true + def execute c = Configuration.new(config, false) + c.app[:parser_cache_path] = nil unless parser_cache? KafoConfigure.config = c KafoConfigure.root_dir = File.expand_path(c.app[:installer_dir]) KafoConfigure.module_dirs = File.expand_path(c.app[:module_dirs]) - KafoConfigure.logger = Logger.new(STDOUT) + KafoConfigure.logger = Logger.new(STDERR) - exporter = self.class.const_get(format.capitalize).new(c) + if output == '-' + file = STDOUT + else + file = File.open(output, 'w') + end + + exporter = self.class.const_get(format.capitalize).new(c, file) exporter.print_out end - class Html - include StringHelper - - def initialize(config) + class Writer + def initialize(config, file) @config = config + @file = file end + def puts(*args) + @file.puts(*args) + end + end + + class Html < Writer + include StringHelper + def print_out puts '
' puts ' ' @@ -77,13 +97,9 @@ module Kafo end end - class Asciidoc + class Asciidoc < Writer include StringHelper - def initialize(config) - @config = config - end - def print_out @config.modules.sort.each do |mod| puts "Parameters for '#{mod.name}':\n\n" @@ -97,12 +113,12 @@ module Kafo end end - class Md + class Md < Writer include StringHelper - def initialize(config) - @config = config - @max = max_description_length + def initialize(*args) + super + @max = max_description_length end def print_out @@ -127,6 +143,12 @@ module Kafo doc_lengths.max end end + + class Parsercache < Writer + def print_out + puts Kafo::ParserCacheWriter.write(@config.modules).to_yaml + end + end end end diff --git a/config/kafo.yaml.example b/config/kafo.yaml.example index 01273742..2bb5b8f5 100644 --- a/config/kafo.yaml.example +++ b/config/kafo.yaml.example @@ -16,6 +16,8 @@ # :module_dirs: /usr/share/kafo/modules # Similar as modules_dir but for kafo internal modules, leave nil if you don't need to change it # :kafo_modules_dir: +# Location of an optional cache of parsed module data, generate with kafo-export-params -f parsercache +# :parser_cache_path: /usr/share/kafo/parser_cache.yaml # Enable colors? If you don't touch this, we'll autodetect terminal capabilities # :colors: true # Color scheme, we support :bright and :dark (first is better for white background, dark for black background) diff --git a/lib/kafo/configuration.rb b/lib/kafo/configuration.rb index d1531537..1dcafa7f 100644 --- a/lib/kafo/configuration.rb +++ b/lib/kafo/configuration.rb @@ -27,7 +27,8 @@ class Configuration :hook_dirs => [], :custom => {}, :low_priority_modules => [], - :verbose_log_level => 'info' + :verbose_log_level => 'info', + :parser_cache_path => './config/parser_cache.json' } def initialize(file, persist = true) @@ -257,6 +258,12 @@ def migrations_dir @config_file.gsub(/\.yaml$/, '.migrations') end + def parser_cache + if app[:parser_cache_path] + @parser_cache ||= Kafo::ParserCacheReader.new_from_file(File.expand_path(app[:parser_cache_path])) + end + end + private def custom_storage diff --git a/lib/kafo/param_builder.rb b/lib/kafo/param_builder.rb index 081390ab..b0720ea4 100644 --- a/lib/kafo/param_builder.rb +++ b/lib/kafo/param_builder.rb @@ -79,7 +79,7 @@ def find_or_build_group(name) end def get_type(type) - type = type.capitalize + type = (type || 'string').capitalize Params.const_defined?(type) ? Params.const_get(type) : raise(TypeError, "undefined parameter type '#{type}'") end end diff --git a/lib/kafo/parser_cache_reader.rb b/lib/kafo/parser_cache_reader.rb new file mode 100644 index 00000000..1c0325d2 --- /dev/null +++ b/lib/kafo/parser_cache_reader.rb @@ -0,0 +1,46 @@ +module Kafo + class ParserCacheReader + def self.new_from_file(cache_path) + if cache_path.nil? || cache_path.empty? + logger.debug "No parser cache configured in :parser_cache_path, skipping setup" + return nil + end + + unless File.exist?(cache_path) + logger.warn "Parser cache configured at #{cache_path} is missing, skipping setup" + return nil + end + + parsed = YAML.load(File.read(cache_path)) + if !parsed.is_a?(Hash) || parsed[:version] != 1 || !parsed[:files].is_a?(Hash) + logger.warn "Parser cache is from a different version of Kafo, skipping setup" + return nil + end + + new(parsed) + end + + def self.logger + KafoConfigure.logger + end + + def initialize(cache) + @cache = cache + end + + def logger + KafoConfigure.logger + end + + def get(key, manifest_path) + return nil unless @cache[:files].has_key?(key) + + if @cache[:files][key][:mtime] && File.mtime(manifest_path).to_i > @cache[:files][key][:mtime] + logger.debug "Parser cache for #{manifest_path} is outdated, ignoring cache entry" + return nil + end + + @cache[:files][key][:data] + end + end +end diff --git a/lib/kafo/parser_cache_writer.rb b/lib/kafo/parser_cache_writer.rb new file mode 100644 index 00000000..dcfddfdc --- /dev/null +++ b/lib/kafo/parser_cache_writer.rb @@ -0,0 +1,14 @@ +module Kafo + class ParserCacheWriter + def self.write(modules) + { + :version => 1, + :files => Hash[modules.sort.map { |m| write_module(m) }] + } + end + + def self.write_module(mod) + [mod.identifier, {:data => mod.raw_data, :mtime => File.mtime(mod.manifest_path).to_i}] + end + end +end diff --git a/lib/kafo/puppet_module.rb b/lib/kafo/puppet_module.rb index 1661ece7..84c54291 100644 --- a/lib/kafo/puppet_module.rb +++ b/lib/kafo/puppet_module.rb @@ -1,6 +1,7 @@ # encoding: UTF-8 require 'kafo/param' require 'kafo/param_builder' +require 'kafo/parser_cache_reader' require 'kafo_parsers/puppet_module_parser' require 'kafo/validator' @@ -9,7 +10,7 @@ class PuppetModule PRIMARY_GROUP_NAME = 'Parameters' attr_reader :name, :identifier, :params, :dir_name, :class_name, :manifest_name, :manifest_path, - :groups, :params_path, :params_class_name, :configuration + :groups, :params_path, :params_class_name, :configuration, :raw_data def initialize(identifier, parser = KafoParsers::PuppetModuleParser, configuration = KafoConfigure.config) @identifier = identifier @@ -27,6 +28,7 @@ def initialize(identifier, parser = KafoParsers::PuppetModuleParser, configurati end @manifest_path = File.join(module_dir, module_manifest_path) @parser = parser + @parser_cache = @configuration.parser_cache @validations = [] @logger = KafoConfigure.logger @groups = {} @@ -48,9 +50,10 @@ def enable def parse(builder_klass = ParamBuilder) @params = [] - raw_data = @parser.parse(manifest_path) - builder = builder_klass.new(self, raw_data) - @validations = raw_data[:validations] + @raw_data = @parser_cache.get(identifier, manifest_path) if @parser_cache + @raw_data ||= @parser.parse(manifest_path) + builder = builder_klass.new(self, @raw_data) + @validations = @raw_data[:validations] builder.validate @params = builder.build_params diff --git a/test/kafo/parser_cache_reader_test.rb b/test/kafo/parser_cache_reader_test.rb new file mode 100644 index 00000000..cf04a3e9 --- /dev/null +++ b/test/kafo/parser_cache_reader_test.rb @@ -0,0 +1,58 @@ +require 'test_helper' +require 'kafo/parser_cache_writer' + +module Kafo + describe ParserCacheReader do + subject { ParserCacheReader } + + describe ".new_from_file" do + let(:valid_cache) do + {:version => 1, :files => {}} + end + + specify { subject.new_from_file(nil).must_be_nil } + specify { subject.new_from_file('').must_be_nil } + specify { subject.new_from_file('/non/existent/file').must_be_nil } + + describe "with empty file" do + let(:cache) { ParserCacheFactory.build('') } + specify { subject.new_from_file(cache.path).must_be_nil } + end + + describe "with version other than 1" do + let(:cache) { ParserCacheFactory.build(valid_cache.update(:version => 2)) } + specify { subject.new_from_file(cache.path).must_be_nil } + end + + describe "with missing files section" do + let(:cache) { ParserCacheFactory.build(valid_cache.reject { |k,v| k == :files }) } + specify { subject.new_from_file(cache.path).must_be_nil } + end + + describe "with correct format" do + let(:cache) { ParserCacheFactory.build(valid_cache) } + specify { subject.new_from_file(cache.path).must_be_instance_of subject } + end + end + + describe "#get" do + specify { subject.new({:files => {}}).get('test', '/test/file.pp').must_be_nil } + specify { subject.new({:files => {'test' => {:data => {:parameters => []}}}}).get('test', '/test/file.pp').must_equal(:parameters => []) } + specify { File.stub(:mtime, 1) { subject.new({:files => {'test' => {:mtime => 1, :data => :test}}}).get('test', '/test/file.pp').must_equal(:test) } } + specify { File.stub(:mtime, 2) { subject.new({:files => {'test' => {:mtime => 1, :data => :test}}}).get('test', '/test/file.pp').must_be_nil } } + end + + describe "compatibility with writer" do + before do + KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path) + end + + let(:parser) { TestParser.new(BASIC_MANIFEST) } + let(:mod) { PuppetModule.new('puppet', parser) } + let(:writer) { ParserCacheWriter.write([mod]) } + let(:cache) { ParserCacheFactory.build(writer) } + + specify { File.stub(:mtime, 1) { subject.new_from_file(cache.path).get('puppet', parser.manifest_file).must_equal mod.raw_data } } + end + end +end diff --git a/test/kafo/parser_cache_writer_test.rb b/test/kafo/parser_cache_writer_test.rb new file mode 100644 index 00000000..0eb28026 --- /dev/null +++ b/test/kafo/parser_cache_writer_test.rb @@ -0,0 +1,32 @@ +require 'test_helper' +require 'kafo/parser_cache_writer' +require 'tempfile' + +module Kafo + describe ParserCacheWriter do + subject { ParserCacheWriter } + + describe ".write" do + specify { subject.write([])[:version].must_equal 1 } + specify { subject.write([])[:files].must_equal({}) } + + describe "with a module" do + let(:manifest) { Tempfile.new("#{subject}_manifest") } + let(:mod) do + mod = MiniTest::Mock.new + mod.expect(:identifier, 'module') + mod.expect(:manifest_path, manifest.path) + mod.expect(:raw_data, {:parameters => [], :groups => []}) + mod + end + let(:output) { subject.write([mod]) } + + specify { output[:files].keys.must_equal ['module'] } + specify { output[:files]['module'].keys.sort.must_equal [:data, :mtime] } + specify { output[:files]['module'][:mtime].must_equal File.mtime(manifest.path).to_i } + specify { output[:files]['module'][:data].has_key?(:parameters).must_equal true } + specify { output[:files]['module'][:data][:parameters].must_equal([]) } + end + end + end +end diff --git a/test/kafo/puppet_module_test.rb b/test/kafo/puppet_module_test.rb index f9231813..28baa865 100644 --- a/test/kafo/puppet_module_test.rb +++ b/test/kafo/puppet_module_test.rb @@ -6,7 +6,8 @@ module Kafo KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path) end - let(:mod) { PuppetModule.new 'puppet', TestParser.new(BASIC_MANIFEST) } + let(:parser) { TestParser.new(BASIC_MANIFEST) } + let(:mod) { PuppetModule.new 'puppet', parser } describe "#enabled?" do specify { mod.enabled?.must_equal true } @@ -23,8 +24,8 @@ module Kafo end # BASIC_CONFIGURATION has mapping configured for this module - let(:plugin1_mod) { PuppetModule.new 'foreman::plugin::default_hostgroup', TestParser.new(BASIC_MANIFEST) } - let(:plugin2_mod) { PuppetModule.new 'foreman::plugin::chef', TestParser.new(BASIC_MANIFEST) } + let(:plugin1_mod) { PuppetModule.new 'foreman::plugin::default_hostgroup', parser } + let(:plugin2_mod) { PuppetModule.new 'foreman::plugin::chef', parser } describe "#dir_name" do specify { mod.dir_name.must_equal 'puppet' } @@ -58,6 +59,13 @@ module Kafo specify { plugin2_mod.params_class_name.must_equal 'params' } end + describe "#raw_data" do + it "returns data from parser" do + mod.parse + mod.raw_data.must_equal parser.parse(mod.manifest_path) + end + end + let(:parsed) { mod.parse } describe "#parse(builder)" do @@ -85,6 +93,17 @@ module Kafo end end + describe "with parser cache" do + before do + KafoConfigure.config.app[:parser_cache_path] = ParserCacheFactory.build( + {:files => {"puppet" => {:data => {:parameters => [], :groups => []}}}} + ).path + end + + specify { parsed.raw_data[:parameters].must_equal [] } + specify { parsed.raw_data[:groups].must_equal [] } + end + describe "with groups" do let(:groups) { parsed.groups.map(&:name) } specify { groups.must_include('Parameters') } diff --git a/test/parser_cache_factory.rb b/test/parser_cache_factory.rb new file mode 100644 index 00000000..907013a1 --- /dev/null +++ b/test/parser_cache_factory.rb @@ -0,0 +1,20 @@ +require 'tempfile' +require 'yaml' + +class ParserCacheFactory + @caches = {} + + def self.build(content) + content = {:version => 1}.merge(content).to_yaml if content.is_a?(Hash) + key = content.hash + @caches[key] or @caches[key] = build_file(content) + @caches[key] + end + + def self.build_file(content) + f = Tempfile.new(['testing_cache', '.json']) + f.write content + f.close + f + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index a120e205..342131fa 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -8,6 +8,7 @@ require 'manifest_file_factory' require 'config_file_factory' +require 'parser_cache_factory' require 'test_parser' require 'kafo' diff --git a/test/test_parser.rb b/test/test_parser.rb index db709cdc..0baa3a93 100644 --- a/test/test_parser.rb +++ b/test/test_parser.rb @@ -1,15 +1,13 @@ class TestParser + attr_reader :manifest_file def initialize(manifest = BASIC_MANIFEST) @manifest = manifest + @manifest_file = ManifestFileFactory.build(manifest).path end # we use @manifest instead of manifest for testing def parse(manifest) - self.class.parse(@manifest) - end - - def self.parse(manifest) - KafoParsers::PuppetModuleParser.parse(ManifestFileFactory.build(manifest).path) + KafoParsers::PuppetModuleParser.parse(manifest_file) end end