From bab0e66421da4483821d666eb75f2fd9e0853919 Mon Sep 17 00:00:00 2001 From: BogdanIrimie Date: Fri, 28 Aug 2020 11:51:38 +0300 Subject: [PATCH] (FACT-2753) Resolve facts sequentially. --- acceptance/tests/options/parallel.rb | 11 ++++ facter.gemspec | 1 + lib/facter.rb | 12 ++++ lib/facter/framework/cli/cli.rb | 4 ++ .../framework/core/fact/internal/core_fact.rb | 2 +- .../fact/internal/internal_fact_manager.rb | 66 ++++++++++++++++--- .../framework/core/options/option_store.rb | 4 +- .../core/options/option_store_spec.rb | 3 +- 8 files changed, 91 insertions(+), 12 deletions(-) create mode 100644 acceptance/tests/options/parallel.rb diff --git a/acceptance/tests/options/parallel.rb b/acceptance/tests/options/parallel.rb new file mode 100644 index 0000000000..56c75fd91d --- /dev/null +++ b/acceptance/tests/options/parallel.rb @@ -0,0 +1,11 @@ +test_name "--parallel argument does not generate any errors" do + tag 'risk:high' + + agents.each do |agent| + step "--parallel should generate no errors" do + on(agent, facter("--parallel --debug"), :acceptable_exit_codes => 0) do |facter_output| + assert_match(/Resolving fact in parallel/, facter_output.stderr, "Resolving facts in parallel") + end + end + end +end diff --git a/facter.gemspec b/facter.gemspec index d65ff57f96..2186662e57 100644 --- a/facter.gemspec +++ b/facter.gemspec @@ -33,6 +33,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'sys-filesystem', '~> 1.3' spec.add_development_dependency 'yard', '~> 0.9' + spec.add_runtime_dependency 'concurrent-ruby' spec.add_runtime_dependency 'hocon', '~> 1.3' spec.add_runtime_dependency 'thor', ['>= 1.0.1', '< 2.0'] end diff --git a/lib/facter.rb b/lib/facter.rb index 16fac3093c..3f7161d7db 100644 --- a/lib/facter.rb +++ b/lib/facter.rb @@ -104,6 +104,18 @@ def debugging(debug_bool) Facter::Options[:debug] = debug_bool end + def enable_parallel + Facter::Options[:parallel] = true + end + + def disable_parallel + Facter::Options[:parallel] = false + end + + def parallel? + Facter::Options[:parallel] + end + # Returns a fact object by name. If you use this, you still have to # call {Facter::Util::Fact#value `value`} on it to retrieve the actual # value. diff --git a/lib/facter/framework/cli/cli.rb b/lib/facter/framework/cli/cli.rb index b4b216d415..d660cacda8 100755 --- a/lib/facter/framework/cli/cli.rb +++ b/lib/facter/framework/cli/cli.rb @@ -108,6 +108,10 @@ class Cli < Thor aliases: '-p', desc: 'Load the Puppet libraries, thus allowing Facter to load Puppet-specific facts.' + class_option :parallel, + type: :boolean, + desc: 'Resolve facts in parallel' + desc '--man', 'Manual', hide: true map ['--man'] => :man def man(*args) diff --git a/lib/facter/framework/core/fact/internal/core_fact.rb b/lib/facter/framework/core/fact/internal/core_fact.rb index 3d6772a0a7..f25e4c8834 100644 --- a/lib/facter/framework/core/fact/internal/core_fact.rb +++ b/lib/facter/framework/core/fact/internal/core_fact.rb @@ -9,7 +9,7 @@ def initialize(searched_fact) def create fact_class = @searched_fact.fact_class - fact_class.new.call_the_resolver + fact_class&.new&.call_the_resolver end end end diff --git a/lib/facter/framework/core/fact/internal/internal_fact_manager.rb b/lib/facter/framework/core/fact/internal/internal_fact_manager.rb index b536a55980..6c469362d4 100644 --- a/lib/facter/framework/core/fact/internal/internal_fact_manager.rb +++ b/lib/facter/framework/core/fact/internal/internal_fact_manager.rb @@ -6,8 +6,16 @@ class InternalFactManager def resolve_facts(searched_facts) internal_searched_facts = filter_internal_facts(searched_facts) - threads = start_threads(internal_searched_facts) - resolved_facts = join_threads(threads, internal_searched_facts) + + resolved_facts = if Options[:parallel] + @@log.debug('Resolving fact in parallel') + threads = start_threads(internal_searched_facts) + join_threads(threads, internal_searched_facts) + # thread_pool(internal_searched_facts) + else + @@log.debug('Resolving facts sequentially') + resolve_sequentially(internal_searched_facts) + end nil_resolved_facts = resolve_nil_facts(searched_facts) @@ -29,6 +37,25 @@ def resolve_nil_facts(searched_facts) resolved_facts end + def resolve_sequentially(searched_facts) + resolved_facts = [] + + searched_facts + .uniq { |searched_fact| searched_fact.fact_class.name } + .each do |searched_fact| + begin + fact = CoreFact.new(searched_fact) + fact_value = fact.create + resolved_facts << fact_value unless fact_value.nil? + rescue StandardError => e + @@log.log_exception(e) + end + end + + resolved_facts.flatten! + FactAugmenter.augment_resolved_facts(searched_facts, resolved_facts) + end + def start_threads(searched_facts) threads = [] # only resolve a fact once, even if multiple search facts depend on that fact @@ -36,13 +63,7 @@ def start_threads(searched_facts) .uniq { |searched_fact| searched_fact.fact_class.name } .each do |searched_fact| threads << Thread.new do - begin - fact = CoreFact.new(searched_fact) - fact.create - rescue StandardError => e - @@log.log_exception(e) - nil - end + resolve_fact(searched_fact) end end @@ -61,5 +82,32 @@ def join_threads(threads, searched_facts) FactAugmenter.augment_resolved_facts(searched_facts, resolved_facts) end + + def thread_pool(searched_facts) + require 'concurrent' + + pool = Concurrent::FixedThreadPool.new(12) + pr_futures = [] + + searched_facts + .uniq { |searched_fact| searched_fact.fact_class.name } + .each do |searched_fact| + pr_futures << Concurrent::Promises.future_on(pool) do + resolve_fact(searched_fact) + end + end + + resolved_facts = Concurrent::Promises.zip(*pr_futures).value!.flatten.compact + + FactAugmenter.augment_resolved_facts(searched_facts, resolved_facts) + end + + def resolve_fact(searched_fact) + fact = CoreFact.new(searched_fact) + fact.create + rescue StandardError => e + @@log.log_exception(e) + nil + end end end diff --git a/lib/facter/framework/core/options/option_store.rb b/lib/facter/framework/core/options/option_store.rb index 7cc586ca7e..2a56fccbdd 100644 --- a/lib/facter/framework/core/options/option_store.rb +++ b/lib/facter/framework/core/options/option_store.rb @@ -23,6 +23,7 @@ class OptionStore @block_list = {} @fact_groups = {} @color = false + @parallel = false class << self attr_reader :debug, :verbose, :log_level, :show_legacy, :ruby, @@ -31,7 +32,7 @@ class << self attr_accessor :config, :user_query, :strict, :json, :haml, :external_facts, :cache, :yaml, :puppet, :ttls, :block, :cli, :config_file_custom_dir, :config_file_external_dir, :default_external_dir, :fact_groups, - :block_list, :color, :trace + :block_list, :color, :trace, :parallel attr_writer :external_dir @@ -159,6 +160,7 @@ def reset_config @blocked_facts = [] @fact_groups = {} @block_list = {} + @parallel = false end def fallback_external_dir diff --git a/spec/framework/core/options/option_store_spec.rb b/spec/framework/core/options/option_store_spec.rb index 95cfadf55f..816e667c42 100644 --- a/spec/framework/core/options/option_store_spec.rb +++ b/spec/framework/core/options/option_store_spec.rb @@ -35,7 +35,8 @@ config: nil, cache: true, color: false, - trace: false + trace: false, + parallel: false ) end end