From f6f4bf19b93fad1b56f498ef19a169d0f8c98a6b Mon Sep 17 00:00:00 2001 From: Tim Hinderliter Date: Wed, 9 Mar 2011 15:11:26 -0800 Subject: More work on refactoring of dependency solver: * dependency_solver renamed to cookbook_version_selector * all dep_selector-dependent methods removed from RunList and moved to cookbook_version_selector * error messages handled differently; still have work to do for getting and translating compound messages from DepSelector- to Chef-land * nodes/:node_id/cookbooks refactored to use CookbookVersionSelector#expand_to_cookbook_version * tests written for current exception handling from /environments/:env_id/cookbook_versions --- chef-server-api/Gemfile.lock | 1 - chef-server-api/app/controllers/environments.rb | 11 +- chef-server-api/app/controllers/nodes.rb | 36 ++--- .../spec/unit/environments_controller_spec.rb | 62 ++++++++- .../unit/nodes_controller_environments_spec.rb | 10 +- chef/lib/chef/cookbook_version_selector.rb | 146 +++++++++++++++++++++ chef/lib/chef/run_list.rb | 101 -------------- 7 files changed, 228 insertions(+), 139 deletions(-) create mode 100644 chef/lib/chef/cookbook_version_selector.rb diff --git a/chef-server-api/Gemfile.lock b/chef-server-api/Gemfile.lock index 4a321bd2bc..5c1c42d263 100644 --- a/chef-server-api/Gemfile.lock +++ b/chef-server-api/Gemfile.lock @@ -4,7 +4,6 @@ PATH chef (0.10.0.beta.0) bunny (>= 0.6.0) erubis - extlib highline json (>= 1.4.4, <= 1.4.6) mixlib-authentication (>= 1.1.0) diff --git a/chef-server-api/app/controllers/environments.rb b/chef-server-api/app/controllers/environments.rb index baa7a74276..05f2d3549f 100644 --- a/chef-server-api/app/controllers/environments.rb +++ b/chef-server-api/app/controllers/environments.rb @@ -18,6 +18,7 @@ # require 'chef/environment' +require 'chef/cookbook_version_selector' class Environments < Application @@ -153,10 +154,18 @@ class Environments < Application end # Expand the run list in the scope of the specified environment. - names_to_cookbook_version = run_list.expand_to_cookbook_versions(environment_input, 'couchdb') + names_to_cookbook_version = Chef::CookbookVersionSelector.expand_to_cookbook_versions(run_list, environment_input) rescue Chef::Exceptions::CouchDBNotFound raise NotFound, "Cannot load environment #{params[:environment_id]}" + rescue Chef::Exceptions::CookbookVersionConflict => e + error = { :message => e.message, + #:unsatisfiable_solution_constraint => e.unsatisfiable_solution_constraint, + #:non_existent_cookbooks => e.disabled_non_existent_packages, + #:most_constrained_cookbooks => e.disabled_most_constrained_packages + } + raise PreconditionFailed, error.to_json end + # convert the hash which is # name => CookbookVersion diff --git a/chef-server-api/app/controllers/nodes.rb b/chef-server-api/app/controllers/nodes.rb index f726f9aee0..9147700080 100644 --- a/chef-server-api/app/controllers/nodes.rb +++ b/chef-server-api/app/controllers/nodes.rb @@ -17,9 +17,10 @@ # limitations under the License. # -require 'chef' / 'node' +require 'chef/node' require 'chef/version_class' require 'chef/version_constraint' +require 'chef/cookbook_version_selector' class Nodes < Application @@ -81,6 +82,8 @@ class Nodes < Application display @node end + # Return a hash, cookbook_name => cookbook manifest, of the cookbooks + # appropriate for this node, using its run_list and environment. def cookbooks begin @node = Chef::Node.cdb_load(params[:id]) @@ -88,30 +91,15 @@ class Nodes < Application raise NotFound, "Cannot load node #{params[:id]}" end - display(load_all_files) - end - - private - - def load_all_files - all_cookbooks = Chef::Environment.cdb_load_filtered_cookbook_versions(@node.chef_environment) - - included_cookbooks = cookbooks_for_node(all_cookbooks) - nodes_cookbooks = Hash.new - included_cookbooks.each do |cookbook_name, cookbook| - nodes_cookbooks[cookbook_name.to_s] = cookbook.generate_manifest_with_urls{|opts| absolute_url(:cookbook_file, opts) } - end + # Get the mapping of cookbook_name => CookbookVersion applicable to + # this node's run_list and its environment. + included_cookbooks = Chef::CookbookVersionSelector.expand_to_cookbook_versions(@node.run_list, @node.chef_environment) - nodes_cookbooks - end - - # returns name -> CookbookVersion for all cookbooks included on the given node. - def cookbooks_for_node(all_cookbooks) - begin - @node.constrain_cookbooks(all_cookbooks, 'couchdb') - rescue Chef::Exceptions::CookbookVersionConflict => e - raise PreconditionFailed, e.message - end + # Then map it to the return format. + display(included_cookbooks.inject({}) do |acc, (cookbook_name, cookbook)| + acc[cookbook_name.to_s] = cookbook.generate_manifest_with_urls{|opts| absolute_url(:cookbook_file, opts) } + acc + end) end end diff --git a/chef-server-api/spec/unit/environments_controller_spec.rb b/chef-server-api/spec/unit/environments_controller_spec.rb index 704a33acfd..5b0391192b 100644 --- a/chef-server-api/spec/unit/environments_controller_spec.rb +++ b/chef-server-api/spec/unit/environments_controller_spec.rb @@ -26,17 +26,24 @@ describe "Environments controller" do Merb.logger.set_log(StringIO.new) @env1 = make_environment("env1") + + @filtered_cookbook_list_env1 = make_filtered_cookbook_hash(make_cookbook("cookbook1", "1.0.0"), + make_cookbook("cookbook2", "1.0.0")) + @filtered_cookbook_list_env1["cookbook_noversions"] = Array.new - @filtered_cookbook_list_env1 = - make_filtered_cookbook_hash(make_cookbook("cookbook1", "1.0.0"), - make_cookbook("cookbook2", "1.0.0")) - @filtered_cookbook_list_env2 = - make_filtered_cookbook_hash(make_cookbook("cookbook1", "2.0.0"), - make_cookbook("cookbook2", "2.0.0")) + @filtered_cookbook_list_env2 = make_filtered_cookbook_hash(make_cookbook("cookbook1", "2.0.0"), + make_cookbook("cookbook2", "2.0.0")) + + @cookbook_deps_on_nosuch = make_cookbook("cookbook_deps_on_nosuch", "1.0.0") + @cookbook_deps_on_nosuch.metadata.depends("cookbook_nosuch") + + @cookbook_deps_on_badver = make_cookbook("cookbook_deps_on_badver", "1.0.0") + @cookbook_deps_on_badver.metadata.depends("cookbook1", ">= 3.0.0") end describe "when handling Environments API calls" do - it "should expand the passed-in run_list using the correct environment" do + it "should expand the passed-in run_list using the correct environment: one run_list item" do + # Env1 pins both versions at 1.0.0. Expect only the one we ask for, cookbook1, # back in the result. Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1) @@ -46,7 +53,9 @@ describe "Environments controller" do response["cookbook1"].should_not == nil response["cookbook1"]['version'].should == "1.0.0" response["cookbook1"]['url'].should == "#{root_url}/cookbooks/cookbook1/1.0.0" + end + it "should expect the passed-in run_list using the correct environment: two run_list items" do # Ask for both cookbook1 and cookbook2 back. Expect version 2.0.0 for # each, as those are what's appropriate for the environment. Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env2").and_return(@filtered_cookbook_list_env2) @@ -60,6 +69,45 @@ describe "Environments controller" do response["cookbook2"]['version'].should == "2.0.0" response["cookbook2"]['url'].should == "#{root_url}/cookbooks/cookbook2/2.0.0" end + + it "should report no_such_cookbook if given a dependency on a non-existant cookbook" do + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1) + expected_error = { + "message" => "Run list item (cookbook_nosuch >= 0.0.0) specifies a cookbook that does not exist in the dependency graph", + }.to_json + + lambda { + response = post_json("/environments/env1/cookbook_versions", {"run_list" => ["recipe[cookbook_nosuch]"]}) + }.should raise_error(Merb::ControllerExceptions::PreconditionFailed, expected_error) + end + + it "should report no_such_version if given a dependency on a cookbook that doesn't have any valid versions for an environment" do + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1) + expected_error = { + "message" => "Run list item (cookbook_noversions >= 0.0.0) does not match any versions", + }.to_json + + lambda { + response = post_json("/environments/env1/cookbook_versions", {"run_list" => ["recipe[cookbook_noversions]"]}) + }.should raise_error(Merb::ControllerExceptions::PreconditionFailed, expected_error) + end + + + # TODO; have top-level cookbooks depend on other, non-existent cookbooks, + # to get the other kind of exceptions. + it "should report multiple failures (compound exceptions) if there is more than one error in dependencies" do + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@filtered_cookbook_list_env1) + begin + response = post_json("/environments/env1/cookbook_versions", + {"run_list" => ["recipe[cookbook_nosuch_1]", "recipe[cookbook_nosuch_2]"]}) + rescue => e + puts "e is #{e}" + puts "e.stacktrace =\n #{e.backtrace.join(" \n")}" + + require 'pp' + pp(:e => e) + end + end end end diff --git a/chef-server-api/spec/unit/nodes_controller_environments_spec.rb b/chef-server-api/spec/unit/nodes_controller_environments_spec.rb index 4ce44ada58..ecaa3f71c5 100644 --- a/chef-server-api/spec/unit/nodes_controller_environments_spec.rb +++ b/chef-server-api/spec/unit/nodes_controller_environments_spec.rb @@ -39,8 +39,8 @@ describe "Nodes controller - environments" do @node1.run_list << "role[role1]" @role1 = make_role("role1") - @role1.env_run_lists({"_default" => make_runlist("recipe[cb_for_default]"), - "env1" => make_runlist("recipe[cb_for_env1]")}) + @role1.env_run_lists({"env1" => make_runlist("recipe[cb_for_env1]")}) + @role1.run_list(make_runlist("recipe[cb_for_default]")) @all_filtered_cookbook_list = make_filtered_cookbook_hash(make_cookbook("cb_for_default", "1.0.0"), @@ -52,8 +52,8 @@ describe "Nodes controller - environments" do # Test that node@_default resolves to use cookbook cb_for_default Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1) - Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("_default").and_return(@all_filtered_cookbook_list) Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1) + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("_default").and_return(@all_filtered_cookbook_list) response = get_json("/nodes/node1/cookbooks") response.should be_kind_of(Hash) @@ -65,8 +65,8 @@ describe "Nodes controller - environments" do # Test that node@env1 resolves to use cookbook cb_for_env1 @node1.chef_environment("env1") Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1) - Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@all_filtered_cookbook_list) Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1) + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env1").and_return(@all_filtered_cookbook_list) response = get_json("/nodes/node1/cookbooks") response.should be_kind_of(Hash) @@ -80,8 +80,8 @@ describe "Nodes controller - environments" do # because env_fallback falls back to _default @node1.chef_environment("env_fallback") Chef::Node.should_receive(:cdb_load).with("node1").and_return(@node1) - Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env_fallback").and_return(@all_filtered_cookbook_list) Chef::Role.should_receive(:cdb_load).with("role1", nil).and_return(@role1) + Chef::Environment.should_receive(:cdb_load_filtered_cookbook_versions).with("env_fallback").and_return(@all_filtered_cookbook_list) response = get_json("/nodes/node1/cookbooks") response.should be_kind_of(Hash) diff --git a/chef/lib/chef/cookbook_version_selector.rb b/chef/lib/chef/cookbook_version_selector.rb new file mode 100644 index 0000000000..c59b04c15d --- /dev/null +++ b/chef/lib/chef/cookbook_version_selector.rb @@ -0,0 +1,146 @@ +# +# Author:: Tim Hinderliter () +# Copyright:: Copyright (c) 2011 Opscode, 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. + +require 'dep_selector' + +class Chef + module CookbookVersionSelector + # This method replaces verbiage from DepSelector messages with + # Chef-domain-specific verbiage, such as replacing package with + # cookbook. + # + # TODO [cw, 2011/2/25]: this is a near-term hack. In the long run, + # we'll do this better. + def self.filter_dep_selector_message(message) + m = message + m.gsub!("Package", "Cookbook") + m.gsub!("package", "cookbook") + m.gsub!("Solution constraint", "Run list item") + m.gsub!("solution constraint", "run list item") + m + end + + # all_cookbooks - a hash mapping cookbook names to an array of + # available CookbookVersions. + # + # Creates a DependencyGraph from CookbookVersion objects + def self.create_dependency_graph_from_cookbooks(all_cookbooks) + dep_graph = DepSelector::DependencyGraph.new + + all_cookbooks.each do |cb_name, cb_versions| + cb_versions.each do |cb_version| + cb_version_deps = cb_version.metadata.dependencies + # TODO [cw. 2011/2/10]: CookbookVersion#version returns a + # String even though we're storing as a DepSelector::Version + # object underneath. This should be changed so that we + # return the object and handle proper serialization and + # de-serialization. For now, I'm just going to create a + # Version object from the String representation. + pv = dep_graph.package(cb_name).add_version(DepSelector::Version.new(cb_version.version)) + cb_version_deps.each_pair do |dep_name, constraint_str| + constraint = DepSelector::VersionConstraint.new(constraint_str) + pv.dependencies << DepSelector::Dependency.new(dep_graph.package(dep_name), constraint) + end + end + end + + dep_graph + end + + # Return a hash mapping cookbook names to a CookbookVersion + # object. If there is no solution that satisfies the constraints, + # the first run list item that caused unsatisfiability is + # returned. + # + # This is the final version-resolved list of cookbooks for the + # RunList. + # + # all_cookbooks - a hash mapping cookbook names to an array of + # available CookbookVersions. + # + # recipe_constraints - an array of hashes describing the expanded + # run list. Each element is a hash containing keys :name and + # :version_constraint. The :name component is either the + # fully-qualified recipe name (e.g. "cookbook1::non_default_recipe") + # or just a cookbook name, indicating the default recipe is to be + # run (e.g. "cookbook1"). + def self.constrain(all_cookbooks, recipe_constraints) + dep_graph = create_dependency_graph_from_cookbooks(all_cookbooks) + + # extract cookbook names from (possibly) fully-qualified recipe names + cookbook_constraints = recipe_constraints.map do |recipe_spec| + cookbook_name = (recipe_spec[:name][/^(.+)::/, 1] || recipe_spec[:name]) + DepSelector::SolutionConstraint.new(dep_graph.package(cookbook_name), + recipe_spec[:version_constraint]) + end + + # Pass in the list of all available cookbooks (packages) so that + # DepSelector can distinguish between "no version available for + # cookbook X" and "no such cookbook X" when an environment + # filters out all versions for a given cookbook. + all_packages = all_cookbooks.inject([]) do |acc, (cookbook_name, cookbook_versions)| + acc << dep_graph.package(cookbook_name) + acc + end + + # find a valid assignment of CoookbookVersions. If no valid + # assignment exists, indicate which run_list_item causes the + # unsatisfiability and try to hint at what might be wrong. + soln = + begin + DepSelector::Selector.new(dep_graph).find_solution(cookbook_constraints, all_packages) + rescue DepSelector::Exceptions::NoSolutionExists + puts <