fix Chef-14 chef_fs/chef-zero perf regressionlcg/chef-zero-perf

Fixes a perf bug introduced by #6471 where we were taking O(n^2)
with the number of cookbook versions in the cookbook synch phase.

This is a minimum viable fix that changes the algorithm so that
it does the old, fast method of looking up the CBV first.  Only
if it doesn't find it does it then go searching for cookbooks
that are in a differently named directory but which match by
the name in the metadata.

There may be edge conditions here if people mix-and-match and
have cookbooks which have both named-directory and
name-in-metadata versions.  Please try not to do that.  Don't
cross the streams.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

1 files changed, 27 insertions, 19 deletions

diff --git a/lib/chef/chef_fs/chef_fs_data_store.rb b/lib/chef/chef_fs/chef_fs_data_store.rb
index cdb793f808..e44c2cb8b8 100644
--- a/lib/chef/chef_fs/chef_fs_data_store.rb
+++ b/lib/chef/chef_fs/chef_fs_data_store.rb
@@ -1,6 +1,6 @@
 #
 # Author:: John Keiser (<jkeiser@chef.io>)
-# Copyright:: Copyright 2012-2016, Chef Software Inc.
+# Copyright:: Copyright 2012-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -307,25 +307,14 @@ class Chef
 
         # GET /cookbooks/NAME/VERSION or /cookbook_artifacts/NAME/IDENTIFIER
         elsif %w{cookbooks cookbook_artifacts}.include?(path[0]) && path.length == 3
-          with_entry([path[0]]) do |entry|
+          with_cookbook_manifest(path) do |manifest, entry|
             cookbook_type = path[0]
-            cookbook_entry = entry.children.select do |child|
-              child.chef_object.full_name == "#{path[1]}-#{path[2]}" ||
-                (child.chef_object.name.to_s == path[1] && child.chef_object.identifier == path[2])
-            end[0]
-            raise ChefZero::DataStore::DataNotFoundError.new(path) if cookbook_entry.nil?
-            result = nil
-            begin
-              result = Chef::CookbookManifest.new(cookbook_entry.chef_object, policy_mode: cookbook_type == "cookbook_artifacts").to_hash
-            rescue Chef::ChefFS::FileSystem::NotFoundError => e
-              raise ChefZero::DataStore::DataNotFoundError.new(to_zero_path(e.entry), e)
-            end
 
-            result.each_pair do |key, value|
+            manifest.each_pair do |key, value|
               if value.is_a?(Array)
                 value.each do |file|
                   if file.is_a?(Hash) && file.has_key?("checksum")
-                    relative = ["file_store", "repo", cookbook_type, cookbook_entry.name ]
+                    relative = ["file_store", "repo", cookbook_type, entry.name ]
                     relative += file[:path].split("/")
                     file["url"] = ChefZero::RestBase.build_uri(request.base_uri, relative)
                   end
@@ -334,8 +323,8 @@ class Chef
             end
 
             if cookbook_type == "cookbook_artifacts"
-              result["metadata"] = result["metadata"].to_hash
-              result["metadata"].delete_if do |key, value|
+              manifest["metadata"] = manifest["metadata"].to_hash
+              manifest["metadata"].delete_if do |key, value|
                 value == [] ||
                   (value == {} && !%w{dependencies attributes recipes}.include?(key)) ||
                   (value == "" && %w{source_url issues_url}.include?(key)) ||
@@ -343,9 +332,8 @@ class Chef
               end
             end
 
-            Chef::JSONCompat.to_json_pretty(result)
+            Chef::JSONCompat.to_json_pretty(manifest)
           end
-
         else
           with_entry(path) do |entry|
             begin
@@ -768,6 +756,26 @@ class Chef
         path.length == 1 && BASE_DIRNAMES.include?(path[0])
       end
 
+      def with_cookbook_manifest(path)
+        cookbook_type = path[0]
+        begin
+          # this is fast and equivalent to with_entry() that also returns the cb manifest
+          entry = Chef::ChefFS::FileSystem.resolve_path(chef_fs, to_chef_fs_path(path))
+          yield Chef::CookbookManifest.new(entry.chef_object, policy_mode: cookbook_type == "cookbook_artifacts").to_hash, entry
+        rescue Chef::ChefFS::FileSystem::NotFoundError, ChefZero::DataStore::DataNotFoundError => e
+          # this is very slow and we walk through all the cookbook versions to find ones that have the correct name in the metadata
+          dir = Chef::ChefFS::FileSystem.resolve_path(chef_fs, to_chef_fs_path([path[0]]))
+          entry = dir.children.select do |child|
+            child.chef_object.full_name == "#{path[1]}-#{path[2]}" ||
+              (child.chef_object.name.to_s == path[1] && child.chef_object.identifier == path[2])
+          end[0]
+          raise ChefZero::DataStore::DataNotFoundError.new(path) if entry.nil?
+          yield Chef::CookbookManifest.new(entry.chef_object, policy_mode: cookbook_type == "cookbook_artifacts").to_hash, entry
+        end
+      rescue Chef::ChefFS::FileSystem::NotFoundError => e
+        raise ChefZero::DataStore::DataNotFoundError.new(to_zero_path(e.entry), e)
+      end
+
       def with_entry(path)
         yield Chef::ChefFS::FileSystem.resolve_path(chef_fs, to_chef_fs_path(path))
       rescue Chef::ChefFS::FileSystem::NotFoundError => e