Adding the ability to use rsync to converge cached and installed modules

Need to use the parent of dest for rsync

Reverting gemfile

Adding a test. Going to check with maintainers to see if this is a valid testing strategy.

Adding ctime to the mix to make sure that the file isn't just assigned the same inode

Adding tests for the rsync option using the install command

Only compare ctime if inodes match

Adding test case to make sure update works properly with git

README.md

@@ -256,6 +256,28 @@ Configuration can be set by passing specific options to other commands.
   the environment or global config will be used.
 
 
+## Rsync Option
+
+The default convergence strategy between the cache and the module directory is
+to execute an `rm -r` on the module directory and just `cp -r` from the cache.
+This causes the module to be removed from the module path every time librarian
+puppet updates, regardless of whether the content has changed. This can cause
+some problems in environments with lots of change. The problem arises when the
+module directory gets removed while Puppet is trying to read files inside it.
+The `puppet master` process will lose its CWD and the catalog will fail to
+compile. To avoid this, you can use `rsync` to implement a more conservative
+convergence strategy. This will use `rsync` with the `-avz` and `--delete`
+flags instead of a `rm -r` and `cp -r`. To use this feature, just set the
+`rsync` configuration setting to `true`.
+
+    $ librarian-puppet config rsync true --global
+
+Alternatively, using an environment variable:
+
+    LIBRARIAN_PUPPET_RSYNC='true'
+
+Note that the directories will still be purged if you run librarian-puppet with
+the --clean or --destructive flags.
 
 ## How to Contribute
 

features/install.feature

@@ -40,3 +40,75 @@ Feature: cli/install
     """
     Unable to parse .*/bad_modulefile/Modulefile, ignoring: Missing version
     """
+
+    Scenario: Install a module with the rsync configuration using the --clean flag
+      Given a file named "Puppetfile" with:
+      """
+      forge "http://forge.puppetlabs.com"
+
+      mod 'maestrodev/test'
+      """
+      And a file named ".librarian/puppet/config" with:
+      """
+      ---
+      LIBRARIAN_PUPPET_RSYNC: 'true'
+      """
+      When I run `librarian-puppet config`
+      Then the exit status should be 0
+      And the output should contain "rsync: true"
+      When I run `librarian-puppet install`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should have an inode and ctime
+      When I run `librarian-puppet install --clean`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should not have the same inode or ctime as before
+
+    Scenario: Install a module with the rsync configuration using the --destructive flag
+      Given a file named "Puppetfile" with:
+      """
+      forge "http://forge.puppetlabs.com"
+
+      mod 'maestrodev/test'
+      """
+      And a file named ".librarian/puppet/config" with:
+      """
+      ---
+      LIBRARIAN_PUPPET_RSYNC: 'true'
+      """
+      When I run `librarian-puppet config`
+      Then the exit status should be 0
+      And the output should contain "rsync: true"
+      When I run `librarian-puppet install`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should have an inode and ctime
+      When I run `librarian-puppet install --destructive`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should not have the same inode or ctime as before
+
+    Scenario: Install a module with the rsync configuration
+      Given a file named "Puppetfile" with:
+      """
+      forge "http://forge.puppetlabs.com"
+
+      mod 'maestrodev/test'
+      """
+      And a file named ".librarian/puppet/config" with:
+      """
+      ---
+      LIBRARIAN_PUPPET_RSYNC: 'true'
+      """
+      When I run `librarian-puppet config`
+      Then the exit status should be 0
+      And the output should contain "rsync: true"
+      When I run `librarian-puppet install`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should have an inode and ctime
+      When I run `librarian-puppet install`
+      Then the exit status should be 0
+      And a directory named "modules/test" should exist
+      And the file "modules/test" should have the same inode and ctime as before

features/step_definitions/convergence_steps.rb

@@ -0,0 +1,28 @@
+Then /^the file "([^"]*)" should have an inode and ctime$/ do |file|
+    prep_for_fs_check do
+        stat = File.stat(File.expand_path(file))
+        @before_inode = { 'ino' => stat.ino, 'ctime' => stat.ctime }
+        expect(@before_inode['ino']).not_to eq nil
+        expect(@before_inode['ctime']).not_to eq nil
+    end
+end
+
+Then /^the file "([^"]*)" should have the same inode and ctime as before$/ do |file|
+    prep_for_fs_check do
+        stat = File.stat(File.expand_path(file))
+        expect(stat.ino).to eq @before_inode['ino']
+        expect(stat.ctime).to eq @before_inode['ctime']
+    end
+end
+
+Then /^the file "([^"]*)" should not have the same inode or ctime as before$/ do |file|
+    prep_for_fs_check do
+        stat = File.stat(File.expand_path(file))
+
+        begin
+            expect(stat.ino).not_to eq @before_inode['ino']
+        rescue RSpec::Expectations::ExpectationNotMetError
+            expect(stat.ctime).not_to eq @before_inode['ctime']
+        end
+    end
+end

features/update.feature

@@ -204,3 +204,80 @@ Feature: cli/update
     And the file "Puppetfile.lock" should match /maestrodev.test \(1\.0\.[1-9][0-9]\)/
     And the file "modules/test/Modulefile" should contain "name 'maestrodev-test'"
     And the file "modules/test/Modulefile" should match /version '1\.0\.[1-9][0-9]'/
+
+  Scenario: Updating a forge module with the rsync configuration
+    Given a file named "Puppetfile" with:
+    """
+    forge "http://forge.puppetlabs.com"
+
+    mod 'maestrodev/test'
+    """
+    And a file named "Puppetfile.lock" with:
+    """
+    FORGE
+      remote: http://forge.puppetlabs.com
+      specs:
+        maestrodev/test (1.0.2)
+
+    DEPENDENCIES
+      maestrodev/test (>= 0)
+      """
+    And a file named ".librarian/puppet/config" with:
+    """
+    ---
+    LIBRARIAN_PUPPET_RSYNC: 'true'
+    """
+    When I run `librarian-puppet config`
+    Then the exit status should be 0
+    And the output should contain "rsync: true"
+    When I run `librarian-puppet update --verbose`
+    Then the exit status should be 0
+    And a directory named "modules/test" should exist
+    And the file "modules/test" should have an inode and ctime
+    When I run `librarian-puppet update --verbose`
+    Then the exit status should be 0
+    And a directory named "modules/test" should exist
+    And the file "modules/test" should have the same inode and ctime as before
+
+  Scenario: Updating a git module with the rsync configuration
+    Given a file named "Puppetfile" with:
+    """
+    forge "http://forge.puppetlabs.com"
+
+    mod "stdlib",
+      :git => "https://github.com/puppetlabs/puppetlabs-stdlib.git", :ref => "3.1.x"
+    """
+    And a file named "Puppetfile.lock" with:
+    """
+    GIT
+      remote: https://github.com/puppetlabs/puppetlabs-stdlib.git
+      ref: 3.1.x
+      sha: 614b3fbf6c15893e89ed8654fb85596223b5b7c5
+      specs:
+        stdlib (3.1.1)
+
+    DEPENDENCIES
+      stdlib (>= 0)
+    """
+    And a file named ".librarian/puppet/config" with:
+    """
+    ---
+    LIBRARIAN_PUPPET_RSYNC: 'true'
+    """
+    When I run `librarian-puppet config`
+    Then the exit status should be 0
+    And the output should contain "rsync: true"
+    When I run `librarian-puppet install`
+    Then the exit status should be 0
+    And the file "modules/stdlib/.git/HEAD" should match /614b3fbf6c15893e89ed8654fb85596223b5b7c5/
+    And a directory named "modules/stdlib" should exist
+    When I run `librarian-puppet update`
+    Then the exit status should be 0
+    And a directory named "modules/stdlib" should exist
+    And the file "modules/stdlib" should have an inode and ctime
+    And the file "modules/stdlib/.git/HEAD" should match /a3c600d5f277f0c9d91c98ef67daf7efc9eed3c5/
+    When I run `librarian-puppet update`
+    Then the exit status should be 0
+    And a directory named "modules/stdlib" should exist
+    And the file "modules/stdlib" should have the same inode and ctime as before
+    And the file "modules/stdlib/.git/HEAD" should match /a3c600d5f277f0c9d91c98ef67daf7efc9eed3c5/

lib/librarian/puppet/source/forge/repo.rb

@@ -55,7 +55,7 @@ def install_version!(version, install_path)
 
             cache_version_unpacked! version
 
-            if install_path.exist?
+            if install_path.exist? && rsync? != true
               install_path.rmtree
             end
 

lib/librarian/puppet/source/githubtarball/repo.rb

@@ -48,7 +48,7 @@ def install_version!(version, install_path)
 
             cache_version_unpacked! version
 
-            if install_path.exist?
+            if install_path.exist? && rsync? != true
               install_path.rmtree
             end
 

lib/librarian/puppet/source/local.rb

@@ -20,7 +20,7 @@ def install!(manifest)
           end
 
           install_path = environment.install_path.join(organization_name(name))
-          if install_path.exist?
+          if install_path.exist? && rsync? != true
             debug { "Deleting #{relative_path_to(install_path)}" }
             install_path.rmtree
           end

lib/librarian/puppet/util.rb

@@ -1,3 +1,5 @@
+require 'rsync'
+
 module Librarian
   module Puppet
 
@@ -13,16 +15,26 @@ def warn(*args, &block)
         environment.logger.warn(*args, &block)
       end
 
+      def rsync?
+          environment.config_db.local['rsync'] == 'true'
+      end
+
       # workaround Issue #173 FileUtils.cp_r will fail if there is a symlink that points to a missing file
       # or when the symlink is copied before the target file when preserve is true
       # see also https://tickets.opscode.com/browse/CHEF-833
+      #
+      # If the rsync configuration parameter is set, use rsync instead of FileUtils
       def cp_r(src, dest)
-        begin
-          FileUtils.cp_r(src, dest, :preserve => true)
-        rescue Errno::ENOENT
-          debug { "Failed to copy from #{src} to #{dest} preserving file types, trying again without preserving them" }
-          FileUtils.rm_rf(dest)
-          FileUtils.cp_r(src, dest)
+        if rsync?
+          Rsync.run(File.join(src, "/"), dest, ['-avz', '--delete'])
+        else
+          begin
+            FileUtils.cp_r(src, dest, :preserve => true)
+          rescue Errno::ENOENT
+            debug { "Failed to copy from #{src} to #{dest} preserving file types, trying again without preserving them" }
+            FileUtils.rm_rf(dest)
+            FileUtils.cp_r(src, dest)
+          end
         end
       end
 

librarian-puppet.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |s|
   s.executables = ['librarian-puppet']
 
   s.add_dependency "librarian", ">=0.1.2"
+  s.add_dependency "rsync"
   s.add_dependency "puppet_forge"
 
   s.add_development_dependency "rake"