Features #2198 - Combine package installation

[Stéphan Gorget]

I am trying to write a patch for the Feature #2198 to combine package
installation.
The patch is working but I wonder if events and errors are correctly
handled and created.
Furthermore the call to the package provider is through a static
function, I am not entirely satisfied because it breaks the object model
(The one to one relation between the provider instance and the package
instance) but I couldn't see an other easy way to install multiple package.
As well I am a bite confused because some logs which were printed by
properties are no longer produced as properties are not used. (the
notice for example)

Spec and unit tests are still missing but I will write them when I will
be satisfied by the patch.

Github branch : http://github.com/phantez/puppet/tree/features/master/2198

--
commit 8627db799ec4567167beb3f902ac6ebb7c8f6e0c
Author: Stéphan Gorget <gor...@ocre.cea.fr>
Date: Tue May 19 13:30:06 2009 +0200

Features #2198 - Combine package installation

Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>

diff --git a/lib/puppet/provider/package/rpm.rb
b/lib/puppet/provider/package/rpm.rb
index 09f78bf..df3275d 100755
--- a/lib/puppet/provider/package/rpm.rb
+++ b/lib/puppet/provider/package/rpm.rb
@@ -130,5 +130,24 @@ Puppet::Type.type(:package).provide :rpm, :source
=> :rpm, :parent => Puppet::Pr
hash[:ensure] = "#{hash[:version]}-#{hash[:release]}"
return hash
end
+
+ # static function that return a hash of the information in rpm -q
and nil if the packet isn't installed
+ def self.is_installed(name)
+ cmd = ["-q", name, "--nosignature", "--nodigest", "--qf",
"#{NEVRAFORMAT}\n"]
+
+ begin
+ output = rpm(*cmd)
+ rescue Puppet::ExecutionFailure
+ return nil
+ end
+
+ hash = self.nevra_to_hash(output)
+
+ if hash and hash != ""
+ return hash
+ else
+ return nil
+ end
+ end
end

diff --git a/lib/puppet/provider/package/yum.rb
b/lib/puppet/provider/package/yum.rb
index 581a446..ec62c38 100755
--- a/lib/puppet/provider/package/yum.rb
+++ b/lib/puppet/provider/package/yum.rb
@@ -104,5 +104,25 @@ Puppet::Type.type(:package).provide :yum, :parent
=> :rpm, :source => :rpm do
def purge
yum "-y", :erase, @resource[:name]
end
- end

+ # Static function that enable multiple package to be install at the
same time
+ # return the name of the packet that haven't been installed
+ def self.combine(resource_names)
+
+ begin
+ output = yum "-d", "0", "-e", "0", "-y", :install,
resource_names
+ rescue Puppet::ExecutionFailure
+ return resource_names
+ end
+
+ resource_failed = resource_names.reject { |name|
+ self.is_installed(name)
+ }
+
+ return resource_failed
+ end
+
+ def self.is_installed(name)
+ super
+ end
+ end
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 37f51b2..d40b2a0 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -219,7 +219,11 @@ class Transaction

# Perform the actual changes
seconds = thinmark do
- events += apply(resource)
+ if combineable?(resource)
+ events += combine(resource)
+ else
+ events += apply(resource)
+ end
end

if children and ! resource.depthfirst?
@@ -473,7 +477,125 @@ class Transaction
end
end
end
+
+ # Test if a specific resource can be combined, only package
resources can be combined so far
+ #
+ # Retrun true if it can be combined false otherwise
+ # * its provider should support combine
+ # * it should not be skiped
+ # * the combine attribute should be set to true
+ # * puppet is not running on noop mode
+ # * the packet is ensure (present, latest or installed), version is
not supported for multiple install yet.
+ def combineable?(resource)
+
+ if resource.provider.class.respond_to?(:combine) and
+ ! resource.noop and
+ ! skip?(resource) and
+ resource[:combine] == :true and
+ [:present,:latest,:installed].include?(resource[:ensure]) and
+ allow_processing?(resource,nil) and
+ return true
+
+ else
+ return false
+ end
+ end
+
+ # Function that install all the package available of the same class
in a single package manager call if it is possible to combine them.
+ def combine(resource)
+
+ provider_class = resource.provider.class
+
+ @combine_events ||= {}
+ @combine_events[provider_class] ||= {}
+
+ if events = @combine_events[provider_class][resource.name]
+ return events
+ end
+
+ # Find all resources matching the provider class that set combining
+ resources = catalog.vertices.reject { |r|
+
+ !r.provider.class.equal?(provider_class) or !combineable?(r)
+
+ # All the resource that will be installed
+ }.collect { |r|
+
+ begin
+ changes = r.evaluate
+ rescue => detail
+ if Puppet[:trace]
+ puts detail.backtrace
+ end
+
+ r.err "Failed to retrieve current state of resource:
%s" % detail
+
+ # Mark that it failed
+ @failures[r] += 1
+
+ # And then return
+ changes = []
+ end
+
+ changes = [changes] unless changes.is_a?(Array)
+
+ if changes.length > 0
+ @resourcemetrics[:out_of_sync] += 1
+ end
+
+ changes.each { |c|
+ @changes << c
+ @count += 1
+ }
+
+ resourceevents = [Puppet::Transaction::Event.new(r.name, r)]
+
+ # Record when we last synced
+ r.cache(:synced, Time.now)

+ # Flush, if appropriate
+ if r.respond_to?(:flush)
+ r.flush
+ end
+
+ # And set a trigger for refreshing this resource if it's a
+ # self-refresher
+ if r.self_refresh? and ! r.deleting?
+ # Create an edge with this resource as both the source and
+ # target. The triggering method treats these specially for
+ # logging.
+ events = resourceevents.collect { |e| e.name }
+ set_trigger(Puppet::Relationship.new(r,r, :callback =>
:refresh, :event => events))
+ end
+
+ @combine_events[provider_class][r.name] = resourceevents
+
+ r
+ }
+
+ # collecting their names in an array
+ resource_names = resources.collect { |r| r[:name] }
+
+ # Add error handling ...
+ if resource_names.length > 0
+ begin
+ output = provider_class.combine(resource_names)
+
+ if output.nitems > 0
+ resource.err "Could not install the following
package: %s" % output
+ end
+
+ rescue => detail
+ if Puppet[:trace]
+ puts detail.backtrace
+ end
+ resource.err "Combine error: %s" % detail
+ end
+ end
+
+ @combine_events[provider_class][resource.name]
+ end
+
# Prepare to evaluate the resources in a transaction.
def prepare
# Now add any dynamically generated resources
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index 655f9e0..2c507ed 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -277,6 +277,14 @@ module Puppet
newvalues(:true, :false)
end

+ newparam(:combine) do
+ desc "Tells package provider if the packet installation
have to be
+ perform in a single call by the packet manager or if it
has to
+ be executed alone."
+ defaultto :false
+ newvalues(:true, :false)
+ end
+
autorequire(:file) do
autos = []
[:responsefile, :adminfile].each { |param|

--
Stéphan Gorget

[Luke Kanies]

In most of this patch, s/packet/package/g.

This method shouldn't be necessary -- see below.

And if it does stick around, shouldn't it just be a boolean method?

I'd prefer the method be named something like 'evaluate_multiple', to
keep terminology consistent with (admittedly poor) existing method
names.

There's some ugly crossing of boundaries between transaction, type,
and provider here - I'd really prefer the method name be automatically
determined by the value of 'ensure' (e.g., 'install_multiple'), but
that might be pushing it for this first pass.

>
> + begin
> + output = yum "-d", "0", "-e", "0", "-y", :install,
> resource_names
> + rescue Puppet::ExecutionFailure
> + return resource_names
> + end
> +
> + resource_failed = resource_names.reject { |name|
> + self.is_installed(name)
> + }

Oh my god. That's right - yum is too stupid to tell you when there's
a failure.

It'd be much better if we can do something like use the rpm prefetch
method here -- it already knows how to retrieve information about
multiple resources. This would require changing your method profile
to expect resources rather than resource names, which I think is a
good idea.

You basically need to do something like:

prefetch
install_multiple
prefetch

This is three total calls, which is not preferred but isn't really
avoidable. Then you can just do 'resource.provider.installed?' or
whatever the method is on the provider. Again, this is too much
crossing of class boundaries for my tastes, but I'm not sure how much
choice we have without a lot of redesign.

Doesn't the eval_resource method already take care of skipping
resources if necessary? And this includes checking for noop.

>
> + resource[:combine] == :true and

If you define the 'combine' parameter with ':boolean => true', then
you can just do 'resource.combine?'. Really, I'd prefer you just
define the method in Puppet::Type, and have it do most of these
checks. At the least, it can check the provider and the value
of :combine.

>
> + [:present,:latest,:installed].include?
> (resource[:ensure]) and

This is definitely a bad idea - having the transaction know anything
at all about packages is bad. I'd prefer you override that 'combine?'
method in the package type to return false if 'ensure' is set to
something other than these values.

>
> + allow_processing?(resource,nil) and
> + return true
> +
> + else
> + return false
> + end
> + end
> +
> + # Function that install all the package available of the same
> class
> in a single package manager call if it is possible to combine them.
> + def combine(resource)

I probably picked this method name, but something longer and more
descriptive would probably be a good idea.

>
> + provider_class = resource.provider.class
> +
> + @combine_events ||= {}
> + @combine_events[provider_class] ||= {}
> +
> + if events = @combine_events[provider_class][resource.name]
> + return events
> + end
> +
> + # Find all resources matching the provider class that set
> combining
> + resources = catalog.vertices.reject { |r|
> +
> + !r.provider.class.equal?(provider_class) or !
> combineable?(r)

This would read a bit better as:

resources = catalog.vertices.find_all { |r| r.provider.class.equal?
(provider_class) and r.combine? }

This is, switch the collection to be find_all rather than reject, and
get rid of the negations by s/or/and/.

>
> + # All the resource that will be installed
> + }.collect { |r|
> +
> + begin
> + changes = r.evaluate
> + rescue => detail
> + if Puppet[:trace]
> + puts detail.backtrace
> + end
> +
> + r.err "Failed to retrieve current state of resource:
> %s" % detail
> +
> + # Mark that it failed
> + @failures[r] += 1
> +
> + # And then return
> + changes = []

What's going on here? You shouldn't need to call evaluate at all,
AFAICT - the provider should support prefetch (and it should probably
be a failure if someone tries to provide 'combine' support on a
provider without prefetch), so you shouldn't need to call 'evaluate'
on the resource at all.

Most of this code is a bit confusing to me, and definitely belongs in
a separate method. I know that the Transaction class needs lots of
extract-method refactoring, but that's no excuse to allow further
proliferation of long methods with multiple roles.

> + r
> + }
> +
> + # collecting their names in an array
> + resource_names = resources.collect { |r| r[:name] }
> +
> + # Add error handling ...

What's this comment about?

>
> + if resource_names.length > 0
> + begin
> + output = provider_class.combine(resource_names)
> +
> + if output.nitems > 0
> + resource.err "Could not install the following
> package: %s" % output
> + end
> +
> + rescue => detail
> + if Puppet[:trace]
> + puts detail.backtrace
> + end
> + resource.err "Combine error: %s" % detail
> + end
> + end

Again this should be a separate method.

It looks like you should have one main entry-point method here
(evaluate_multiple_resources or whatever), and it should call out to
three separate methods. Something like:

def evaluate_multiple_resource(resource)
if events = retrieve_combined_resource_events(resource)
return events
end
resources = find_combineable_resources(resource)
events = evaluate_multiple_resources(resource) # note the
multiple here
set_triggers_from_resource(resource, events)
end

Hopefully each method name is sufficiently descriptive.

>
> + @combine_events[provider_class][resource.name]
> + end
> +
> # Prepare to evaluate the resources in a transaction.
> def prepare
> # Now add any dynamically generated resources
> diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
> index 655f9e0..2c507ed 100644
> --- a/lib/puppet/type/package.rb
> +++ b/lib/puppet/type/package.rb
> @@ -277,6 +277,14 @@ module Puppet
> newvalues(:true, :false)
> end
>
> + newparam(:combine) do
> + desc "Tells package provider if the packet installation
> have to be
> + perform in a single call by the packet manager or
> if it
> has to
> + be executed alone."
> + defaultto :false
> + newvalues(:true, :false)
> + end

Hmm. On the one hand, this should really be a metaparameter, since
any resource type should support it, but on the other hand, it should
require feature support (e.g., see the other provider features defined
in package.rb), and we don't have support for metafeatures.

For now, I'd recommend just keeping this in package.rb, but open a
ticket for creating metafeatures (that is, features that can affect
all resource types). Then define a feature in package.rb that defines
whether a given provider has 'combineable'; something like:

feature :combineable, "Multiple packages can be installed or
uninstalled in one command, rather than requiring one command per
package."

At this point, features can't specify that they need a class method,
so your providers would need to declare 'has_feature :combineable'.

Then your parameter can be changed to:

newparam(:combine, :required_features => :combineable) do ... end

This gets you automatic validation. And there's no reason to have
this feature default to 'false' - just make sure your 'combine?'
method treats 'nil' as equivalent to 'false'.

>
> autorequire(:file) do
> autos = []
> [:responsefile, :adminfile].each { |param|

--
Charm is a way of getting the answer yes without asking a clear
question. -- Albert Camus
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Stephan Gorget]

Luke Kanies a écrit :

I rewrote the patch to match what you said, but package installation
doesn't respect ordering now because I no longer use
resource.evaluate(), I haven't found a function that determines if the
dependency graph is satisfied, the allow_processing?() function, which
based on the name seems to be the right one to use, doesn't do that.

To make the patch work correctly I need a way to know if a resource have
its dependencies satisfied and if a package has to be installed.

diff --git a/lib/puppet/provider/package/yum.rb
b/lib/puppet/provider/package/yum.rb
index 581a446..e0da3d7 100755
--- a/lib/puppet/provider/package/yum.rb
+++ b/lib/puppet/provider/package/yum.rb
@@ -2,6 +2,7 @@ Puppet::Type.type(:package).provide :yum, :parent =>

:rpm, :source => :rpm do

desc "Support via ``yum``."

has_feature :versionable
+ has_feature :combineable

commands :yum => "yum", :rpm => "rpm", :python => "python"

@@ -104,5 +105,26 @@ Puppet::Type.type(:package).provide :yum, :parent

=> :rpm, :source => :rpm do
def purge
yum "-y", :erase, @resource[:name]
end
- end

+ # Static function that enables multiple package to be install at
the same time
+ # it returns the resources that haven't been installed correctly
+ def self.install_multiple(resources)
+
+ # collect the names of all the resources that need to be installed
+ resources_not_installed = resources.reject{ |r| r.provider.query }
+ resources_not_installed_name = resources_not_installed.collect{
|r| r[:name]}
+
+ begin
+ if resources_not_installed.empty?
+ return []
+ else

+ output = yum "-d", "0", "-e", "0", "-y", :install,

resources_not_installed_name
+ end
+ rescue Puppet::ExecutionFailure
+ return resources
+ end
+
+ # collect the listed resources that are not installed
+ resources_failed = resources_not_installed.reject { |r|
r.provider.query }

+ end
+ end
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb

index dd86894..1ba904b 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -246,7 +246,11 @@ class Transaction

# Perform the actual changes
seconds = thinmark do
- events += apply(resource)

+ if resource.combine?
+ events += evaluate_multiple_resource(resource)

+ else
+ events += apply(resource)
+ end
end

if ! children.empty? and ! resource.depthfirst?
@@ -481,7 +485,100 @@ class Transaction
end
end
end
-
+
+ # Function that evaluate multiple resource at the same class if
they support it.
+ def evaluate_multiple_resource(resource)
+

+ provider_class = resource.provider.class
+

+ if events = retrieve_combined_resource_events(resource)

+ return events
+ end
+

+ resources = find_combineable_resources(resource)
+ events = evaluate_multiple_resources(resources, provider_class)
+ set_triggers_from_resource(resources, provider_class, events)
+

+ @combine_events[provider_class][resource.name]
+ end
+

+ # retrieves events associated to a specific resource if they exist
otherwise return nil
+ def retrieve_combined_resource_events(resource)
+ @combine_events ||= {}
+ @combine_events[resource.provider.class] ||= {}
+
+ if events = @combine_events[resource.provider.class][resource.name]
+ return events

+ else
+ return nil
+ end
+ end

+
+ # Find all resources matching the provider class that set combine
+ def find_combineable_resources(resource)
+ resources = catalog.vertices.find_all { |r|
+ r.provider.class.equal?(resource.provider.class) and r.combine?
+ }
+ end
+
+ # it performs the multiple evaluation
+ def evaluate_multiple_resources(resources, provider_class)
+ # Uses the first resource to define noop or not and print logs
+ reference_resource = resources.first
+ if resources.length > 0
+ begin
+ if !reference_resource.noop
+ output = provider_class.install_multiple(resources)
+ end
+ # Define event for each resources
+ resources.each { |r|
+ if output.include?(r)
+ # Mark that it has failed
+ @failures[r] +=1

+ @combine_events[provider_class][r.name] =

[Puppet::Transaction::Event.new(:noop, r)]
+ reference_resource.err "Could not install the
following package: %s" % r.name
+ else
+ # Generate event
+ if !r.noop

+ @combine_events[provider_class][r.name] =

[Puppet::Transaction::Event.new(r.name, r)]
+ r.notice "Package %s have been installed
with the combine method" % r.name
+ @count += 1
+ else

+ @combine_events[provider_class][r.name] =

[Puppet::Transaction::Event.new(:noop, r)]
+ r.notice "Would have installed %s" % r.name

+ end
+ end
+ }

+ rescue => detail
+ reference_resource.err "evaluate multiple resources
error: %s" % detail

+ if Puppet[:trace]
+ puts detail.backtrace
+ end

+ # All the resources failed
+ resources.each { |r|

+ @failures[r] += 1
+ }

+ return nil
+ end
+ end

+ end
+
+ def set_triggers_from_resource(resources, provider_class,
resource_events)
+ resources.each { |r|
+ # The following piece of code have been copied from the
apply function
+ #

+ # And set a trigger for refreshing this resource if it's a
+ # self-refresher

+ if r.self_refresh? and !r.deleting? and !r.noop

+ # Create an edge with this resource as both the source and
+ # target. The triggering method treats these specially for
+ # logging.

+ events = resource_events.collect { |e| e.name }
+ set_trigger(Puppet::Relationship.new(r, r, :callback =>

:refresh, :event => events))
+ end
+ }

+ end
+
# Prepare to evaluate the resources in a transaction.
def prepare
# Now add any dynamically generated resources

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 99ac909..42583e1 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1647,6 +1647,15 @@ class Type
end
end

+ # returns true if the resource can be combined
+ def combine?
+ if provider.class.respond_to?(:install_multiple) and
resource[:combine] == :true
+ return true

+ else
+ return false
+ end
+ end
+

###############################
# All of the relationship code.

diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index 655f9e0..7f1fda0 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -35,6 +35,9 @@ module Puppet
package database for installed version(s), and can select
which out of a set of available versions of a package to
install if asked."
+ feature :combineable, "Multiple packages can be installed or
+ uninstalled in one command, rather than requiring one
command per
+ package."

ensurable do
desc "What state the package should be in.
@@ -248,7 +251,6 @@ module Puppet
newparam(:configfiles) do
desc "Whether configfiles should be kept or replaced. Most
packages
types do not support this parameter."
-
defaultto :keep

newvalues(:keep, :replace)
@@ -277,6 +279,22 @@ module Puppet
newvalues(:true, :false)
end

+ newparam(:combine, :boolean => true, :required_features =>
:combineable) do
+ desc "Tells package provider if the package installation
have to be
+ perform in a single call by the package manager or if
it has to
+ be executed separetly."
+
+ defaultto {
+ if provider.class.respond_to?(:install_multiple) and

[:present,:latest,:installed].include?(resource[:ensure])

+ :true
+ else
+ :false
+ end
+ }
+

+ newvalues(:true, :false)
+ end

+

[Luke Kanies]

On May 29, 2009, at 8:06 AM, Stephan Gorget wrote:
>
> I rewrote the patch to match what you said, but package installation
> doesn't respect ordering now because I no longer use
> resource.evaluate(), I haven't found a function that determines if the
> dependency graph is satisfied, the allow_processing?() function, which
> based on the name seems to be the right one to use, doesn't do that.

Ordering shouldn't really matter here -- packages will only ever be
installed too early, rather than too late.

>
> To make the patch work correctly I need a way to know if a resource
> have
> its dependencies satisfied and if a package has to be installed.

What do you mean?

This could just be 'return @combine_events[resource.provider.class]
[resource.name]', because it's either a value or nil.

>
> + end
> +
> + # Find all resources matching the provider class that set combine
> + def find_combineable_resources(resource)
> + resources = catalog.vertices.find_all { |r|
> + r.provider.class.equal?(resource.provider.class) and
> r.combine?
> + }

The tmp variable is unnecessary here; s/resources = //.

>
> + end
> +
> + # it performs the multiple evaluation
> + def evaluate_multiple_resources(resources, provider_class)
> + # Uses the first resource to define noop or not and print
> logs
> + reference_resource = resources.first
> + if resources.length > 0
> + begin
> + if !reference_resource.noop
> + output =
> provider_class.install_multiple(resources)
> + end

This isn't really sufficient. You need to check whether a resource
should be skipped by using the transaction's 'skip?' method:

resources.reject! { |r| skip?(r) }

This does bring up one case where dependencies could be problematic -
normally Puppet will refuse to do anything with a resource whose
dependencies have failed, but in this case you could. But it
shouldn't matter much for packages, I think.

>
> + # Define event for each resources
> + resources.each { |r|
> + if output.include?(r)
> + # Mark that it has failed
> + @failures[r] +=1
> + @combine_events[provider_class][r.name] =
> [Puppet::Transaction::Event.new(:noop, r)]
> + reference_resource.err "Could not install the
> following package: %s" % r.name

How do you know that the resource has failed from the output?
Shouldn't the provider handle whether something has failed or not?

Existing behaviour throws an exception on failure, and the transaction
handles that exception. IMO, if you're doing combined resource
evaluation, then either all fail or all succeed, and you log the
output. It's always going to be a bit non-deterministic, but
installing multiple packages at once is inherently non-deterministic.

See, now you have to handle the failure case twice - once in the case
of bad output, and once in the case of an exception. Throwing an
exception on failure fixes that.

>
> + end
> + end
> + end
> +
> + def set_triggers_from_resource(resources, provider_class,
> resource_events)
> + resources.each { |r|
> + # The following piece of code have been copied from the
> apply function

If it's duplicate code, you should extract it into a separate method.

This can be reduced to:

def combine?

provider.class.respond_to?(:install_multiple) and
resource[:combine] == :true

end

:)

This should have no default value. No way this should default to true.

>
> + newvalues(:true, :false)
> + end
> +
> autorequire(:file) do
> autos = []
> [:responsefile, :adminfile].each { |param|

--
SELF-EVIDENT, adj. Evident to one's self and to nobody else.
-- Ambrose Bierce

[Stephan Gorget]

Luke Kanies a écrit :

> On May 29, 2009, at 8:06 AM, Stephan Gorget wrote:
>> I rewrote the patch to match what you said, but package installation
>> doesn't respect ordering now because I no longer use
>> resource.evaluate(), I haven't found a function that determines if the
>> dependency graph is satisfied, the allow_processing?() function, which
>> based on the name seems to be the right one to use, doesn't do that.
>
> Ordering shouldn't really matter here -- packages will only ever be
> installed too early, rather than too late.

From my point of view ordering matters, you can have a require for a
specific package, for example package1 requires file1 and file1 requires
package2, so there is no way you could combine the installation of
package1 and package2 without breaking the dependency graph.

I'm not parsing the output, I use several call to rpm -q (with
rpm.query) to test which packages have been installed, because if you
specify packages that exist and packages that don't exist to yum, it is
going to install the ones that exist. And testing which package have
been installed makes the problem be deterministic.

>
> Existing behaviour throws an exception on failure, and the transaction
> handles that exception. IMO, if you're doing combined resource
> evaluation, then either all fail or all succeed, and you log the
> output. It's always going to be a bit non-deterministic, but
> installing multiple packages at once is inherently non-deterministic.
>
>> + else
>> + # Generate event
>> + if !r.noop
>> + @combine_events[provider_class][r.name] =
>> [Puppet::Transaction::Event.new(r.name, r)]
>> + r.notice "Package %s have been installed
>> with the combine method" % r.name
>> + @count += 1
>> + else
>> + @combine_events[provider_class][r.name] =
>> [Puppet::Transaction::Event.new(:noop, r)]
>> + r.notice "Would have installed %s" %
>> r.name

What kind of event should be created when a resource is already in a
correct state ?

Should this feature enable by default if the provider supports it ? if
yes how should it be done to map nil to :true without using defaultto ?

[Luke Kanies]

On Jun 5, 2009, at 7:53 AM, Stephan Gorget wrote:

>
> Luke Kanies a écrit :
>> On May 29, 2009, at 8:06 AM, Stephan Gorget wrote:
>>> I rewrote the patch to match what you said, but package installation
>>> doesn't respect ordering now because I no longer use
>>> resource.evaluate(), I haven't found a function that determines if
>>> the
>>> dependency graph is satisfied, the allow_processing?() function,
>>> which
>>> based on the name seems to be the right one to use, doesn't do that.
>>
>> Ordering shouldn't really matter here -- packages will only ever be
>> installed too early, rather than too late.
> From my point of view ordering matters, you can have a require for a
> specific package, for example package1 requires file1 and file1
> requires
> package2, so there is no way you could combine the installation of
> package1 and package2 without breaking the dependency graph.

The point, though, is that you will very rarely find a case where
installing a package too soon is much of a problem. Compare this to,
say, services, where attempting to start a service before its
dependencies have been installed is a real problem.

With packages (at least with yum and apt), any dependencies of the
package itself will be automatically installed, too, so it's no problem.

Really, I think the only resource types that could even be combineable
are those that don't have significant ordering concerns.

Ok. How much does that cost, relative to the value of the 'combine'
call?

It's probably worth using provider.prefetch to fill in all of that
data in one go, rather than doing individual calls to rpm.

>> Existing behaviour throws an exception on failure, and the
>> transaction
>> handles that exception. IMO, if you're doing combined resource
>> evaluation, then either all fail or all succeed, and you log the
>> output. It's always going to be a bit non-deterministic, but
>> installing multiple packages at once is inherently non-deterministic.
>>
>>> + else
>>> + # Generate event
>>> + if !r.noop
>>> + @combine_events[provider_class]
>>> [r.name] =
>>> [Puppet::Transaction::Event.new(r.name, r)]
>>> + r.notice "Package %s have been
>>> installed
>>> with the combine method" % r.name
>>> + @count += 1
>>> + else
>>> + @combine_events[provider_class]
>>> [r.name] =
>>> [Puppet::Transaction::Event.new(:noop, r)]
>>> + r.notice "Would have installed %s" %
>>> r.name
> What kind of event should be created when a resource is already in a
> correct state ?

None at all.

Now that I think of it, though, the combineable code should skip
packages that are already in sync.

I know yum supports prefetch, so you should be able to relatively
easily skip the packages that are already fine. The current code just
includes all combineable packages, but if it rejected the ones already
in sync then you would only need to worry about success or failure.

In looking in transaction.rb, maybe the apply_changes method is the
right place to hook in, rather than eval_resource().

It should not be enabled by default; there should be no 'defaultto'
block at all.

>
>>
>>> + newvalues(:true, :false)
>>> + end
>>> +
>>> autorequire(:file) do
>>> autos = []
>>> [:responsefile, :adminfile].each { |param|
>>
>>
>
>
> >

--
Barondes' First Law:
Science abhors contradictions; scientists' minds are replete with
them.

[David Schmitt]

Luke Kanies schrieb:

I don't know how much code/effort it would be, but when the dependency
graph is toposorted, it is (mathematically) possible to find groups of
packages with non-conflicting dependencies. Essentially by going through
all packages and selecting those which are only connected by direct
dependencies (or not connected at all). These groups can then be
installed together without violating the configuration's constraints.

Implementing the grouping in the dependency graph could make it easier
for all types/providers to provide grouping functionality. Most
providers having a flush method (e.g. parsedfile) probably could profit
from this.

Regards, DavidS

--
dasz.at OG Tel: +43 (0)664 2602670 Web: http://dasz.at
Klosterneuburg UID: ATU64260999

FB-Nr.: FN 309285 g FB-Gericht: LG Korneuburg

[Luke Kanies]

Yeah, I know this is a possibility, and would be required for more
complicated types.

For the simple types like package, I don't think it's worth it, but
once you extend, yeah, you need to think about how to use the graph to
solve some of these problems. I do expect it's a bit complicated,
though.

--
Millions long for immortality who do not know what to do with
themselves on a rainy Sunday afternoon. -- Susan Ertz

[Stephan Gorget]

Luke Kanies a écrit :

I modified the patch so that it doesn't try to install non-synced
resource and it takes into account the dependency tree. This is based on
the fact that a resource can only been installed if all its dependencies
have the cached value :check setted. As I have to use
resource.evaluate() to do so I do not invoke query() (rpm -q) before
installing. It's only invoked after the yum call (it is what is done
without the patch), it's perfectible but it's not taking a long time.

commit b6979292adb63a1f7a94eb15f4b218d96c7dc296
Author: Stéphan Gorget <gor...@ocre.cea.fr>
Date: Mon Jun 8 13:26:53 2009 +0200

Features #2198 - Combine package installation

Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>

diff --git a/lib/puppet/provider/package/yum.rb
b/lib/puppet/provider/package/yum.rb
index 6fdff69..52eec37 100755

--- a/lib/puppet/provider/package/yum.rb
+++ b/lib/puppet/provider/package/yum.rb
@@ -2,6 +2,7 @@ Puppet::Type.type(:package).provide :yum, :parent =>
:rpm, :source => :rpm do
desc "Support via ``yum``."

has_feature :versionable
+ has_feature :combineable

commands :yum => "yum", :rpm => "rpm", :python => "python"

@@ -104,5 +105,23 @@ Puppet::Type.type(:package).provide :yum, :parent

=> :rpm, :source => :rpm do
def purge
yum "-y", :erase, @resource[:name]
end
- end

+ # Static function that enables multiple package to be install at
the same time
+ # it returns the resources that haven't been installed correctly
+ def self.install_multiple(resources)
+

+ # check which resource is not installed
+ begin

+ # collect the names of all the resources that need to be
installed

+ resources_name = resources.collect{ |r| r[:name]}
+
+ # install package

+ output = yum "-d", "0", "-e", "0", "-y", :install,

resources_name
+
+ # resources that have not been installed
+ resources.reject { |r| r.provider.query }
+ rescue Puppet::ExecutionFailure
+ raise Puppet::Error, "Failed to install packages"

+ end
+ end
+ end

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index f09ca80..fb5664e 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -43,6 +43,16 @@ class Transaction
return true
end

+ # check if all the dependencies have been checked
+ def dependencies_completed?(resource)
+ deps = relationship_graph.dependencies(resource)
+ deps.each { |r|
+ # return false if a dependency have not been checked
+ return false if r.cached(:checked).to_i
+ }
+ return true
+ end
+
# Are there any failed resources in this transaction?
def any_failed?
failures = @failures.inject(0) { |failures, array| failures +=
array[1]; failures }
@@ -92,18 +102,22 @@ class Transaction
resource.flush
end

- # And set a trigger for refreshing this resource if it's a
- # self-refresher
- if resource.self_refresh? and ! resource.deleting?
- # Create an edge with this resource as both the source and
- # target. The triggering method treats these specially for
- # logging.
- events = resourceevents.collect { |e| e.name }
- set_trigger(Puppet::Relationship.new(resource,
resource, :callback => :refresh, :event => events))
- end
end

- resourceevents
+ resourceevents = set_trigger_and_refresh(resource, resourceevents)
+ end
+
+ def set_trigger_and_refresh(resource, resourceevents)

+ # And set a trigger for refreshing this resource if it's a
+ # self-refresher

+ if resource.self_refresh? and ! resource.deleting?

+ # Create an edge with this resource as both the source and
+ # target. The triggering method treats these specially for
+ # logging.

+ events = resourceevents.collect { |e| e.name }

+ set_trigger(Puppet::Relationship.new(resource, resource,

:callback => :refresh, :event => events))
+ end

+ resourceevents
end

# Apply each change in turn.
@@ -115,7 +129,11 @@ class Transaction
begin
# use an array, so that changes can return more than one
# event if they want
- events = [change.forward].flatten.reject { |e| e.nil? }
+ if resource.combine?
+ events = evaluate_multiple_resource(resource)
+ else
+ events = [change.forward].flatten.reject { |e| e.nil? }
+ end
rescue => detail
if Puppet[:trace]
puts detail.backtrace
@@ -482,6 +500,109 @@ class Transaction
end
end

+ # Function that evaluate multiple resource at the same class if
they support it.
+ def evaluate_multiple_resource(resource)
+ provider_class = resource.provider.class
+
+ if events = retrieve_combined_resource_events(resource)
+ return events
+ end
+
+ resources = find_combineable_resources(resource)
+ events = evaluate_multiple_resources(resources, provider_class)

+ set_triggers_from_resource(resources)

+
+ @combine_events[provider_class][resource.name]
+ end
+
+ # retrieves events associated to a specific resource if they exist
otherwise return nil
+ def retrieve_combined_resource_events(resource)
+ @combine_events ||= {}
+ @combine_events[resource.provider.class] ||= {}

+ @combine_events[resource.provider.class][resource.name]
+ end
+

+ # Find all resources matching the provider class that set combine
+ def find_combineable_resources(resource)
+

+ # collects the other resources that can be combined with resource

+ resources = catalog.vertices.find_all { |r|

+ r != resource and
r.provider.class.equal?(resource.provider.class) and r.combine? and
!skip?(r)
+ }
+
+ # rejects the resources that are already synced or couldn't
been synced
+ resources.reject! { |r|

+ begin
+ changes = r.evaluate

+ rescue => detail

+ if Puppet[:trace]
+ puts detail.backtrace
+ end

+ r.err "Failed to retrieve current state of resource :
%s" % detail

+ # Mark that it failed
+ @failures[r] = 1
+ changes = []

+ end
+
+ changes = [changes] unless changes.is_a?(Array)

+ changes_not_empty = changes.length > 0
+
+ changes.each { |c| @changes << c }
+
+ !changes_not_empty or !allow_processing?(r, changes) or
!dependencies_completed?(r)
+ }
+
+ # push the reference resource to the list
+ resources.push(resource)

+ end
+
+ # it performs the multiple evaluation
+ def evaluate_multiple_resources(resources, provider_class)
+ # Uses the first resource to define noop or not and print logs
+ reference_resource = resources.first
+ if resources.length > 0
+ begin

+ # Define event for each resources

+ failed_resources =
provider_class.install_multiple(resources)
+ resources.each { |r|
+ # Generate event
+ if failed_resources.include?(r)
+ r.err "Resource %s failed to install" % r[:name]

+ @failures[r] += 1

+ else
+ if r.noop
+ r.notice "Resource %s would have been
installed" % r[:name]

+ @combine_events[provider_class][r.name] =
[Puppet::Transaction::Event.new(:noop, r)]

+ else
+ r.notice "Resource %s have been installed"
% r[:name]

+ @combine_events[provider_class][r.name] =
[Puppet::Transaction::Event.new(r.name, r)]

+ end
+ @count = 1
+ r.cache(:synced, Time.now)
+ r.cache(:checked, Time.now)

+ end
+ }
+ rescue => detail
+ reference_resource.err "evaluate multiple resources
error: %s" % detail
+ if Puppet[:trace]
+ puts detail.backtrace
+ end
+ # All the resources failed
+ resources.each { |r|
+ @failures[r] += 1
+ }
+ return nil

+ end
+ end
+ end
+

+ def set_triggers_from_resource(resources)
+ resources.each { |r|
+ events = retrieve_combined_resource_events(r)
+ set_trigger_and_refresh(r, events)

+ }
+ end
+
# Prepare to evaluate the resources in a transaction.
def prepare
# Now add any dynamically generated resources

@@ -592,6 +713,8 @@ class Transaction
resource.debug "Not scheduled"
elsif failed_dependencies?(resource)
resource.warning "Skipping because of failed dependencies"
+ elsif failed?(resource)
+ resource.warning "Skipping because it has already failed once"
else
return false
end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index d300338..fcc4991 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1647,6 +1647,11 @@ class Type

end
end

+ # returns true if the resource can be combined
+ def combine?

+ provider.class.respond_to?(:install_multiple) and
resource[:combine] == :true

+ end
+
###############################
# All of the relationship code.

diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb

index bb3db28..6600cdd 100644

--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -35,6 +35,9 @@ module Puppet
package database for installed version(s), and can select
which out of a set of available versions of a package to
install if asked."
+ feature :combineable, "Multiple packages can be installed or
+ uninstalled in one command, rather than requiring one
command per
+ package."

ensurable do
desc "What state the package should be in.

@@ -277,6 +280,14 @@ module Puppet

newvalues(:true, :false)
end

+ newparam(:combine, :boolean => true, :required_features =>
:combineable) do
+ desc "Tells package provider if the package installation
have to be
+ perform in a single call by the package manager or if
it has to

+ be executed separately."
+

+ newvalues(:true, :false)
+ end
+
autorequire(:file) do
autos = []
[:responsefile, :adminfile].each { |param|

@@ -315,6 +326,11 @@ module Puppet
props
end
end
+
+ def combine?
+ provider.class.respond_to?(:install_multiple) and
self[:combine] == :true and
[:present,:latest,:installed].include?(self[:ensure])
+ end
+
end # Puppet::Type.type(:package)
end

[Luke Kanies]

On Jun 8, 2009, at 6:39 AM, Stephan Gorget wrote:

>
> Luke Kanies a écrit :
>> On Jun 5, 2009, at 9:12 AM, David Schmitt wrote:
>>
>> Yeah, I know this is a possibility, and would be required for more
>> complicated types.
>>
>> For the simple types like package, I don't think it's worth it, but
>> once you extend, yeah, you need to think about how to use the graph
>> to
>> solve some of these problems. I do expect it's a bit complicated,
>> though.
>>
> I modified the patch so that it doesn't try to install non-synced
> resource and it takes into account the dependency tree. This is
> based on
> the fact that a resource can only been installed if all its
> dependencies
> have the cached value :check setted. As I have to use
> resource.evaluate() to do so I do not invoke query() (rpm -q) before
> installing. It's only invoked after the yum call (it is what is done
> without the patch), it's perfectible but it's not taking a long time.

I don't know what you mean about the 'checked' value needing to be
cached. If that's required for the dependency system to work, then
that needs to be reworked. The transaction class currently doesn't
know anything about this 'checked' value, and it shouldn't need to.

--
The only thing that saves us from the bureaucracy is inefficiency. An
efficient bureaucracy is the greatest threat to liberty.
--Eugene McCarthy

[Marcin Owsiany]

On Wed, Jun 03, 2009 at 03:24:28PM -0500, Luke Kanies wrote:
> This does bring up one case where dependencies could be problematic -
> normally Puppet will refuse to do anything with a resource whose
> dependencies have failed, but in this case you could. But it
> shouldn't matter much for packages, I think.

I strongly disagree. This would mean that dependency relationships would
be silently seemingly randomly ignored. If someone specifies that a
package depends on something, then it means they really need that.

In the best case (package properly checking its dependencies) such
installation will fail and maybe succeed on a subsequent puppet run
(just introducing a delay).

In the worst case, installing a package without having prepared the
system for it will cause damage which is hard to debug and fix. One
might argue that such packages are broken, and it's not puppet's
responsibility to work around this, but:
- that is not always the case,
- even if it's the case, you need to think how many such broken
packages there are (*cough* Oracle *cough*)

--
Marcin Owsiany <mar...@owsiany.pl> http://marcin.owsiany.pl/
GnuPG: 1024D/60F41216 FE67 DA2D 0ACA FC5E 3F75 D6F6 3A0D 8AA0 60F4 1216

"Every program in development at MIT expands until it can read mail."
-- Unknown

[Luke Kanies]

On Jun 14, 2009, at 8:47 AM, Marcin Owsiany wrote:

>
> On Wed, Jun 03, 2009 at 03:24:28PM -0500, Luke Kanies wrote:
>> This does bring up one case where dependencies could be problematic -
>> normally Puppet will refuse to do anything with a resource whose
>> dependencies have failed, but in this case you could. But it
>> shouldn't matter much for packages, I think.
>
> I strongly disagree. This would mean that dependency relationships
> would
> be silently seemingly randomly ignored. If someone specifies that a
> package depends on something, then it means they really need that.
>
> In the best case (package properly checking its dependencies) such
> installation will fail and maybe succeed on a subsequent puppet run
> (just introducing a delay).
>
> In the worst case, installing a package without having prepared the
> system for it will cause damage which is hard to debug and fix. One
> might argue that such packages are broken, and it's not puppet's
> responsibility to work around this, but:
> - that is not always the case,
> - even if it's the case, you need to think how many such broken
> packages there are (*cough* Oracle *cough*)

If that's the case, then this seems essentially unresolvable without
spending a lot of development effort incorporating dependencies into
any attempt to combine installation.

That is, you could figure out from the graph what sets of packages
could be ordered together based on their respective dependencies and
then run them all at once, but without doing this work (if the above
is true), then it's essentially impossible to combine any package
installations.

Maybe we could do a first version that only installed packages that
didn't have any depenencies listed?

--
I can win an argument on any topic, against any opponent. People know
this, and steer clear of me at parties. Often, as a sign of their
great respect, they don't even invite me. -- Dave Barry

[Peter Meier]

Hi

> If that's the case, then this seems essentially unresolvable without
> spending a lot of development effort incorporating dependencies into
> any attempt to combine installation.

I'd like to second the arguments the previous poster raised. a package
installation could for example depend on a repository we're deploying,
etc. Which they do in various cases in my manifests.

> That is, you could figure out from the graph what sets of packages
> could be ordered together based on their respective dependencies and
> then run them all at once, but without doing this work (if the above
> is true), then it's essentially impossible to combine any package
> installations.

I think that this would be the most proper solution and follows
straight the idea of puppet's dependecy system, which I really like!

with that you would get various package "clouds" from the dependency
tree, which can be installed together.

> Maybe we could do a first version that only installed packages that
> didn't have any depenencies listed?

I think this would already be a huge step and I would be very happy if
puppet would install packages, which aren't in a depency chain,
together in one step.

cheers pete

[Luke Kanies]

On Jun 14, 2009, at 2:56 PM, Peter Meier wrote:

> Hi
>
>> If that's the case, then this seems essentially unresolvable without
>> spending a lot of development effort incorporating dependencies into
>> any attempt to combine installation.
>
> I'd like to second the arguments the previous poster raised. a
> package installation could for example depend on a repository we're
> deploying, etc. Which they do in various cases in my manifests.

Good point, I hadn't thought of that.

>
>> That is, you could figure out from the graph what sets of packages
>> could be ordered together based on their respective dependencies and
>> then run them all at once, but without doing this work (if the above
>> is true), then it's essentially impossible to combine any package
>> installations.
>
> I think that this would be the most proper solution and follows
> straight the idea of puppet's dependecy system, which I really like!
>
> with that you would get various package "clouds" from the dependency
> tree, which can be installed together.

The problems with this are a) it is actually a good bit of work and b)
it will likely require significant tuning of the graphing system.
Most of the graphing operations are potentially expensive, and adding
more per-resource dependency searches could have a large affect on the
performance.

The big problem is just the additional work, though, I think.

>
>> Maybe we could do a first version that only installed packages that
>> didn't have any depenencies listed?
>
> I think this would already be a huge step and I would be very happy
> if puppet would install packages, which aren't in a depency chain,
> together in one step.

Okay, I'll rely on the developer to incorporate this into the code,
then. I guess it's the only reasonable choice until we do all of the
additional work necessary in the graph.

--
Get forgiveness now -- tomorrow you may no longer feel guilty.

[Marcin Owsiany]

On Sun, Jun 14, 2009 at 02:31:00PM -0500, Luke Kanies wrote:
> Maybe we could do a first version that only installed packages that
> didn't have any depenencies listed?

This sounds like a solution that would give you the biggest bang for
buck, without violating any language semantics.

[Stéphan Gorget]

On Thu, Jun 11, 2009 at 5:03 PM, Luke Kanies<lu...@madstop.com> wrote:
>
> On Jun 8, 2009, at 6:39 AM, Stephan Gorget wrote:
>
>>
>> Luke Kanies a écrit :
>>> On Jun 5, 2009, at 9:12 AM, David Schmitt wrote:
>>>
>>> Yeah, I know this is a possibility, and would be required for more
>>> complicated types.
>>>
>>> For the simple types like package, I don't think it's worth it, but
>>> once you extend, yeah, you need to think about how to use the graph
>>> to
>>> solve some of these problems.  I do expect it's a bit complicated,
>>> though.
>>>
>> I modified the patch so that it doesn't try to install non-synced
>> resource and it takes into account the dependency tree. This is
>> based on
>> the fact that a resource can only been installed if all its
>> dependencies
>> have the cached value :check setted. As I have to use
>> resource.evaluate() to do so I do not invoke query() (rpm -q) before
>> installing. It's only invoked after the yum call (it is what is done
>> without the patch), it's perfectible but it's not taking a long time.
>
> I don't know what you mean about the 'checked' value needing to be
> cached.  If that's required for the dependency system to work, then
> that needs to be reworked.  The transaction class currently doesn't
> know anything about this 'checked' value, and it shouldn't need to.

There is a :synced and a :cached value stored in the cache, the
:synced value seems to be the timestamp when the resource was synced
(only if changes have been made) and :checked is the value of the
timestamp when the resource was evaluated. I was wondering if there is
a way to use these values to define that the dependency graph of a
specific resource have already been synced or checked. So that I can
make sure that the resource doesn't have any non-synced dependency.

However as a package can require another package in this case there is
maybe a problem because I use resource.evaluate() while installing
multiple package to define if a package is already installed. And as
evaluate() set the value of :checked that might make my reasoning be
wrong.

But, I think there might be a workaround that doesn't involve lot of
refactoring for example adding another value like :synced that can be
set when the resource have been synced or if no synchronization is
needed. And when all the dependency of a specific resource have this
value set that means that the resource can be applied without breaking
the dependency tree.

I haven't find if :synced is used (read or trigger sthg) somewhere
maybe this value can be set even if no synchronization was needed.

I hope I haven't miss something obvious in the code that made me do a
completely wrong argument.

--
Stéphan Gorget

[Luke Kanies]

You might be able to make that work, but I don't like the idea of
depending on those.

I think for now, if you're going to worry about dependencies then the
only reasonable way to do it is to skip any resources that actually
have any dependencies. Either that, or use the graph to create
subgraphs of resources with shared dependencies, and only install
those groups when the dependencies have been processed. E.g., if you
have a yum repo and all of your packages depend on that repo, then you
could see in the graph that the repo needs to have already been
processed by the time you get to the packages.

I don't actually know how this would work, exactly, which is why I've
been saying it's complicated and not recommended.

>
> However as a package can require another package in this case there is
> maybe a problem because I use resource.evaluate() while installing
> multiple package to define if a package is already installed. And as
> evaluate() set the value of :checked that might make my reasoning be
> wrong.
>
> But, I think there might be a workaround that doesn't involve lot of
> refactoring for example adding another value like :synced that can be
> set when the resource have been synced or if no synchronization is
> needed. And when all the dependency of a specific resource have this
> value set that means that the resource can be applied without breaking
> the dependency tree.
>
> I haven't find if :synced is used (read or trigger sthg) somewhere
> maybe this value can be set even if no synchronization was needed.

The problem with any of these values is that it's a bit ad-hoc in
puppet, and they're not really used anywhere. I'd be hesitant on
relying on them for much of anything, and that's one of the parts of
the transactional system that I actually want to significantly
refactor. If you want to add a more formal service for registering
states of resources, and then making those registrations available,
then I'd be open to that, but relying on what is currently internal
state data is not a good idea.

--
A gentleman is a man who can play the accordion but doesn't. --Unknown

[Stephan Gorget]

This version of the patch only combine package installation when a
package doesn't have a dependency. To use this feature the resource (the
package) should have the attribute "combine" set to True.

commit 08825636aa9f29d3307f9bc9de6eddb48500100c
Author: Stéphan Gorget <gor...@ocre.cea.fr>
Date: Mon Jun 22 21:32:18 2009 +0200

index f09ca80..528de72 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -43,6 +43,11 @@ class Transaction
return true
end

+ # check if all the dependencies have been synced
+ def has_dependencies?(resource)
+ return !relationship_graph.dependencies(resource).empty?

+ end
+
# Are there any failed resources in this transaction?
def any_failed?
failures = @failures.inject(0) { |failures, array| failures +=
array[1]; failures }

@@ -92,18 +97,22 @@ class Transaction

@@ -115,7 +124,11 @@ class Transaction

begin
# use an array, so that changes can return more than one
# event if they want
- events = [change.forward].flatten.reject { |e| e.nil? }
+ if resource.combine?
+ events = evaluate_multiple_resource(resource)
+ else
+ events = [change.forward].flatten.reject { |e| e.nil? }
+ end
rescue => detail
if Puppet[:trace]
puts detail.backtrace

@@ -482,6 +495,108 @@ class Transaction

has_dependencies?(r)

@@ -592,6 +707,8 @@ class Transaction

[Stéphan Gorget]

I realize that this patch cannot be pushed right now because the 0.26
branch is not set yet and it is premature to push it in the 0.25, but
I'd like to know if this patch seems correct to you before I add some
tests and specs.

--
Stéphan Gorget

[Luke Kanies]

On Jul 5, 2009, at 1:55 PM, Stéphan Gorget wrote:

> I realize that this patch cannot be pushed right now because the 0.26
> branch is not set yet and it is premature to push it in the 0.25, but
> I'd like to know if this patch seems correct to you before I add some
> tests and specs.

It's in my queue to review, I've just been busy moving across the
country and stuff, and it's a big patch, so I haven't been able to
review it yet.

I should get to it this week, hopefully Monday or Tuesday.

--
'Tis better to be silent and be thought a fool, than to speak and
remove all doubt. --Abraham Lincoln

[Luke Kanies]

I don't think this is quite right -- the trigger_and_refresh method
shouldn't replace the value of resourceevents, I think.

There's something I don't like about this method. If you're only
finding resources, why are you evaluating them? If the goal is to
figure out which ones to evaluate, then you shouldn't be evaluating.
Or am I missing something?

>
> + # push the reference resource to the list
> + resources.push(resource)
> + end
> +
> + # it performs the multiple evaluation
> + def evaluate_multiple_resources(resources, provider_class)
> + # Uses the first resource to define noop or not and print
> logs
> + reference_resource = resources.first
> + if resources.length > 0
> + begin
> + # Define event for each resources
> + failed_resources =
> provider_class.install_multiple(resources)
> + resources.each { |r|
> + # Generate event
> + if failed_resources.include?(r)
> + r.err "Resource %s failed to install" %
> r[:name]

This isn't quite right - well, it's right for now but if we add more
combineable resource types it won't be right.

This whole method is so large and nested it's barely readable. It'd
be great if you split it out a bit more.

Shouldn't this be 'resource.combine?', rather than [:combine] == :true?

--
It is better to sleep on things beforehand than lie awake about them
afterward. -- Baltasar Gracian

[Stephan Gorget]

Luke Kanies a écrit :

you're right

Evaluate a resource tells us if there is differences between the state
of the resource and the reference state, so that we can know if we have
to do something.

>
>> + # push the reference resource to the list
>> + resources.push(resource)
>> + end
>> +
>> + # it performs the multiple evaluation
>> + def evaluate_multiple_resources(resources, provider_class)
>> + # Uses the first resource to define noop or not and print
>> logs
>> + reference_resource = resources.first
>> + if resources.length > 0
>> + begin
>> + # Define event for each resources
>> + failed_resources =
>> provider_class.install_multiple(resources)
>> + resources.each { |r|
>> + # Generate event
>> + if failed_resources.include?(r)
>> + r.err "Resource %s failed to install" %
>> r[:name]
>
> This isn't quite right - well, it's right for now but if we add more
> combineable resource types it won't be right.

I can replace all the install in the log by evaluate, but the function
install_multiple is still directly call from here. But we can change its
name by evaluate_multiple which is more generic and less explicit

Yes I can split the function and extract the inside of resources.each {}.

if it has the correct behaviour (I haven't tried yet), yes it should.

[Stéphan Gorget]

Here is the version with the modifications.
I took a look at spec/unit/transaction.rb and test/util/transaction.rb
and I do not really feel confident about how to test it well, I'd be
glad to have some advice.

commit 8de5272cbab0c92980feb1f5ca3189ee0e4d278a

   Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>

index c0b4c0e..eff4cc7 100644

+        set_trigger_and_refresh(resource, resourceevents)
        resourceevents

+     end
+
+     def set_trigger_and_refresh(resource, resourceevents)
+         # And set a trigger for refreshing this resource if it's a
+         # self-refresher
+         if resource.self_refresh? and ! resource.deleting?
+             # Create an edge with this resource as both the source and
+             # target.  The triggering method treats these specially for
+             # logging.
+             events = resourceevents.collect { |e| e.name }
+             set_trigger(Puppet::Relationship.new(resource, resource,
:callback => :refresh, :event => events))
+         end

    end

    # Apply each change in turn.
@@ -115,7 +124,11 @@ class Transaction
            begin
                # use an array, so that changes can return more than one
                # event if they want
-                events = [change.forward].flatten.reject { |e| e.nil? }
+                if resource.combine?
+                    events = evaluate_multiple_resource(resource)
+                else
+                    events = [change.forward].flatten.reject { |e| e.nil? }
+                end
            rescue => detail
                if Puppet[:trace]
                    puts detail.backtrace

@@ -482,6 +495,125 @@ class Transaction
        end
    end

+    # Function that evaluate multiple resource at the same class if
they support it.
+    def evaluate_multiple_resource(resource)
+        provider_class = resource.provider.class
+
+        if events = retrieve_combined_resource_events(resource)
+            return events
+        end
+
+        resources = find_combineable_resources(resource)

+        events = evaluate_multiple_resources(resources)

+

+        # push the reference resource to the list
+        resources.push(resource)
+    end
+
+    # it performs the multiple evaluation

+    def evaluate_multiple_resources(resources)
+        # Uses the first resource as the one who notified errors
+        reference_resource = resources.first
+        provider_class = reference_resource.provider.class
+

+        if resources.length > 0
+            begin
+                # Define event for each resources
+                failed_resources = provider_class.install_multiple(resources)
+

+                resources.each { |r|
+                    set_combined_resource_metrics(r, failed_resources)
+                }

+
+            rescue => detail
+                reference_resource.err "evaluate multiple resources
error: %s" % detail
+

+                if Puppet[:trace]
+                    puts detail.backtrace
+                end
+

+                # All the resources failed
+                resources.each { |r|
+                    @failures[r] += 1
+                }
+
+                return nil
+            end
+        end

+    end
+
+    # Set information about the modification applied
+    def set_combined_resource_metrics(r, failed_resources)
+        provider_class = r.provider.class
+

+        # Generate event
+        if failed_resources.include?(r)

+            r.err "Resource %s failed to evaluate" % r[:name]

+            @failures[r] += 1
+

+        else
+            if r.noop

+                r.notice "Resource %s would have been evaluated" % r[:name]

+                @combine_events[provider_class][r.name] =
[Puppet::Transaction::Event.new(:noop, r)]
+
+            else

+                r.notice "Resource %s have been evaluated" % r[:name]
+                r.cache(:synced, Time.now)

+                @combine_events[provider_class][r.name] =
[Puppet::Transaction::Event.new(r.name, r)]
+            end
+

+            @count += 1

+        end
+    end
+

+    def set_triggers_from_resource(resources)
+        resources.each { |r|
+            events = retrieve_combined_resource_events(r)
+            set_trigger_and_refresh(r, events)
+        }
+    end
+
    # Prepare to evaluate the resources in a transaction.
    def prepare
        # Now add any dynamically generated resources

@@ -590,6 +722,8 @@ class Transaction

            resource.warning "Skipping because of failed dependencies"

        elsif resource.exported?
            resource.debug "Skipping because exported"

+        elsif failed?(resource)
+            resource.warning "Skipping because it has already failed once"
        else
            return false
        end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb

index 098d832..6668568 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1656,6 +1656,11 @@ class Type
        end
    end

+    # returns true if the resource can be combined
+    def combine?

+        provider.class.respond_to?(:install_multiple) and resource.combine?

--
Stéphan

[Nigel Kersten]

On Fri, Jul 24, 2009 at 7:38 AM, Stéphan Gorget <pha...@gmail.com> wrote:
>
> Here is the version with the modifications.
> I took a look at spec/unit/transaction.rb and test/util/transaction.rb
> and I do not really feel confident about how to test it well, I'd be
> glad to have some advice.

Anyone willing to step up and help Stéphan with this patch? Being able
to combine package installs together should improve client run time
significantly...

> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en
> -~----------~----~----~----~------~----~------~--~---
>
>

--
nigel

[Luke Kanies]

On Sep 27, 2010, at 9:30 AM, Nigel Kersten wrote:

> On Fri, Jul 24, 2009 at 7:38 AM, Stéphan Gorget <pha...@gmail.com> wrote:
>>
>> Here is the version with the modifications.
>> I took a look at spec/unit/transaction.rb and test/util/transaction.rb
>> and I do not really feel confident about how to test it well, I'd be
>> glad to have some advice.
>
> Anyone willing to step up and help Stéphan with this patch? Being able
> to combine package installs together should improve client run time
> significantly...

I agree it would make a big difference, and I worked with Stéphan on it when he first wrote it, but it ended up being far more complicated than initially expected.

I believe Markus will be making this one of his priorities for 2.7, although don't quite me on that quite yet.

--
Take the utmost trouble to find the right thing to say, and then say
it with the utmost levity. -- George Bernard Shaw
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199

[Nigel Kersten]

On Mon, Sep 27, 2010 at 11:02 PM, Luke Kanies <lu...@puppetlabs.com> wrote:
> On Sep 27, 2010, at 9:30 AM, Nigel Kersten wrote:
>
>> On Fri, Jul 24, 2009 at 7:38 AM, Stéphan Gorget <pha...@gmail.com> wrote:
>>>
>>> Here is the version with the modifications.
>>> I took a look at spec/unit/transaction.rb and test/util/transaction.rb
>>> and I do not really feel confident about how to test it well, I'd be
>>> glad to have some advice.
>>
>> Anyone willing to step up and help Stéphan with this patch? Being able
>> to combine package installs together should improve client run time
>> significantly...
>
> I agree it would make a big difference, and I worked with Stéphan on it when he first wrote it, but it ended up being far more complicated than initially expected.
>
> I believe Markus will be making this one of his priorities for 2.7, although don't quite me on that quite yet.

Have we been able to come up with any use cases other than packages
where combining offers a benefit?

I haven't been able to think of any yet.

[Luke Kanies]

All of the parsedfile stuff is a great fit, but I haven't thought of much else.

However, the basic idea of using the graph frontier is very widely
applicable - e.g., parallelizing file downloads. You can queue all of
the file downloads, and then mark each file as available as the
content comes in.

--
Luke Kanies | +1-615-594-8199

[Jeff McCune]

On Sep 28, 2010, at 12:15 PM, Nigel Kersten wrote:
> Have we been able to come up with any use cases other than packages
> where combining offers a benefit?

Mount resources.

A customer has around 800 static mount resources modeled in the catalog and each resource takes roughly 2 to 3 seconds to manage. Quite a long time for each puppet run.

I haven't dove into the code or system calls on this yet, but it was reported to me the mount resource executes mount twice for each resource which triggers a boatload of NFS RPC calls each invocation.

If we could just look at /proc/mounts once up front, then batch all of the out of sync resources together and then provide the option to call mount -a rather than N mount commands it might end up being much more efficient.

-Jeff

[Luke Kanies]

It should be pretty straightforward to add prefetching to 'mount' - I had no idea they were that expensive. That should probably just about kill most of that cost, I bet.

--
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it.
-- (attributed to) Brian W. Kernighan (unconfirmed)

[Jeff McCune]

On Sep 28, 2010, at 10:34 PM, Luke Kanies wrote:
> It should be pretty straightforward to add prefetching to 'mount' - I had no idea they were that expensive. That should probably just about kill most of that cost, I bet.

I'll take a look. Do you think it's wise to use package prefetching as a reference, or is there another resource with a "better" prefetching implementation?

-Jeff

[Luke Kanies]

Packaging is probably good. The only problem, I think, is that it sometimes is difficult to tell what's part of the API and what's part of the provider implementation.

--
I went to a restaurant that serves "breakfast at anytime". So I
ordered French Toast during the Renaissance. -- Stephen Wright

[David Schmitt]

On Tue, 28 Sep 2010 12:15:47 -0700, Nigel Kersten <nig...@google.com>
wrote:

> Have we been able to come up with any use cases other than packages
> where combining offers a benefit?

The nagios types seem to be reparsing/rewriting the target on every
resource. Having them grouped and written out at once might reduce the
overhead. I can imagine a provider implementation which prefetches the
target and delays the write-out internally, but I'd rather not ;-)
Especially since the nagios types have the additional problem of mixing
multiple types within the same target.

Having this in the "orchestration" layer and supported by a well-defined
API on the type and provider level (having the grouping explicit and allow
the implementation to provide a group_flush method), would make the flow
much clearer and keep types/providers from second-guessing the required
flush-barriers.

Best Regards, David

[Daniel Pittman]

For what it is worth that "orchestration" layer sounds suspiciously like what
might be needed internally to implement part of my mental model for resources.

Specifically (and, as y'all have heard before), the case where you have a
configuration file that is composed of multiple independent components like,
say, an Apache configuration file with multiple sites, but without using
'include' to implement it.[1]

Just sayin'
Daniel

Why, yes, I might have my eye on things that make this easier to implement
tomorrow, indeed.

Footnotes:
[1] Usually, of course, in the form of something that has multiple
independent components, but upstream doesn't provide 'include', and where
you currently find the 'concat' module or something similar working
around the issue of modelling this naturally.

--
✣ Daniel Pittman ✉ dan...@rimspace.net ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons

[Luke Kanies]

On Sep 30, 2010, at 1:27 AM, David Schmitt wrote:

>
> On Tue, 28 Sep 2010 12:15:47 -0700, Nigel Kersten <nig...@google.com>
> wrote:
>> Have we been able to come up with any use cases other than packages
>> where combining offers a benefit?
>
> The nagios types seem to be reparsing/rewriting the target on every
> resource. Having them grouped and written out at once might reduce the
> overhead. I can imagine a provider implementation which prefetches the
> target and delays the write-out internally, but I'd rather not ;-)
> Especially since the nagios types have the additional problem of mixing
> multiple types within the same target.

The Nagios types might just be a bit too complicated to do this if they are used to actually write multiple types to the same file, but in general, yeah, this is a good fit for the combining multiple resources into one run.

> Having this in the "orchestration" layer and supported by a well-defined
> API on the type and provider level (having the grouping explicit and allow
> the implementation to provide a group_flush method), would make the flow
> much clearer and keep types/providers from second-guessing the required
> flush-barriers.

Yep.

--
"They called me mad, and I called them mad, and damn them, they
outvoted me." -- Nathaniel Lee, on being consigned to a mental
institution, circa 17th c.

[Luke Kanies]

Interesting. I'm not sure it gets you all the way there, because you're at risk of having a dependency split the file into multiple saves, which probably doesn't do what you want.

I've been trying to come up with some kind of inspiration for how to do what amounts to snippets in memory - that is, building file snippets and combining them into a file without writing them to a disk - but I haven't quite. Not sure how much this helps, but I can almost see how you might think it'd get us there.

--
In science, 'fact' can only mean 'confirmed to such a degree that it
would be perverse to withhold provisional assent.' I suppose that
apples might start to rise tomorrow, but the possibility does not
merit equal time in physics classrooms. -- Stephen Jay Gould