You can now select the encoding format when transferring the catalog,

with 'yaml' still being the default but 'marshal' being an option.
This is because testing has shown drastic performance differences
between the two, with up to 70% of compile time being spent
in YAML code.  Use the 'catalog_format' setting to choose your format,
and the setting must be set on the client.

Signed-off-by: Luke Kanies <luke@madstop.com>

7 files changed, 144 insertions, 31 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 13edbede9..9004cf619 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,11 @@
 0.24.5
+    You can now select the encoding format when transferring the catalog,
+    with 'yaml' still being the default but 'marshal' being an option.
+    This is because testing has shown drastic performance differences
+    between the two, with up to 70% of compile time being spent
+    in YAML code.  Use the 'catalog_format' setting to choose your format,
+    and the setting must be set on the client.
+
     Fixed #1431 - Provider confines must now specify similar tests in one call.
     I.e., you can't do confine :operatingsystem => %w{a b} and then
     confine :operatingsystem => %w{b c}; you'd need to do them in one command.
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 5f71bb8b7..87ccd62f6 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -416,7 +416,10 @@ module Puppet
         :ca_server => ["$server", "The server to use for certificate
             authority requests.  It's a separate server because it cannot
             and does not need to horizontally scale."],
-        :ca_port => ["$masterport", "The port to use for the certificate authority."]
+        :ca_port => ["$masterport", "The port to use for the certificate authority."],
+        :catalog_format => ["yaml", "What format to use to dump the catalog.  Only supports
+            'marshal' and 'yaml'.  Only matters on the client, since it asks the server
+            for a specific format."]
     )
         
     self.setdefaults(:filebucket,
diff --git a/lib/puppet/network/client/master.rb b/lib/puppet/network/client/master.rb
index 0de3a117b..2da6cf890 100644
--- a/lib/puppet/network/client/master.rb
+++ b/lib/puppet/network/client/master.rb
@@ -143,15 +143,20 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
 
         # If we can't retrieve the catalog, just return, which will either
         # fail, or use the in-memory catalog.
-        unless yaml_objects = get_actual_config(facts)
+        unless marshalled_objects = get_actual_config(facts)
             use_cached_config(true)
             return
         end
 
         begin
-            objects = YAML.load(yaml_objects)
+            case Puppet[:catalog_format]
+            when "marshal": objects = Marshal.load(marshalled_objects)
+            when "yaml": objects = YAML.load(marshalled_objects)
+            else
+                raise "Invalid catalog format '%s'" % Puppet[:catalog_format]
+            end
         rescue => detail
-            msg = "Configuration could not be translated from yaml"
+            msg = "Configuration could not be translated from %s" % Puppet[:catalog_format]
             msg += "; using cached catalog" if use_cached_config(true)
             Puppet.warning msg
             return
@@ -175,7 +180,7 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
         end
 
         if ! @catalog.from_cache
-            self.cache(yaml_objects)
+            self.cache(marshalled_objects)
         end
 
         # Keep the state database up to date.
@@ -442,7 +447,7 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
         benchmark(:debug, "Retrieved catalog") do
             # error handling for this is done in the network client
             begin
-                textobjects = @driver.getconfig(textfacts, "yaml")
+                textobjects = @driver.getconfig(textfacts, Puppet[:catalog_format])
                 begin
                     textobjects = CGI.unescape(textobjects)
                 rescue => detail
diff --git a/lib/puppet/network/handler/master.rb b/lib/puppet/network/handler/master.rb
index a050b089b..9682c460e 100644
--- a/lib/puppet/network/handler/master.rb
+++ b/lib/puppet/network/handler/master.rb
@@ -64,7 +64,14 @@ class Puppet::Network::Handler
 
             catalog = Puppet::Node::Catalog.find(client)
 
-            return translate(catalog.extract)
+            case format
+            when "yaml":
+                return CGI.escape(catalog.extract.to_yaml(:UseBlock => true))
+            when "marshal":
+                return CGI.escape(Marshal.dump(catalog.extract))
+            else
+                raise "Invalid markup format '%s'" % format
+            end
         end
 
         # 
@@ -90,11 +97,6 @@ class Puppet::Network::Handler
 
         # Translate our configuration appropriately for sending back to a client.
         def translate(config)
-            if local?
-                config
-            else
-                CGI.escape(config.to_yaml(:UseBlock => true))
-            end
         end
     end
 end
diff --git a/spec/integration/defaults.rb b/spec/integration/defaults.rb
index c506700c8..54b673a1a 100755
--- a/spec/integration/defaults.rb
+++ b/spec/integration/defaults.rb
@@ -40,4 +40,8 @@ describe "Puppet defaults" do
         Puppet.settings.element(:rundir).owner.should be_nil
         Puppet.settings.element(:rundir).group.should be_nil
     end
+
+    it "should default to yaml as the catalog format" do
+        Puppet.settings[:catalog_format].should == "yaml"
+    end
 end
diff --git a/spec/unit/network/client/master.rb b/spec/unit/network/client/master.rb
index 79ab8c86f..754fd0583 100755
--- a/spec/unit/network/client/master.rb
+++ b/spec/unit/network/client/master.rb
@@ -59,34 +59,92 @@ describe Puppet::Network::Client::Master, " when retrieving the catalog" do
         @client.getconfig
     end
 
-    it "should load the retrieved catalog using YAML" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    describe "when the catalog format is set to yaml" do
+        before do
+            Puppet.settings.stubs(:value).returns "foo"
+            Puppet.settings.stubs(:value).with(:pluginsync).returns false
+            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
+            Puppet.settings.stubs(:value).with(:factsync).returns false
+            Puppet.settings.stubs(:value).with(:catalog_format).returns "yaml"
+        end
 
-        config = mock 'config'
-        YAML.expects(:load).with("myconfig").returns(config)
+        it "should request a yaml-encoded catalog" do
+            @client.stubs(:dostorage)
+            @client.class.stubs(:facts).returns(@facts)
+            @master.expects(:getconfig).with { |*args| args[1] == "yaml" }
 
-        @client.stubs(:setclasses)
+            @client.getconfig
+        end
 
-        config.stubs(:classes)
-        config.stubs(:to_catalog).returns(config)
-        config.stubs(:host_config=)
-        config.stubs(:from_cache).returns(true)
+        it "should load the retrieved catalog using YAML" do
+            @client.stubs(:dostorage)
+            @client.class.stubs(:facts).returns(@facts)
+            @master.stubs(:getconfig).returns("myconfig")
 
-        @client.getconfig
+            config = mock 'config'
+            YAML.expects(:load).with("myconfig").returns(config)
+
+            @client.stubs(:setclasses)
+
+            config.stubs(:classes)
+            config.stubs(:to_catalog).returns(config)
+            config.stubs(:host_config=)
+            config.stubs(:from_cache).returns(true)
+
+            @client.getconfig
+        end
+
+        it "should use the cached catalog if the retrieved catalog cannot be converted from YAML" do
+            @client.stubs(:dostorage)
+            @client.class.stubs(:facts).returns(@facts)
+            @master.stubs(:getconfig).returns("myconfig")
+
+            YAML.expects(:load).with("myconfig").raises(ArgumentError)
+
+            @client.expects(:use_cached_config).with(true)
+
+            @client.getconfig
+        end
     end
 
-    it "should use the cached catalog if the retrieved catalog cannot be converted from YAML" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    describe "from Marshal" do
+        before do
+            Puppet.settings.stubs(:value).returns "foo"
+            Puppet.settings.stubs(:value).with(:pluginsync).returns false
+            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
+            Puppet.settings.stubs(:value).with(:factsync).returns false
+            Puppet.settings.stubs(:value).with(:catalog_format).returns "marshal"
+        end
 
-        YAML.expects(:load).with("myconfig").raises(ArgumentError)
+        it "should load the retrieved catalog using Marshal" do
+            @client.stubs(:dostorage)
+            @client.class.stubs(:facts).returns(@facts)
+            @master.stubs(:getconfig).returns("myconfig")
 
-        @client.expects(:use_cached_config).with(true)
+            config = mock 'config'
+            Marshal.expects(:load).with("myconfig").returns(config)
 
-        @client.getconfig
+            @client.stubs(:setclasses)
+
+            config.stubs(:classes)
+            config.stubs(:to_catalog).returns(config)
+            config.stubs(:host_config=)
+            config.stubs(:from_cache).returns(true)
+
+            @client.getconfig
+        end
+
+        it "should use the cached catalog if the retrieved catalog cannot be converted from Marshal" do
+            @client.stubs(:dostorage)
+            @client.class.stubs(:facts).returns(@facts)
+            @master.stubs(:getconfig).returns("myconfig")
+
+            Marshal.expects(:load).with("myconfig").raises(ArgumentError)
+
+            @client.expects(:use_cached_config).with(true)
+
+            @client.getconfig
+        end
     end
 
     it "should set the classes.txt file with the classes listed in the retrieved catalog" do
diff --git a/test/network/handler/master.rb b/test/network/handler/master.rb
index 448667b73..d342af88c 100755
--- a/test/network/handler/master.rb
+++ b/test/network/handler/master.rb
@@ -56,3 +56,37 @@ class TestMaster < Test::Unit::TestCase
         @master.getconfig("facts", "yaml", "foo.com")
     end
 end
+
+class TestMasterFormats < Test::Unit::TestCase
+    def setup
+        @facts = stub('facts', :save => nil)
+        Puppet::Node::Facts.stubs(:new).returns(@facts)
+
+        @master = Puppet::Network::Handler.master.new(:Code => "")
+        @master.stubs(:decode_facts)
+
+        @catalog = stub 'catalog', :extract => ""
+        Puppet::Node::Catalog.stubs(:find).returns(@catalog)
+    end
+
+    def test_marshal_can_be_used
+        @catalog.expects(:extract).returns "myextract"
+
+        Marshal.expects(:dump).with("myextract").returns "eh"
+
+        @master.getconfig("facts", "marshal", "foo.com")
+    end
+
+    def test_yaml_can_be_used
+        extract = mock 'extract'
+        @catalog.expects(:extract).returns extract
+
+        extract.expects(:to_yaml).returns "myaml"
+
+        @master.getconfig("facts", "yaml", "foo.com")
+    end
+
+    def test_failure_when_non_yaml_or_marshal_is_used
+        assert_raise(RuntimeError) { @master.getconfig("facts", "blah", "foo.com") }
+    end
+end