Enable caching for backend calls #2309

jquick · 2017-11-15T18:33:19Z

This PR enables caching for all calls to the train backend. Currently this is set on the profile level. This was inspired by @sdelano and his work via #2158

Signed-off-by: Jared Quick jquick@chef.io

adamleff

This is a great first pass, @jquick! I have some questions and some cleanup items.

adamleff · 2017-11-15T20:05:57Z

lib/inspec/profile.rb

@@ -124,6 +127,10 @@ def version
      metadata.params[:version]
    end

+    def cache_resources?
+      metadata.params[:cache_resources] || false


|| logic with default boolean values always makes me nervous. If someone decides to make this default behavior and just changes the false to true, it will never be able to be set to false:

[1] pry(main)> a = true => true [2] pry(main)> a || true => true [3] pry(main)> a = false => false [4] pry(main)> a || true => true

To avoid a silly regression like that, how about we change this to use #fetch instead?

metadata.params.fetch(:cache_resources, false)

adamleff · 2017-11-15T20:07:47Z

lib/inspec/transport.rb

+
+module Inspec
+  class Transport
+    attr_accessor :cache_resources, :conn


Unless it's necessary, I'd rather we didn't abbreviate variable names. (I know we just did it in the Train platforms PR - i.e. plat instead of platform - but it still bugs me 🙂 )

Any reason this shouldn't be connection?

adamleff · 2017-11-15T20:08:46Z

lib/inspec/transport.rb

+    alias platform os
+
+    # This is needed for the mock connection which has some
+    # unique method calls


Can we provide an example or two in the comment as to the type of method calls we're catching here, deeming this necessary?

adamleff · 2017-11-15T20:11:47Z

lib/inspec/transport.rb

+      if @cache_resources == true && @cmd_cache.key?(cmd)
+        @cmd_cache[cmd]
+      elsif @cache_resources == true
+        Inspec::Log.warn('running raw command and caching')


These should be debug messages, no?

adamleff · 2017-11-15T20:12:00Z

lib/inspec/transport.rb

+        Inspec::Log.warn('running raw command and caching')
+        @cmd_cache[cmd] = @conn.run_command(cmd)
+      else
+        Inspec::Log.warn('running raw command')


Debug instead of warn?

adamleff · 2017-11-15T20:20:13Z

test/unit/transport_test.rb

+
+    describe 'create train connection' do
+      it 'return new train transport' do
+        trans = transport


What's the purpose of assigning this to a new variable? This happens throughout the tests, so I'm not going to comment on all of them, but I'm curious as to why it's necessary.

adamleff · 2017-11-15T20:22:54Z

test/unit/transport_test.rb

+      it 'check mock_os method' do
+        trans = transport
+        os = trans.mock_os({ name: 'mock'})
+        os.must_be_kind_of Train::Transports::Mock::Connection::OS


Should probably also check that os[:name] == 'mock' here to properly exercise the mock_os method.

adamleff · 2017-11-15T20:24:25Z

test/unit/transport_test.rb

+
+      it 'load file with caching' do
+        trans = transport
+        trans.instance_variable_set(:@cache_resources, true)


Isn't cache_resources an attr_accessor? I like to avoid setting/getting instance variables unless necessary. I think we can just change this to:

transport.cache_resources = true

adamleff · 2017-11-15T20:25:02Z

test/unit/transport_test.rb

+
+      it 'run command with caching' do
+        trans = transport
+        trans.instance_variable_set(:@cache_resources, true)


Same as above re: using the attr_accessor on the transport.

jquick · 2017-11-15T21:19:02Z

@adamleff - Thanks for the feedback. I think I have those items resolved.

chris-rock · 2017-11-17T10:39:36Z

This is a great improvement @jquick and a very good start for a discussion!

After thinking for a while, I came to the conclusion that it would be best to implement a CacheConnection in train that would take another connection as initialize argument. This would solve multiple problems:

every train user can use it, not just inspec
the decision is still up to the train user if he wants to use caching or not
InSpec will just use the cache connection for normal execution, but not for shell

This also means we should clean up the simple caching as we have it in train: https://github.com/chef/train/blob/master/lib/train/transports/docker.rb#L73-L80

jquick · 2017-11-17T12:56:41Z

@chris-rock - Thanks for the feedback. I will move the caching to Train. How do you feel about keeping the toggle here to enable/disable on the profile level?

adamleff · 2017-11-17T13:02:14Z

Moving it to Train seems totally fine to me, too. I still think we need to keep the toggle here on the InSpec side to allow the InSpec profile author to opt-in to the caching, and the InSpec will use the proper Train transport accordingly.

chris-rock · 2017-11-27T09:37:49Z

lib/inspec/backend.rb

@@ -1,9 +1,4 @@
 # encoding: utf-8
-# copyright: 2015, Dominik Richter


I think you cannot remove the copyright, since it stays available. This was developed by Dom before it was acquired by Chef

chris-rock · 2017-11-27T09:38:17Z

lib/inspec/backend.rb

@@ -32,7 +27,7 @@ def to_s

      # Ruby internal for pretty-printing a summary for this class
      def inspect
-        "Inspec::Backend::Class @transport=#{backend.class}"
+        "Inspec::Backend::Class @transport=#{@backend.connection.class}"


I think we do not need this here anymore

chris-rock

Nice cleanup @jquick I think we should not leave it to the profile author if caching is enabled or not. This is a decision of the runtime. We may decided to leave caching of for now but for InSpec 2.0 we may turn this on by default.

chris-rock · 2017-11-27T13:48:37Z

lib/inspec/profile.rb

@@ -124,6 +126,10 @@ def version
      metadata.params[:version]
    end

+    def cache_resources?
+      metadata.params.fetch(:cache_resources, false)


I think we should not make the caching part of the inspec.yml since this is static content. From my perspective, this needs to be a flag for inspec cli where by default caching is turned off

jquick · 2017-11-27T15:29:53Z

This should be set. Flag was set to --backend-cache/--no-backend-cache as --cache was already taken for profile caching on exec. Open to any other name though.

chris-rock · 2017-11-27T15:38:09Z

lib/inspec/base_cli.rb

@@ -15,6 +15,8 @@ def self.target_options
        desc: 'Choose a backend: local, ssh, winrm, docker.'
      option :host, type: :string,
        desc: 'Specify a remote host which is tested.'
+      option :backend_cache, type: :boolean,


i think we should just call it cache

maybe we set the default here? default: false,

adamleff

I'd like to discuss this PR in more depth, perhaps after standup today. Comments in-line. Also, we cannot merge this until Train has a released version that supports the enable/disable calls. At that time, this PR will need to be updated to include a gemspec update so we properly depend on a version of Train that supports the feature.

adamleff · 2017-11-27T17:17:24Z

lib/inspec/backend.rb

        define_method :backend do
+          if @cache_resources


Is there another place we can/should put the enable_cache calls? The way I read this, every time something in InSpec calls for the backend it will send these methods to the Train connection. That could happen a LOT.

Maybe we just need an enable/disable method on the backend itself that can be called once when appropriate?

Also, we're making an assumption here that caching is disabled by default in Train, which is half correct -- we always cache files, and don't provide a way to disable currently. Should this have an else statement where we disable the cache?

This probably warrants further discussion after standup... until Train is consistent with the default behavior of both files and commands, we may want to consider only manipulating the command cache for now and then deal with files later.

jquick · 2017-11-27T20:36:50Z

Please note: test will bomb until we upgrade train with the caching changes

adamleff

One last question regarding the caching settings conditional and we should be good to go.

adamleff · 2017-11-27T22:48:57Z

lib/inspec/backend.rb

+      elsif config[:debug_shell]
+        connection.disable_cache(:file)
+        connection.disable_cache(:command)
+      elsif !config[:backend_cache] && config[:backend] != :mock


Could you explain why this can't be an else? It's not quite clear to me. And probably warrants a code comment explaining the condition (i.e. why the mock backend is special here).

Yeah I'll add a comment. Basically the mock transport uses the base connection caching also. So we never want to disable it. We hard set the cache values for testing and usually bypass thor so it would always be false. It actually may be cleaner to put the mock check on the enable and just ensure its enabled.

What would happen in the mock transport if it was disabled? Slower tests? If so, by how much?

Not really, you cant actually run file/command calls via mock without preloading it. Else it will just fall to errors. So for the core of MockTransport to work caching is always needed. Ex:
https://github.com/chef/train/blob/master/lib/train/transports/mock.rb#L139

We hard code it during the transport creation but this would override it.

adamleff

Conditionally approved. Before we merge:

a new version of Train needs to be released
this PR needs to be updated with a gemspec pin update for the train gem
tests go green

Nice work, @jquick!

chris-rock · 2017-11-28T08:29:41Z

lib/inspec/backend.rb

+        connection.disable_cache(:file)
+        connection.disable_cache(:command)
+      else
+        connection.disable_cache(:command)


should we explicitly enable file caching? so we do not depend on train defaults?

chris-rock

Great improvement @jquick

jquick · 2017-12-04T19:41:21Z

This is tested and looks good. We wont have as good windows caching until #2368 is merged. It would be nice to rebase this again after that is merged and make sure my numbers match up.

Signed-off-by: Jared Quick <jquick@chef.io>

jquick · 2017-12-04T20:51:11Z

Confirmed the cache timings on various windows and unix systems. This looks good.

jquick requested a review from a team as a code owner November 15, 2017 18:33

jquick force-pushed the jq/backend_caching branch from 43e28fb to 4c0cae6 Compare November 15, 2017 18:36

jquick changed the title ~~Enable caching for for backend calls~~ Enable caching for all backend calls Nov 15, 2017

jquick changed the title ~~Enable caching for all backend calls~~ Enable caching for backend calls Nov 15, 2017

jquick force-pushed the jq/backend_caching branch from 4c0cae6 to fe450e8 Compare November 15, 2017 19:25

jquick requested review from chris-rock and adamleff November 15, 2017 20:18

adamleff suggested changes Nov 15, 2017

View reviewed changes

chris-rock reviewed Nov 27, 2017

View reviewed changes

jquick mentioned this pull request Nov 27, 2017

Enable caching on connections inspec/train#214

Merged

chris-rock suggested changes Nov 27, 2017

View reviewed changes

chris-rock reviewed Nov 27, 2017

View reviewed changes

chris-rock mentioned this pull request Nov 27, 2017

use --vendor-cache instead of --cache #2339

Closed

adamleff suggested changes Nov 27, 2017

View reviewed changes

jquick force-pushed the jq/backend_caching branch from 1e7d3a0 to 33c66f9 Compare November 27, 2017 20:26

adamleff suggested changes Nov 27, 2017

View reviewed changes

adamleff approved these changes Nov 28, 2017

View reviewed changes

chris-rock reviewed Nov 28, 2017

View reviewed changes

chris-rock approved these changes Nov 29, 2017

View reviewed changes

adamleff added Type: New Feature Adds new functionality Version: Bump Minor labels Dec 4, 2017

jquick force-pushed the jq/backend_caching branch 3 times, most recently from 44dbc93 to 6baa01a Compare December 4, 2017 17:49

jquick force-pushed the jq/backend_caching branch from 9752f7e to 6baa01a Compare December 4, 2017 20:38

jquick added 9 commits December 4, 2017 15:39

Enable caching for command and file calls to train

6cb7fdc

Signed-off-by: Jared Quick <jquick@chef.io>

Moved transport conn to connection and refactored tests

b052521

Signed-off-by: Jared Quick <jquick@chef.io>

Update caching flag to use train caching.

378bf3d

Signed-off-by: Jared Quick <jquick@chef.io>

Move caching flag to cli option.

9a498d3

Signed-off-by: Jared Quick <jquick@chef.io>

Add backed cache default from thor.

e2e1b11

Signed-off-by: Jared Quick <jquick@chef.io>

Add hard disable for cache option and remove all cache from debug shell.

d90b752

Signed-off-by: Jared Quick <jquick@chef.io>

Add comment to caching settings conditional.

b8c0510

Signed-off-by: Jared Quick <jquick@chef.io>

Force file cache on when caching enabled.

af930cb

Signed-off-by: Jared Quick <jquick@chef.io>

Update gemspec for train 0.30.0.

f90fd28

Signed-off-by: Jared Quick <jquick@chef.io>

jquick force-pushed the jq/backend_caching branch from 6baa01a to f90fd28 Compare December 4, 2017 20:41

adamleff merged commit d49f4e3 into master Dec 4, 2017

adamleff deleted the jq/backend_caching branch December 4, 2017 21:40

adamleff mentioned this pull request Dec 5, 2017

[win-perf] reduce number of powershell commands #2158

Closed

zenspider added Expeditor: Bump Minor Version and removed Version: Bump Minor labels Nov 15, 2019

		@@ -1,9 +1,4 @@
		# encoding: utf-8
		# copyright: 2015, Dominik Richter

Enable caching for backend calls #2309

Enable caching for backend calls #2309

Conversation

jquick commented Nov 15, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 15, 2017

chris-rock commented Nov 17, 2017

jquick commented Nov 17, 2017

adamleff commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 27, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

jquick commented Dec 4, 2017 • edited Loading

jquick commented Dec 4, 2017

jquick commented Nov 27, 2017 •

edited

Loading

jquick Nov 27, 2017 •

edited

Loading

jquick commented Dec 4, 2017 •

edited

Loading