Merge pull request #306 from seanmil/get_call_optimization

Provider get() call optimization

Author: LukasAud

lib/puppet/resource_api.rb

@@ -5,6 +5,7 @@
 require 'puppet/resource_api/glue'
 require 'puppet/resource_api/parameter'
 require 'puppet/resource_api/property'
+require 'puppet/resource_api/provider_get_cache'
 require 'puppet/resource_api/puppet_context' unless RUBY_PLATFORM == 'java'
 require 'puppet/resource_api/read_only_parameter'
 require 'puppet/resource_api/transport'
@@ -69,6 +70,15 @@ def type_definition
         apply_to_device
       end
 
+      define_singleton_method(:rsapi_provider_get_cache) do
+        # This gives a new cache per resource provider on each Puppet run:
+        @rsapi_provider_get_cache ||= Puppet::ResourceApi::ProviderGetCache.new
+      end
+
+      def rsapi_provider_get_cache
+        self.class.rsapi_provider_get_cache
+      end
+
       def initialize(attributes)
         # $stderr.puts "A: #{attributes.inspect}"
         if attributes.is_a? Puppet::Resource
@@ -154,8 +164,17 @@ def generate
       end
 
       def rsapi_current_state
-        refresh_current_state unless @rsapi_current_state
-        @rsapi_current_state
+        return @rsapi_current_state if @rsapi_current_state
+        # If the current state is not set, then check the cache and, if a value is
+        # found, ensure it passes strict_check before allowing it to be used:
+        cached_value = rsapi_provider_get_cache.get(rsapi_title)
+        strict_check(cached_value) if cached_value
+        @rsapi_current_state = cached_value
+      end
+
+      def rsapi_current_state=(value)
+        rsapi_provider_get_cache.add(rsapi_title, value)
+        @rsapi_current_state = value
       end
 
       def to_resource
@@ -242,57 +261,77 @@ def to_resource_shim(resource)
         type_definition.create_attribute_in(self, name, param_or_property, parent, options)
       end
 
+      def self.rsapi_provider_get(names = nil)
+        # If the cache has been marked as having all instances, then just return the
+        # full contents:
+        return rsapi_provider_get_cache.all if rsapi_provider_get_cache.cached_all? && names.nil?
+
+        fetched = if type_definition.feature?('simple_get_filter')
+                    my_provider.get(context, names)
+                  else
+                    my_provider.get(context)
+                  end
+
+        fetched.each do |resource_hash|
+          type_definition.check_schema(resource_hash)
+          rsapi_provider_get_cache.add(build_title(type_definition, resource_hash), resource_hash)
+        end
+
+        if names.nil? && !type_definition.feature?('simple_get_filter')
+          # Mark the cache as having all possible instances:
+          rsapi_provider_get_cache.cached_all
+        end
+
+        fetched
+      end
+
       def self.instances
         # puts 'instances'
         # force autoloading of the provider
         provider(type_definition.name)
 
-        initial_fetch = if type_definition.feature?('simple_get_filter')
-                          my_provider.get(context, nil)
-                        else
-                          my_provider.get(context)
-                        end
-
-        initial_fetch.map do |resource_hash|
-          type_definition.check_schema(resource_hash)
+        rsapi_provider_get.map do |resource_hash|
           # allow a :title from the provider to override the default
           result = if resource_hash.key? :title
                      new(title: resource_hash[:title])
                    else
                      new(title: build_title(type_definition, resource_hash))
                    end
+          # Cache the state in the generated resource, but unfortunately
+          # this only benefits "puppet resource", not apply runs:
           result.cache_current_state(resource_hash)
           result
         end
       end
 
       def refresh_current_state
-        @rsapi_current_state = if type_definition.feature?('simple_get_filter')
-                                 my_provider.get(context, [rsapi_title]).find { |h| namevar_match?(h) }
-                               else
-                                 my_provider.get(context).find { |h| namevar_match?(h) }
-                               end
-
-        if @rsapi_current_state
-          type_definition.check_schema(@rsapi_current_state)
-          strict_check(@rsapi_current_state)
+        current_state = self.class.rsapi_provider_get([rsapi_title]).find { |h| namevar_match?(h) }
+
+        if current_state
+          strict_check(current_state)
         else
-          @rsapi_current_state = if rsapi_title.is_a? Hash
-                                   rsapi_title.dup
-                                 else
-                                   { title: rsapi_title }
-                                 end
-          @rsapi_current_state[:ensure] = :absent if type_definition.ensurable?
+          current_state = if rsapi_title.is_a? Hash
+                            rsapi_title.dup
+                          else
+                            { title: rsapi_title }
+                          end
+          current_state[:ensure] = :absent if type_definition.ensurable?
         end
+        self.rsapi_current_state = current_state
       end
 
-      # Use this to set the current state from the `instances` method
+      # Use this to set the current state from the `instances` method. "puppet resources"
+      # needs this to minimize provider get() calls, but during a Puppet apply run
+      # the instances() method is only used by resource generation, and resource
+      # generators use and then discard the resources created by `instances``, so this
+      # does not help with that:
       def cache_current_state(resource_hash)
-        @rsapi_current_state = resource_hash
-        strict_check(@rsapi_current_state)
+        self.rsapi_current_state = resource_hash
+        strict_check(resource_hash)
       end
 
       def retrieve
+        refresh_current_state unless rsapi_current_state
         Puppet.debug("Current State: #{rsapi_current_state.inspect}")
 
         result = Puppet::Resource.new(self.class, title, parameters: rsapi_current_state)
@@ -316,17 +355,17 @@ def flush
         # puts 'flush'
         target_state = rsapi_canonicalized_target_state
 
-        retrieve unless @rsapi_current_state
+        retrieve unless rsapi_current_state
 
-        return if @rsapi_current_state == target_state
+        return if rsapi_current_state == target_state
 
         Puppet.debug("Target State: #{target_state.inspect}")
 
         # enforce init_only attributes
-        if Puppet.settings[:strict] != :off && @rsapi_current_state && (@rsapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
+        if Puppet.settings[:strict] != :off && rsapi_current_state && (rsapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
           target_state.each do |name, value|
-            next unless type_definition.attributes[name][:behaviour] == :init_only && value != @rsapi_current_state[name]
-            message = "Attempting to change `#{name}` init_only attribute value from `#{@rsapi_current_state[name]}` to `#{value}`"
+            next unless type_definition.attributes[name][:behaviour] == :init_only && value != rsapi_current_state[name]
+            message = "Attempting to change `#{name}` init_only attribute value from `#{rsapi_current_state[name]}` to `#{value}`"
             case Puppet.settings[:strict]
             when :warning
               Puppet.warning(message)
@@ -337,17 +376,17 @@ def flush
         end
 
         if type_definition.feature?('supports_noop')
-          my_provider.set(context, { rsapi_title => { is: @rsapi_current_state, should: target_state } }, noop: noop?)
+          my_provider.set(context, { rsapi_title => { is: rsapi_current_state, should: target_state } }, noop: noop?)
         else
-          my_provider.set(context, rsapi_title => { is: @rsapi_current_state, should: target_state }) unless noop?
+          my_provider.set(context, rsapi_title => { is: rsapi_current_state, should: target_state }) unless noop?
         end
         if context.failed?
           context.reset_failed
           raise 'Execution encountered an error'
         end
 
         # remember that we have successfully reached our desired state
-        @rsapi_current_state = target_state
+        self.rsapi_current_state = target_state
       end
 
       def raise_missing_attrs

lib/puppet/resource_api/provider_get_cache.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+# rubocop:disable Style/Documentation
+module Puppet; end
+module Puppet::ResourceApi; end
+# rubocop:enable Style/Documentation
+
+# This class provides a simple caching mechanism to support minimizing get()
+# calls into a provider.
+class Puppet::ResourceApi::ProviderGetCache
+  def initialize
+    clear
+  end
+
+  def clear
+    @cache = {}
+    @cached_all = false
+  end
+
+  def all
+    raise 'all method called, but cached_all not called' unless @cached_all
+    @cache.values
+  end
+
+  def add(key, value)
+    @cache[key] = value
+  end
+
+  def get(key)
+    @cache[key]
+  end
+
+  def cached_all
+    @cached_all = true
+  end
+
+  def cached_all?
+    @cached_all
+  end
+end

spec/acceptance/composite_namevar_spec.rb

@@ -33,7 +33,9 @@
       stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php/gem")
       expect(stdout_str.strip).to match %r{^composite_namevar \{ \'php/gem\'}
       expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
-      expect(stdout_str.strip).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]}
+      # "Looking for" will return nil as puppet resource will have already fetched
+      # the resource in instances():
+      expect(stdout_str.strip).to match %r{Looking for nil}
       expect(status.exitstatus).to eq 0
     end
     it 'properly identifies an absent resource if only the title is provided' do

spec/acceptance/device_spec.rb

@@ -36,7 +36,7 @@
                    'Returned values:       \{:name=>"wibble", :ensure=>"present", :string=>"sample", :string_ro=>"fixed"\}\n'\
                    'Canonicalized values:  \{:name=>"wibble", :ensure=>"present", :string=>"changed", :string_ro=>"fixed"\}'
         expect(stdout_str).to match %r{#{stdmatch}}
-        expect(status).to be_success
+        expect(status).not_to be_success
       end
     end
 

spec/acceptance/get_calls_spec.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'tempfile'
+
+RSpec.describe 'minimizing provider get calls' do
+  let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
+
+  describe 'using `puppet resource` with a basic type' do
+    it 'calls get 1 time' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} test_get_calls_basic")
+      expect(stdout_str).to match %r{Notice: test_get_calls_basic: Provider get called 1 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_basic: Provider get called 2 times}
+      expect(status).to eq 0
+    end
+  end
+
+  describe 'using `puppet apply` with a basic type' do
+    it 'calls get 2 times' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_get_calls_basic { foo: } test_get_calls_basic { bar: }\"")
+      expect(stdout_str).to match %r{Notice: test_get_calls_basic: Provider get called 1 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_basic: Provider get called 2 times}
+      expect(stdout_str).not_to match %r{Creating}
+    end
+
+    it 'calls get 1 time with resource purging' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_get_calls_basic { foo: } test_get_calls_basic { bar: } resources { test_get_calls_basic: purge => true }\"")
+      expect(stdout_str).to match %r{Notice: test_get_calls_basic: Provider get called 1 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_basic: Provider get called 2 times}
+      expect(stdout_str).not_to match %r{Creating}
+    end
+  end
+
+  describe 'using `puppet resource` with a simple_get_filter type' do
+    it 'calls get 1 time' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} test_get_calls_sgf")
+      expect(stdout_str).to match %r{Notice: test_get_calls_sgf: Provider get called 1 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_sgf: Provider get called 2 times}
+      expect(status.exitstatus).to be_zero
+    end
+  end
+
+  describe 'using `puppet apply` with a type using simple_get_filter' do
+    it 'calls get 2 times' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_get_calls_sgf { foo: } test_get_calls_sgf { bar: }\"")
+      expect(stdout_str).to match %r{Notice: test_get_calls_sgf: Provider get called 1 times}
+      expect(stdout_str).to match %r{Notice: test_get_calls_sgf: Provider get called 2 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_sgf: Provider get called 3 times}
+      expect(stdout_str).not_to match %r{Creating}
+    end
+
+    it 'calls get 1 time when resource purging' do
+      stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_get_calls_sgf { foo: } test_get_calls_sgf { bar: } resources { test_get_calls_sgf: purge => true }\"")
+      expect(stdout_str).to match %r{Notice: test_get_calls_sgf: Provider get called 1 times}
+      expect(stdout_str).not_to match %r{Notice: test_get_calls_sgf: Provider get called 2 times}
+      expect(stdout_str).not_to match %r{Creating}
+    end
+  end
+end

spec/fixtures/test_module/lib/puppet/provider/test_get_calls_basic/test_get_calls_basic.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'puppet/resource_api/simple_provider'
+
+# Implementation for the test_get_calls_basic type using the Resource API.
+class Puppet::Provider::TestGetCallsBasic::TestGetCallsBasic < Puppet::ResourceApi::SimpleProvider
+  def get(context)
+    @count ||= 0
+    @count += 1
+    context.notice("Provider get called #{@count} times")
+    [
+      {
+        name: 'foo',
+        ensure: 'present',
+        prop: 'fooprop',
+      },
+      {
+        name: 'bar',
+        ensure: 'present',
+        prop: 'barprop',
+      },
+    ]
+  end
+
+  def create(context, name, should)
+    context.notice("Creating '#{name}' with #{should.inspect}")
+  end
+
+  def update(context, name, should)
+    context.notice("Updating '#{name}' with #{should.inspect}")
+  end
+
+  def delete(context, name)
+    context.notice("Deleting '#{name}'")
+  end
+end

spec/fixtures/test_module/lib/puppet/provider/test_get_calls_sgf/test_get_calls_sgf.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require 'puppet/resource_api/simple_provider'
+
+# Implementation for the test_get_calls_sgf type using the Resource API.
+class Puppet::Provider::TestGetCallsSgf::TestGetCallsSgf < Puppet::ResourceApi::SimpleProvider
+  def get(context, names = nil)
+    @count ||= 0
+    @count += 1
+    context.notice("Provider get called #{@count} times with names=#{names}")
+    data = [
+      {
+        name: 'foo',
+        ensure: 'present',
+      },
+      {
+        name: 'bar',
+        ensure: 'present',
+      },
+    ]
+    if names.nil?
+      data
+    else
+      data.select { |r| names.include?(r[:name]) }
+    end
+  end
+
+  def create(context, name, should)
+    context.notice("Creating '#{name}' with #{should.inspect}")
+  end
+
+  def update(context, name, should)
+    context.notice("Updating '#{name}' with #{should.inspect}")
+  end
+
+  def delete(context, name)
+    context.notice("Deleting '#{name}'")
+  end
+end