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 puppet-dev+unsubscr...@googlegroups.com.
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.
