I dug a little more into this. The weird behavior Jeff and Aurélien seen
was coming from the following conditional statement :
if ensure_prop = property(:ensure) or (self.class.needs_ensure_retrieved and
self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
# Retrieve ensure
else
# Don't retrieve ensure
end
Aurélien's patch adds
if ... and ensure_prop.should
Jeff's patch adds
if ... and (ignoreensure or ensure_prop.should)
And the problem was coming from this ensure_prop.should. During puppet runs
ensure_prop always exists, but ensure_prop.should only exists if something
if specified. But, when we are coming from Puppet::Type.to_resource,
ensure_prop still exists (if the property is ensurable) but ensure_prop
don't.
Thus, we are seeing this behavior :
- In the puppet resource application, with ignore_ensure=false and an
ensurable type (Jeff's patch == Aurélien's patch in this case), this
conditional statement evaluates to false (true or true and false). Because
*ensure_prop.should
evaluates to false*.
And, seeing this behavior, Jeff (IMHO) wrongly concluded
As an aside, I tested Aurelien's original patch with the mcollective and
> datacat_collector modules and noticed the same breakage my patch
> exhibited:
> *if ensure is not present in the resources returned from Type.retrieve,
> then modules using collected resources will break*.
But it's works, by removing the ensure_prop.should out of the conditional
statement. No need to check for ensure_prop.should
Please correct me if I'm wrong.
Cheers,
Le mercredi 22 juillet 2015 14:44:01 UTC+2, Felix Frank a écrit :
>
> Hi,
>
> thanks for picking this up.
>
> I think what really needs discussing is
> https://github.com/puppetlabs/puppet/pull/4038#issuecomment-119290626
>
> I'm not sure what kind of breakage specifically Jeff noticed in
> https://groups.google.com/forum/#!msg/puppet-dev/P8ReCHvmd2o/tuhzAM86wbYJ
>
> Perhaps this is about collections of exported resources using Service<<|
> |>> ? Could you make sure that still works with and without ensure
> attribute set in those?
>
> Any other ideas what might have been the issue? For all we know, the
> cause might have been resolved upstream in the meantime, but it would be
> nice to be sure.
>
> Thanks,
> Felix
>
> On 07/16/2015 09:06 AM, Romain F. wrote:
> > Hello,
> >
> > Necrobumping again this thread. Felix's wishes have been granted in
> > PR-4038 <https://github.com/puppetlabs/puppet/pull/4038> (PUP-4760
> > <https://tickets.puppetlabs.com/browse/PUP-4760>) but this change is
> bit
> > tricky/risky apparently.
> >
> > The original goal was to not retrieve ensure property status when we
> > don't ask to. This need a change in Puppet::Type's retrieve method.
> > Before the change in PR-4038
> > <https://github.com/puppetlabs/puppet/pull/4038>, it was
> > programmatically creating a ensure property when nothing is specified
> > and if the type is ensurable, so it was always retrieving the ensure
> > status.
> >
> > The change in the beginning of this thread was adding another condition
> > to this : if the type is ensurable and if the ensure property have a
> > should attribute.
> >
> > The change in PR-4038 <https://github.com/puppetlabs/puppet/pull/4038>
> > is just skipping the creation of the ensure property when nothing is
> > specified and if the type can continue without a ensure property (it's
> > the case for Services, not for Files for example).
> >
> > Like Felix's patch, it doesn't break any tests and it doesn't break
> > puppet resource <...> (which uses collected resources)
> >
> > Can you confirm that this would work ? Do think it can be extended to
> > some other types ?
> >
> > Cheers,
> >
> > Romain
> >
> >
> >
> >
> > Le lundi 5 mai 2014 02:16:33 UTC+2, Felix Frank a écrit :
> >
> > Hi,
> >
> > necro-bumping yet another thread, I took another stab at that old
> > problem.
> >
> > I molded Jeff's approach into a form that I hope to be more
> palatable.
> > It does not break any tests that I have tried (which is not saying
> it
> > won't break any whatsoever).
> >
> >
> https://github.com/ffrank/puppet/tree/dont-always-retrieve-service-ensure
> >
> >
> > So, if you guys could give it a spin, that would be awesome.
> >
> > Also, a ticket would be helpful, but if you can confirm that this
> works
> > and helps, I can open one on your behalf so we can make a PR for
> this.
> >
> > Cheers,
> > Felix
> >
> > On 12/19/2013 11:39 PM, Jeff Bachtel wrote:
> > >>>
> > >>> In the end, even just the behavior change to "puppet resource"
> > makes
> > >>> the patch a non-starter because it is a widely used feature.
> > >>
> > >> I understand this feature should be kept, but that a pity this
> > should
> > >> impact other even more useful feature like "apply" or "agent".
> > >>
> > >> Could it be possible that "puppet resource" and other like
> > "apply" or
> > >> "agent" retrieves only what they need? In apply/agent case, this
> > come
> > >> from a transaction being applied. For "puppet resource" I assume
> > this
> > >> is not the case. Could method parameter solve this case? And this
> > >> could even keep the compat if this param is not specified
> > >>
> > >
> > > I spent all day (because my Ruby is bad) doing a proof of concept
> > with
> > > this. It touches type.rb and indirector/resource/ral.rb to add a
> > > seemingly transparent method variable flagging whether ensure
> should
> > > be ignored for the purpose of retrieving resources. It defaults to
> > > false (don't ignore ensure), which should cause the speedup
> Aurelien
> > > is needing. The resource RAL indirector is aware of the method
> > > parameter and sets it to true (ignoreensure), thereby exhibiting
> the
> > > old behavior when puppet resource is called from the command line.
> > >
> > > There is a nasty bit that I'm unsure of in the retrieve_resource
> > > method. I discovered that when running puppet agent -t --noop, if
> I
> > > tried to use my newly defined method parameter that parsing would
> > > choke - apparently in that state the retrieve method is targeting
> > > another method. I worked around it by putting in a rescue
> statement
> > > and falling back to the old way of calling retrieve which should,
> > > eventually, still hit the retrieve with Aurelien's improved
> > > conditional logic.
> > >
> > > Anyway, please find attached a diff containing the changes in
> > > question. Feel free to refine and submit it as a PR, my Ruby
> really
> > > isn't up for my doing so myself.
> > >
> > > Jeff
> > >
> > >
>
>
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-dev/614f32ba-64a9-4572-879c-c5593196a420%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
