Merge pull request #3837 from chef/lcg/remote_directory_refactor

refactor remote_directory provider

4 files changed, 244 insertions, 104 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ccefa07544..f66a1400ac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -39,6 +39,7 @@ of partial templates.
 * [**Joel Handwell**](https://github.com/joelhandwell):
   [pr#3821](https://github.com/chef/chef/pull/3821) Human friendly elapsed time in log
 
+* [pr#3837](https://github.com/chef/chef/pull/3837) refactor remote_directory provider for mem+perf improvement
 * [pr#3799](https://github.com/chef/chef/pull/3799) fix supports hash issues in service providers
 * [pr#3817](https://github.com/chef/chef/pull/3817) Remove now-useless forcing of ruby Garbage Collector run
 * [pr#3774](https://github.com/chef/chef/pull/3774) Add support for yum-deprecated in yum provider
diff --git a/lib/chef/deprecation/provider/remote_directory.rb b/lib/chef/deprecation/provider/remote_directory.rb
new file mode 100644
index 0000000000..b55a304696
--- /dev/null
+++ b/lib/chef/deprecation/provider/remote_directory.rb
@@ -0,0 +1,52 @@
+#
+# Author:: Serdar Sutay (<serdar@opscode.com>)
+# Copyright:: Copyright (c) 2013-2015 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+class Chef
+  module Deprecation
+    module Provider
+      module RemoteDirectory
+
+        def directory_root_in_cookbook_cache
+          Chef::Log.deprecation "the Chef::Provider::RemoteDirectory#directory_root_in_cookbook_cache method is deprecated"
+
+          @directory_root_in_cookbook_cache ||=
+            begin
+              cookbook = run_context.cookbook_collection[resource_cookbook]
+              cookbook.preferred_filename_on_disk_location(node, :files, source, path)
+            end
+        end
+
+        # List all excluding . and ..
+        def ls(path)
+          files = Dir.glob(::File.join(Chef::Util::PathHelper.escape_glob(path), '**', '*'),
+                           ::File::FNM_DOTMATCH)
+
+          # Remove current directory and previous directory
+          files = files.reject do |name|
+            basename = Pathname.new(name).basename().to_s
+            ['.', '..'].include?(basename)
+          end
+
+          # Clean all the paths... this is required because of the join
+          files.map {|f| Chef::Util::PathHelper.cleanpath(f)}
+        end
+
+      end
+    end
+  end
+end
diff --git a/lib/chef/provider/remote_directory.rb b/lib/chef/provider/remote_directory.rb
index 85ceb5cdae..56c2ff0caf 100644
--- a/lib/chef/provider/remote_directory.rb
+++ b/lib/chef/provider/remote_directory.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Adam Jacob (<adam@opscode.com>)
-# Copyright:: Copyright (c) 2008 Opscode, Inc.
+# Copyright:: Copyright (c) 2008-2015 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -16,178 +16,266 @@
 # limitations under the License.
 #
 
-require 'chef/provider/file'
 require 'chef/provider/directory'
+require 'chef/resource/file'
 require 'chef/resource/directory'
-require 'chef/resource/remote_file'
+require 'chef/resource/cookbook_file'
 require 'chef/mixin/file_class'
-require 'chef/platform'
-require 'uri'
-require 'tempfile'
-require 'net/https'
-require 'set'
+require 'chef/platform/query_helpers'
 require 'chef/util/path_helper'
+require 'chef/deprecation/warnings'
+require 'chef/deprecation/provider/remote_directory'
+
+require 'forwardable'
 
 class Chef
   class Provider
     class RemoteDirectory < Chef::Provider::Directory
+      extend Forwardable
+      include Chef::Mixin::FileClass
 
       provides :remote_directory
 
-      include Chef::Mixin::FileClass
+      def_delegators :@new_resource, :purge, :path, :source, :cookbook, :cookbook_name
+      def_delegators :@new_resource, :files_rights, :files_mode, :files_group, :files_owner, :files_backup
+      def_delegators :@new_resource, :rights, :mode, :group, :owner
+
+      # The overwrite property on the resource.  Delegates to new_resource but can be mutated.
+      #
+      # @return [Boolean] if we are overwriting
+      #
+      def overwrite?
+        @overwrite = new_resource.overwrite if @overwrite.nil?
+        !!@overwrite
+      end
+
+      attr_accessor :managed_files
+
+      # Hash containing keys of the paths for all the files that we sync, plus all their
+      # parent directories.
+      #
+      # @return [Set] Ruby Set of the files that we manage
+      #
+      def managed_files
+        @managed_files ||= Set.new
+      end
 
+      # Handle action :create.
+      #
       def action_create
         super
-        # Mark all files as needing to be purged
-        files_to_purge = Set.new(ls(@new_resource.path)) # Make sure each path is clean
 
         # Transfer files
         files_to_transfer.each do |cookbook_file_relative_path|
           create_cookbook_file(cookbook_file_relative_path)
-          # parent directories and file being transferred are removed from the purge list
-          Pathname.new(Chef::Util::PathHelper.cleanpath(::File.join(@new_resource.path, cookbook_file_relative_path))).descend do |d|
-            files_to_purge.delete(d.to_s)
-          end
+          # parent directories and file being transferred need to not be removed in the purge
+          add_managed_file(cookbook_file_relative_path)
         end
 
-        purge_unmanaged_files(files_to_purge)
+        purge_unmanaged_files
       end
 
+      # Handle action :create_if_missing.
+      #
       def action_create_if_missing
         # if this action is called, ignore the existing overwrite flag
-        @new_resource.overwrite(false)
+        @overwrite = false
         action_create
       end
 
-      protected
+      private
 
-      # List all excluding . and ..
-      def ls(path)
-        files = Dir.glob(::File.join(Chef::Util::PathHelper.escape_glob(path), '**', '*'),
-                 ::File::FNM_DOTMATCH)
-
-        # Remove current directory and previous directory
-        files = files.reject do |name|
-          basename = Pathname.new(name).basename().to_s
-          ['.', '..'].include?(basename)
+      # Add a file and its parent directories to the managed_files Hash.
+      #
+      # @param [String] cookbook_file_relative_path relative path to the file
+      # @api private
+      #
+      def add_managed_file(cookbook_file_relative_path)
+        if purge
+          Pathname.new(Chef::Util::PathHelper.cleanpath(::File.join(path, cookbook_file_relative_path))).descend do |d|
+            managed_files.add(d.to_s)
+          end
         end
-
-        # Clean all the paths... this is required because of the join
-        files.map {|f| Chef::Util::PathHelper.cleanpath(f)}
       end
 
-      def purge_unmanaged_files(unmanaged_files)
-        if @new_resource.purge
-          unmanaged_files.sort.reverse.each do |f|
-            # file_class comes from Chef::Mixin::FileClass
-            if ::File.directory?(f) && !Chef::Platform.windows? && !file_class.symlink?(f.dup)
-              # Linux treats directory symlinks as files
-              # Remove a directory as a directory when not on windows if it is not a symlink
-              purge_directory(f)
-            elsif ::File.directory?(f) && Chef::Platform.windows?
-              # Windows treats directory symlinks as directories so we delete them here
-              purge_directory(f)
-            else
-              converge_by("delete unmanaged file #{f}") do
-                ::File.delete(f)
-                Chef::Log.debug("#{@new_resource} deleted file #{f}")
+      # Remove all files not in the managed_files Set.
+      #
+      # @api private
+      #
+      def purge_unmanaged_files
+        if purge
+          Dir.glob(::File.join(Chef::Util::PathHelper.escape_glob(path), '**', '*'), ::File::FNM_DOTMATCH).sort!.reverse!.each do |file|
+            # skip '.' and '..'
+            next if ['.','..'].include?(Pathname.new(file).basename().to_s)
+
+            # Clean the path.  This is required because of the ::File.join
+            file = Chef::Util::PathHelper.cleanpath(file)
+
+            # Skip files that we've sync'd and their parent dirs
+            next if managed_files.include?(file)
+
+            if ::File.directory?(file)
+              if !Chef::Platform.windows? && file_class.symlink?(file.dup)
+                # Unix treats dir symlinks as files
+                purge_file(file)
+              else
+                # Unix dirs are dirs, Windows dirs and dir symlinks are dirs
+                purge_directory(file)
               end
+            else
+              purge_file(file)
             end
           end
         end
       end
 
+      # Use a Chef directory sub-resource to remove a directory.
+      #
+      # @param [String] dir The path of the directory to remove
+      # @api private
+      #
       def purge_directory(dir)
-        converge_by("delete unmanaged directory #{dir}") do
-          Dir::rmdir(dir)
-          Chef::Log.debug("#{@new_resource} removed directory #{dir}")
-        end
+        res = Chef::Resource::Directory.new(dir, run_context)
+        res.run_action(:delete)
+        new_resource.updated_by_last_action(true) if res.updated?
       end
 
+      # Use a Chef file sub-resource to remove a file.
+      #
+      # @param [String] file The path of the file to remove
+      # @api private
+      #
+      def purge_file(file)
+        res = Chef::Resource::File.new(file, run_context)
+        res.run_action(:delete)
+        new_resource.updated_by_last_action(true) if res.updated?
+      end
+
+      # Get the files to tranfer.  This returns files in lexicographical sort order.
+      #
+      # FIXME: it should do breadth-first, see CHEF-5080 (please use a performant sort)
+      #
+      # @return Array<String> The list of files to transfer
+      # @api private
+      #
       def files_to_transfer
         cookbook = run_context.cookbook_collection[resource_cookbook]
-        files = cookbook.relative_filenames_in_preferred_directory(node, :files, @new_resource.source)
-        files.sort.reverse
+        files = cookbook.relative_filenames_in_preferred_directory(node, :files, source)
+        files.sort!.reverse!
       end
 
-      def directory_root_in_cookbook_cache
-        @directory_root_in_cookbook_cache ||= begin
-          cookbook = run_context.cookbook_collection[resource_cookbook]
-          cookbook.preferred_filename_on_disk_location(node, :files, @new_resource.source, @new_resource.path)
-        end
+      # Either the explicit cookbook that the user sets on the resource, or the implicit
+      # cookbook_name that the resource was declared in.
+      #
+      # @return [String] Cookbook to get file from.
+      # @api private
+      #
+      def resource_cookbook
+        cookbook || cookbook_name
       end
 
-      # Determine the cookbook to get the file from. If new resource sets an
-      # explicit cookbook, use it, otherwise fall back to the implicit cookbook
-      # i.e., the cookbook the resource was declared in.
-      def resource_cookbook
-        @new_resource.cookbook || @new_resource.cookbook_name
+      # If we are overwriting, then cookbook_file sub-resources should all be action :create,
+      # otherwise they should be :create_if_missing
+      #
+      # @return [Symbol] Action to take on cookbook_file sub-resources
+      # @api private
+      #
+      def action_for_cookbook_file
+        overwrite? ? :create : :create_if_missing
       end
 
+      # This creates and uses a cookbook_file resource to sync a single file from the cookbook.
+      #
+      # @param [String] cookbook_file_relative_path The relative path to the cookbook file
+      # @api private
+      #
       def create_cookbook_file(cookbook_file_relative_path)
-        full_path = ::File.join(@new_resource.path, cookbook_file_relative_path)
+        full_path = ::File.join(path, cookbook_file_relative_path)
 
         ensure_directory_exists(::File.dirname(full_path))
 
-        file_to_fetch = cookbook_file_resource(full_path, cookbook_file_relative_path)
-        if @new_resource.overwrite
-          file_to_fetch.run_action(:create)
-        else
-          file_to_fetch.run_action(:create_if_missing)
-        end
-        @new_resource.updated_by_last_action(true) if file_to_fetch.updated?
+        res = cookbook_file_resource(full_path, cookbook_file_relative_path)
+        res.run_action(action_for_cookbook_file)
+        new_resource.updated_by_last_action(true) if res.updated?
       end
 
+      # This creates the cookbook_file resource for use by create_cookbook_file.
+      #
+      # @param [String] target_path Path on the system to create
+      # @param [String] relative_source_path Relative path in the cookbook to the base source
+      # @return [Chef::Resource::CookbookFile] The built cookbook_file resource
+      # @api private
+      #
       def cookbook_file_resource(target_path, relative_source_path)
-        cookbook_file = Chef::Resource::CookbookFile.new(target_path, run_context)
-        cookbook_file.cookbook_name = @new_resource.cookbook || @new_resource.cookbook_name
-        cookbook_file.source(::File.join(@new_resource.source, relative_source_path))
-        if Chef::Platform.windows? && @new_resource.files_rights
-          @new_resource.files_rights.each_pair do |permission, *args|
-            cookbook_file.rights(permission, *args)
+        res = Chef::Resource::CookbookFile.new(target_path, run_context)
+        res.cookbook_name = resource_cookbook
+        res.source(::File.join(source, relative_source_path))
+        if Chef::Platform.windows? && files_rights
+          files_rights.each_pair do |permission, *args|
+            res.rights(permission, *args)
           end
         end
-        cookbook_file.mode(@new_resource.files_mode)       if @new_resource.files_mode
-        cookbook_file.group(@new_resource.files_group)     if @new_resource.files_group
-        cookbook_file.owner(@new_resource.files_owner)     if @new_resource.files_owner
-        cookbook_file.backup(@new_resource.files_backup)   if @new_resource.files_backup
+        res.mode(files_mode)       if files_mode
+        res.group(files_group)     if files_group
+        res.owner(files_owner)     if files_owner
+        res.backup(files_backup)   if files_backup
 
-        cookbook_file
+        res
       end
 
-      def ensure_directory_exists(path)
-        unless ::File.directory?(path)
-          directory_to_create = resource_for_directory(path)
-          directory_to_create.run_action(:create)
-          @new_resource.updated_by_last_action(true) if directory_to_create.updated?
+      # This creates and uses a directory resource to create a directory if it is needed.
+      #
+      # @param [String] dir The path to the directory to create.
+      # @api private
+      #
+      def ensure_directory_exists(dir)
+        # doing the check here and skipping the resource should be more performant
+        unless ::File.directory?(dir)
+          res = directory_resource(dir)
+          res.run_action(:create)
+          new_resource.updated_by_last_action(true) if res.updated?
         end
       end
 
-      def resource_for_directory(path)
-        dir = Chef::Resource::Directory.new(path, run_context)
-        dir.cookbook_name = @new_resource.cookbook || @new_resource.cookbook_name
-        if Chef::Platform.windows? && @new_resource.rights
+      # This creates the directory resource for ensure_directory_exists.
+      #
+      # @param [String] dir Directory path on the system
+      # @return [Chef::Resource::Directory] The built directory resource
+      # @api private
+      #
+      def directory_resource(dir)
+        res = Chef::Resource::Directory.new(dir, run_context)
+        res.cookbook_name = resource_cookbook
+        if Chef::Platform.windows? && rights
           # rights are only meant to be applied to the toppest-level directory;
           # Windows will handle inheritance.
-          if path == @new_resource.path
-            @new_resource.rights.each do |rights| #rights is a hash
-              permissions = rights.delete(:permissions) #delete will return the value or nil if not found
-              principals = rights.delete(:principals)
-              dir.rights(permissions, principals, rights)
+          if dir == path
+            rights.each do |r|
+              r = r.dup  # do not update the new_resource
+              permissions = r.delete(:permissions)
+              principals = r.delete(:principals)
+              res.rights(permissions, principals, r)
             end
           end
         end
-        dir.mode(@new_resource.mode) if @new_resource.mode
-        dir.group(@new_resource.group)
-        dir.owner(@new_resource.owner)
-        dir.recursive(true)
-        dir
-      end
+        res.mode(mode) if mode
+        res.group(group) if group
+        res.owner(owner) if owner
+        res.recursive(true)
 
-      def whyrun_supported?
-        true
+        res
       end
 
+      #
+      # Add back deprecated methods and aliases that are internally unused and should be removed in Chef-13
+      #
+      extend Chef::Deprecation::Warnings
+      include Chef::Deprecation::Provider::RemoteDirectory
+      add_deprecation_warnings_for(Chef::Deprecation::Provider::RemoteDirectory.instance_methods)
+
+      alias_method :resource_for_directory, :directory_resource
+      add_deprecation_warnings_for([:resource_for_directory])
+
     end
   end
 end
diff --git a/spec/unit/provider/remote_directory_spec.rb b/spec/unit/provider/remote_directory_spec.rb
index 99e2fe285c..6426dafd79 100644
--- a/spec/unit/provider/remote_directory_spec.rb
+++ b/spec/unit/provider/remote_directory_spec.rb
@@ -79,7 +79,7 @@ describe Chef::Provider::RemoteDirectory do
     end
 
     it "configures access control on intermediate directorys" do
-      directory_resource = @provider.send(:resource_for_directory, File.join(Dir.tmpdir, "intermediate_dir"))
+      directory_resource = @provider.send(:directory_resource, File.join(Dir.tmpdir, "intermediate_dir"))
       expect(directory_resource.path).to  eq(File.join(Dir.tmpdir, "intermediate_dir"))
       expect(directory_resource.mode).to  eq("0750")
       expect(directory_resource.group).to eq("wheel")
@@ -219,4 +219,3 @@ describe Chef::Provider::RemoteDirectory do
 
   end
 end
-