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.
>
>>
>>> 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?
>>
>>> 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
>>
>> 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?
> 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.
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().
>
>>> + 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
>>
>> 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.
>>
>>> + #
>>> + # 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
>>
>> This can be reduced to:
>>
>> def combine?
>> provider.class.respond_to?(:install_multiple) and
>> resource[:combine] == :true
>> 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
>>> + }
>>
>> This should have no default value. No way this should default to
>> true.
> 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 ?
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.
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
