From aea1812213a3f0b61b366bb38a9ce8ee62033b7c Mon Sep 17 00:00:00 2001
From: Maggie Dreyer <maggie@puppet.com>
Date: Fri, 10 Sep 2021 13:14:04 -0700
Subject: [PATCH] [#1215] Ensure module postrun command only runs for modified
 envs

Previously, when a postrun command with interpolated environments was
specified, the `deploy module` command with no environments specified
would cause the command to be run for all environments that contained
the module, whether or not the module was actually updated by the
deploy. This could result in a huge amount of unnecessary work being
done.

This commit makes modules return the result of their syncing, true for
"updated" and false for "not updated". The environment's `deploy`
function collects this information, recording the names of each module
that was updated as part of the deploy. The `module deploy` action then
uses this to determine which environments actually had modules modified,
and uses that to populate the `$modifiedenvs` in the postrun command. So
now, the command will only be run for environments that actually updated
a module.

Note this does not affect environment deploys.
---
 lib/r10k/action/deploy/environment.rb  |  1 +
 lib/r10k/action/deploy/module.rb       |  9 ++++----
 lib/r10k/content_synchronizer.rb       | 20 +++++++++++++----
 lib/r10k/environment/base.rb           |  6 +++++-
 lib/r10k/git/stateful_repository.rb    |  4 ++++
 lib/r10k/module/base.rb                |  1 +
 lib/r10k/module/definition.rb          |  2 ++
 lib/r10k/module/forge.rb               |  6 ++++++
 lib/r10k/module/git.rb                 |  8 ++++++-
 lib/r10k/module/local.rb               |  2 ++
 lib/r10k/module/svn.rb                 |  5 +++++
 spec/unit/action/deploy/module_spec.rb | 30 ++++++--------------------
 spec/unit/module/forge_spec.rb         | 14 ++++++++----
 spec/unit/module/git_spec.rb           | 19 ++++++++++++++++
 spec/unit/module/svn_spec.rb           | 17 +++++++++------
 spec/unit/puppetfile_spec.rb           |  2 +-
 16 files changed, 101 insertions(+), 45 deletions(-)

diff --git a/lib/r10k/action/deploy/environment.rb b/lib/r10k/action/deploy/environment.rb
index 2e68befbe..075774acc 100644
--- a/lib/r10k/action/deploy/environment.rb
+++ b/lib/r10k/action/deploy/environment.rb
@@ -135,6 +135,7 @@ def visit_deployment(deployment)
               envs.reject! { |e| !requested_envs.include?(e) } if requested_envs.any?
               postcmd = postcmd.map { |e| e.gsub('$modifiedenvs', envs.join(' ')) }
             end
+            logger.debug _("Executing postrun command.")
             subproc = R10K::Util::Subprocess.new(postcmd)
             subproc.logger = logger
             subproc.execute
diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index 1c1ffd0cf..0631b9a18 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -78,9 +78,9 @@ def visit_deployment(deployment)
         ensure
           if (postcmd = @settings[:postrun])
             if @modified_envs.any?
+              envs_to_run = @modified_envs.join(' ')
+              logger.debug "Running postrun command for environments #{envs_to_run}."
               if postcmd.grep('$modifiedenvs').any?
-                envs_to_run = @modified_envs.join(' ')
-                logger.debug "Running postrun command for environments #{envs_to_run}."
                 postcmd = postcmd.map { |e| e.gsub('$modifiedenvs', envs_to_run) }
               end
               subproc = R10K::Util::Subprocess.new(postcmd)
@@ -103,11 +103,10 @@ def visit_environment(environment)
           else
             logger.debug1(_("Updating modules %{modules} in environment %{env_path}") % {modules: @settings.dig(:overrides, :modules, :requested_modules).inspect, env_path: environment.path})
 
-            environment.deploy
+            updated_modules = environment.deploy
 
-            requested_mods = @settings.dig(:overrides, :modules, :requested_modules) || []
             # We actually synced a module in this env
-            if !((environment.modules.map(&:name) & requested_mods).empty?)
+            if !(updated_modules.nil? || updated_modules.empty?)
               # Record modified environment for postrun command
               @modified_envs << environment.dirname
 
diff --git a/lib/r10k/content_synchronizer.rb b/lib/r10k/content_synchronizer.rb
index cb156af44..74052d055 100644
--- a/lib/r10k/content_synchronizer.rb
+++ b/lib/r10k/content_synchronizer.rb
@@ -8,24 +8,31 @@ def self.serial_accept(modules, visitor, loader)
     end
 
     def self.serial_sync(modules)
+      updated_modules = []
       modules.each do |mod|
-        mod.sync
+        updated = mod.sync
+        updated_modules << mod.name if updated
       end
+      updated_modules
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.concurrent_accept(modules, visitor, loader, pool_size, logger)
       mods_queue = modules_visit_queue(modules, visitor, loader)
       sync_queue(mods_queue, pool_size, logger)
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.concurrent_sync(modules, pool_size, logger)
       mods_queue = modules_sync_queue(modules)
       sync_queue(mods_queue, pool_size, logger)
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.sync_queue(mods_queue, pool_size, logger)
       logger.debug _("Updating modules with %{pool_size} threads") % {pool_size: pool_size}
-      thread_pool = pool_size.times.map { sync_thread(mods_queue, logger) }
+      updated_modules = Queue.new
+      thread_pool = pool_size.times.map { sync_thread(mods_queue, logger, updated_modules) }
       thread_exception = nil
 
       # If any threads raise an exception the deployment is considered a failure.
@@ -33,6 +40,8 @@ def self.sync_queue(mods_queue, pool_size, logger)
       # current work, then re-raise the first exception caught.
       begin
         thread_pool.each(&:join)
+        # Return the list of all modules that were actually updated
+        updated_modules
       rescue => e
         logger.error _("Error during concurrent deploy of a module: %{message}") % {message: e.message}
         mods_queue.clear
@@ -65,11 +74,14 @@ def self.enqueue_modules(queue, modules)
       modules_by_cachedir.values.each {|mods| queue << mods }
     end
 
-    def self.sync_thread(mods_queue, logger)
+    def self.sync_thread(mods_queue, logger, updated_modules)
       Thread.new do
         begin
           while mods = mods_queue.pop(true) do
-            mods.each { |mod| mod.sync }
+            mods.each do |mod|
+              udpated = mod.sync
+              updated_modules << mod.name if udpated
+            end
           end
         rescue ThreadError => e
           logger.debug _("Module thread %{id} exiting: %{message}") % {message: e.message, id: Thread.current.object_id}
diff --git a/lib/r10k/environment/base.rb b/lib/r10k/environment/base.rb
index 056dafcc3..a3df66ee8 100644
--- a/lib/r10k/environment/base.rb
+++ b/lib/r10k/environment/base.rb
@@ -147,6 +147,8 @@ def accept(visitor)
     end
   end
 
+
+  # Returns a Queue of the names of modules actually updated
   def deploy
     if @base_modules.nil?
       load_puppetfile_modules
@@ -154,13 +156,15 @@ def deploy
 
     if ! @base_modules.empty?
       pool_size = @overrides.dig(:modules, :pool_size)
-      R10K::ContentSynchronizer.concurrent_sync(@base_modules, pool_size, logger)
+      updated_modules = R10K::ContentSynchronizer.concurrent_sync(@base_modules, pool_size, logger)
     end
 
     if (@overrides.dig(:purging, :purge_levels) || []).include?(:puppetfile)
       logger.debug("Purging unmanaged Puppetfile content for environment '#{dirname}'...")
       @puppetfile_cleaner.purge!
     end
+
+    updated_modules
   end
 
   def load_puppetfile_modules
diff --git a/lib/r10k/git/stateful_repository.rb b/lib/r10k/git/stateful_repository.rb
index 867f00239..fbbe7a70a 100644
--- a/lib/r10k/git/stateful_repository.rb
+++ b/lib/r10k/git/stateful_repository.rb
@@ -35,6 +35,7 @@ def resolve(ref)
     @cache.resolve(ref)
   end
 
+  # Returns true if the sync actually updated the repo, false otherwise
   def sync(ref, force=true)
     @cache.sync if sync_cache?(ref)
 
@@ -46,6 +47,7 @@ def sync(ref, force=true)
 
     workdir_status = status(ref)
 
+    updated = true
     case workdir_status
     when :absent
       logger.debug(_("Cloning %{repo_path} and checking out %{ref}") % {repo_path: @repo.path, ref: ref })
@@ -67,7 +69,9 @@ def sync(ref, force=true)
       end
     else
       logger.debug(_("%{repo_path} is already at Git ref %{ref}") % {repo_path: @repo.path, ref: ref })
+      updated = false
     end
+    updated
   end
 
   def status(ref)
diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index 66fd4c1e3..d7a2cd8f6 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -106,6 +106,7 @@ def delete_spec_dir
 
   # Synchronize this module with the indicated state.
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
     raise NotImplementedError
   end
diff --git a/lib/r10k/module/definition.rb b/lib/r10k/module/definition.rb
index f275fb7ba..b66870953 100644
--- a/lib/r10k/module/definition.rb
+++ b/lib/r10k/module/definition.rb
@@ -23,8 +23,10 @@ def to_implementation
   end
 
   # syncing is a noop for module definitions
+  # Returns false to inidicate the module was not updated
   def sync(args = {})
     logger.debug1(_("Not updating module %{name}, assuming content unchanged") % {name: name})
+    false
   end
 
   def status
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index 7d53e1e3c..6d169013f 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -60,18 +60,24 @@ def initialize(title, dirname, opts, environment=nil)
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
+    updated = false
     if should_sync?
       case status
       when :absent
+        updated = true
         install
       when :outdated
+        updated = true
         upgrade
       when :mismatched
+        updated = true
         reinstall
       end
       maybe_delete_spec_dir
     end
+    updated
   end
 
   def properties
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index 7de55c592..f6873a799 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -98,10 +98,16 @@ def properties
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
     force = opts[:force] || @force
-    @repo.sync(version, force) if should_sync?
+    if should_sync?
+      updated = @repo.sync(version, force)
+    else
+      updated = false
+    end
     maybe_delete_spec_dir
+    updated
   end
 
   def status
diff --git a/lib/r10k/module/local.rb b/lib/r10k/module/local.rb
index f34681180..3fb29843b 100644
--- a/lib/r10k/module/local.rb
+++ b/lib/r10k/module/local.rb
@@ -28,7 +28,9 @@ def status
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] false, because local modules are always considered in-sync
   def sync(opts={})
     logger.debug1 _("Module %{title} is a local module, always indicating synced.") % {title: title}
+    false
   end
 end
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index d69389ca2..b83bdd3a0 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -75,17 +75,22 @@ def status
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
+    updated = false
     if should_sync?
       case status
       when :absent
+        updated = true
         install
       when :mismatched
+        updated = true
         reinstall
       when :outdated
+        updated = true
         update
       end
       maybe_delete_spec_dir
     end
+    updated
   end
 
   def exist?
diff --git a/spec/unit/action/deploy/module_spec.rb b/spec/unit/action/deploy/module_spec.rb
index 56f96e876..7be2bda67 100644
--- a/spec/unit/action/deploy/module_spec.rb
+++ b/spec/unit/action/deploy/module_spec.rb
@@ -103,21 +103,14 @@
       end
 
       before do
-        @modules = []
         allow(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-          mod = R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)
-          if mod.name == 'first'
+          if environment.name == 'first'
+            expect(environment).to receive(:deploy).and_return(['first'])
             expect(environment).to receive(:generate_types!)
           else
+            expect(environment).to receive(:deploy).and_return([])
             expect(environment).not_to receive(:generate_types!)
           end
-          @modules << mod
-          allow(environment.loader).to receive(:load).and_return({
-            modules: [mod],
-            desired_contents: [],
-            managed_directories: [],
-            purge_exclusions: []
-          }).once
           original.call(environment, &block)
         end
       end
@@ -126,9 +119,8 @@
         expect(subject.settings[:overrides][:environments][:generate_types]).to eq(true)
       end
 
-      it 'only calls puppet generate types on environments with specified module' do
+      it 'only calls puppet generate types on environments where the specified module was updated' do
         subject.call
-        expect(@modules.length).to be(2)
       end
     end
 
@@ -422,16 +414,13 @@
           allow(mock_subprocess).to receive(:logger=)
           expect(mock_subprocess).to receive(:execute)
 
-          mock_mod = double('mock_mod', name: 'mod1')
-
           expect(R10K::Util::Subprocess).to receive(:new).
             with(["/generate/types/wrapper", "first"]).
             and_return(mock_subprocess)
 
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
             if environment.name == 'first'
-              expect(environment).to receive(:deploy).and_return(true)
-              expect(environment).to receive(:modules).and_return([mock_mod])
+              expect(environment).to receive(:deploy).and_return(['first'])
             end
             original.call(environment, &block)
           end.exactly(3).times
@@ -462,12 +451,9 @@
             with(["/generate/types/wrapper", "first third"]).
             and_return(mock_subprocess)
 
-          mock_mod = double('mock_mod', name: 'mod1')
-
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-            expect(environment).to receive(:deploy).and_return(true)
             if ['first', 'third'].include?(environment.name)
-              expect(environment).to receive(:modules).and_return([mock_mod])
+              expect(environment).to receive(:deploy).and_return(['mod1'])
             end
             original.call(environment, &block)
           end.exactly(3).times
@@ -480,9 +466,7 @@
 
           mock_mod2 = double('mock_mod', name: 'mod2')
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-            expect(environment).to receive(:deploy).and_return(true)
-            # Envs have a different module than the one we asked to deploy
-            expect(environment).to receive(:modules).and_return([mock_mod2])
+            expect(environment).to receive(:deploy).and_return([])
             original.call(environment, &block)
           end.exactly(3).times
 
diff --git a/spec/unit/module/forge_spec.rb b/spec/unit/module/forge_spec.rb
index df338fa3d..1183e9fdc 100644
--- a/spec/unit/module/forge_spec.rb
+++ b/spec/unit/module/forge_spec.rb
@@ -212,25 +212,31 @@
       expect(subject).to receive(:install).never
       expect(subject).to receive(:upgrade).never
       expect(subject).to receive(:reinstall).never
-      subject.sync
+      expect(subject.sync).to be false
     end
 
     it 'reinstalls the module when it is mismatched' do
       allow(subject).to receive(:status).and_return :mismatched
       expect(subject).to receive(:reinstall)
-      subject.sync
+      expect(subject.sync).to be true
     end
 
     it 'upgrades the module when it is outdated' do
       allow(subject).to receive(:status).and_return :outdated
       expect(subject).to receive(:upgrade)
-      subject.sync
+      expect(subject.sync).to be true
     end
 
     it 'installs the module when it is absent' do
       allow(subject).to receive(:status).and_return :absent
       expect(subject).to receive(:install)
-      subject.sync
+      expect(subject.sync).to be true
+    end
+
+    it 'returns false if `should_sync?` is false' do
+      # modules do not sync if they are not requested
+      mod = described_class.new('my_org/my_mod', '/path/to/mod', { overrides: { modules: { requested_modules: ['other_mod'] } } })
+      expect(mod.sync).to be false
     end
   end
 
diff --git a/spec/unit/module/git_spec.rb b/spec/unit/module/git_spec.rb
index de417fa84..c30fb1abf 100644
--- a/spec/unit/module/git_spec.rb
+++ b/spec/unit/module/git_spec.rb
@@ -128,6 +128,25 @@
       subject.sync
       expect(Dir.exist?(spec_path)).to eq true
     end
+
+    it 'returns true if repo was updated' do
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mock_repo).to receive(:sync).and_return(true)
+      expect(subject.sync).to be true
+    end
+
+    it 'returns false if repo was not updated (in-sync)' do
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mock_repo).to receive(:sync).and_return(false)
+      expect(subject.sync).to be false
+    end
+
+    it 'returns false if `should_sync?` is false' do
+      # modules do not sync if they are not requested
+      mod = described_class.new(title, dirname, { overrides: { modules: { requested_modules: ['other_mod'] } } })
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mod.sync).to be false
+    end
   end
 
   describe "determining the status" do
diff --git a/spec/unit/module/svn_spec.rb b/spec/unit/module/svn_spec.rb
index b2b9b2550..6263ab03e 100644
--- a/spec/unit/module/svn_spec.rb
+++ b/spec/unit/module/svn_spec.rb
@@ -135,7 +135,6 @@
     subject { described_class.new(title, dirname, {}) }
 
     it 'is kept by default' do
-
       FileUtils.mkdir_p(spec_path)
       expect(subject).to receive(:status).and_return(:absent)
       expect(subject).to receive(:install).and_return(nil)
@@ -157,7 +156,7 @@
 
       it "installs the SVN module" do
         expect(subject).to receive(:install)
-        subject.sync
+        expect(subject.sync).to be true
       end
     end
 
@@ -167,14 +166,14 @@
       it "reinstalls the module" do
         expect(subject).to receive(:reinstall)
 
-        subject.sync
+        expect(subject.sync).to be true
       end
 
       it "removes the existing directory" do
         expect(subject.path).to receive(:rmtree)
         allow(subject).to receive(:install)
 
-        subject.sync
+        expect(subject.sync).to be true
       end
     end
 
@@ -184,7 +183,7 @@
       it "upgrades the repository" do
         expect(subject).to receive(:update)
 
-        subject.sync
+        expect(subject.sync).to be true
       end
     end
 
@@ -196,8 +195,14 @@
         expect(subject).to receive(:reinstall).never
         expect(subject).to receive(:update).never
 
-        subject.sync
+        expect(subject.sync).to be false
       end
     end
+
+    it 'and `should_sync?` is false' do
+      # modules do not sync if they are not requested
+      mod = described_class.new('my_mod', '/path/to/mod', { overrides: { modules: { requested_modules: ['other_mod'] } } })
+      expect(mod.sync).to be false
+    end
   end
 end
diff --git a/spec/unit/puppetfile_spec.rb b/spec/unit/puppetfile_spec.rb
index ea952960f..02389cd49 100644
--- a/spec/unit/puppetfile_spec.rb
+++ b/spec/unit/puppetfile_spec.rb
@@ -273,7 +273,7 @@
       expect(subject).to receive(:modules).and_return([mod1, mod2])
 
       expect(Thread).to receive(:new).exactly(pool_size).and_call_original
-      expect(Queue).to receive(:new).and_call_original
+      expect(Queue).to receive(:new).and_call_original.twice
 
       subject.accept(visitor)
     end