ensure property is always retrieve, even if useless

[DEGREMONT Aurelien]

Hello

Working on upgrading from 0.25 to 3.x, I faced a regression regarding
Service ensure behavior.

In 0.25, this code, on a RedHat system:

service { 'ntpd':
enable => false,
hasstatus => true;
}

would trigger:

/sbin/chkconfig ntpd

and NO:

service ntpd status

Starting from 2.6 and up to 3.x, both commands are run, the service
status is checked (service ntpd status is run), even if ensure is not
set (or set to "undef").
On some systems, we have up to 50 services checked, and this is a huge
performance impact on our Puppet runs to launch this command when we do
not care.
(some services could be quite long to be checked).

I tracked down this behavior change up to this (old) patch:


commit 3f6c9481ce9ac59fb3a84ab6792543d039ee403f
Author: Luke Kanies <lu...@reductivelabs.com>
Date: Tue Jan 19 18:24:15 2010 -0800

Changing Transaction to use the new ResourceHarness

This is a much messier commit than I would like,
mostly because of how 'file' works. I had to
fix multiple special cases, and I had to move others.

The whole system appears to now work, though, and we're
ready to change reports to receive resource status
instances rather than events.

Signed-off-by: Luke Kanies <lu...@reductivelabs.com>


And I quickly workaround this using this oneliner patch:

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1933097..bf09fdb 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -681,7 +681,7 @@ class Type
# Provide the name, so we know we'll always refer to a real thing
result[:name] = self[:name] unless self[:name] == title

- if ensure_prop = property(:ensure) or
(self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
+ if ensure_prop = property(:ensure) or
(self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure)) and
ensure_prop.should
result[:ensure] = ensure_state = ensure_prop.retrieve
else
ensure_state = nil


What do you think of this?


Aur�lien

[Andy Parker]

On Tue, Dec 17, 2013 at 7:42 AM, DEGREMONT Aurelien <aurelien....@cea.fr> wrote:

Hello

Working on upgrading from 0.25 to 3.x, I faced a regression regarding Service ensure behavior.

In 0.25, this code, on a RedHat system:

service { 'ntpd':
    enable => false,
    hasstatus => true;
}

would trigger:

/sbin/chkconfig ntpd

and NO:

service ntpd status

Starting from 2.6 and up to 3.x, both commands are run, the service status is checked (service ntpd status is run), even if ensure is not set (or set to "undef").
On some systems, we have up to 50 services checked, and this is a huge performance impact on our Puppet runs to launch this command when we do not care.
(some services could be quite long to be checked).

Hmmm. That is unfortunate.

 

I tracked down this behavior change up to this (old) patch:


commit 3f6c9481ce9ac59fb3a84ab6792543d039ee403f
Author: Luke Kanies <lu...@reductivelabs.com>
Date:   Tue Jan 19 18:24:15 2010 -0800

    Changing Transaction to use the new ResourceHarness

    This is a much messier commit than I would like,
    mostly because of how 'file' works.  I had to
    fix multiple special cases, and I had to move others.

    The whole system appears to now work, though, and we're
    ready to change reports to receive resource status
    instances rather than events.

    Signed-off-by: Luke Kanies <lu...@reductivelabs.com>

Thanks for tracking it down. I looked over the patch, but as the comment says it is pretty large and I couldn't clearly find the change the caused the different behavior.

 

And I quickly workaround this using this oneliner patch:

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1933097..bf09fdb 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -681,7 +681,7 @@ class Type
     # Provide the name, so we know we'll always refer to a real thing
     result[:name] = self[:name] unless self[:name] == title

-    if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
+    if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure)) and ensure_prop.should
       result[:ensure] = ensure_state = ensure_prop.retrieve
     else
       ensure_state = nil


What do you think of this?

I tried it out. After unfortunately it changes the behavior of how puppet detects the current state of a resource on the system. You can see this pretty clearly when you use the "puppet resource" command to inspect something:

Without patch:

[root@localhost puppet]# puppet resource service rsyslogd

service { 'rsyslogd':

  ensure => 'stopped',

  enable => 'false',

}

With patch:

[root@localhost puppet]# puppet resource service rsyslogd

service { 'rsyslogd':

  enable => 'false',

}

I think you might be on the right track. I haven't looked into this enough to be able to give you any pointers right now about where to make a correct change.

 

Aurélien

--
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+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52B070D5.9040303%40cea.fr.
For more options, visit https://groups.google.com/groups/opt_out.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco

[John Bollinger]

On Tuesday, December 17, 2013 1:05:09 PM UTC-6, Andy Parker wrote:

On Tue, Dec 17, 2013 at 7:42 AM, DEGREMONT Aurelien <aurelien....@cea.fr> wrote:

And I quickly workaround this using this oneliner patch:

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1933097..bf09fdb 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -681,7 +681,7 @@ class Type
     # Provide the name, so we know we'll always refer to a real thing
     result[:name] = self[:name] unless self[:name] == title

-    if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
+    if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure)) and ensure_prop.should
       result[:ensure] = ensure_state = ensure_prop.retrieve
     else
       ensure_state = nil


What do you think of this?

I tried it out. After unfortunately it changes the behavior of how puppet detects the current state of a resource on the system. You can see this pretty clearly when you use the "puppet resource" command to inspect something:

Well that's the point, isn't it?  Aurelien wants Puppet to avoid evaluating the current state of the 'ensure' property of Service resources where that property is unmanaged.

At least, he wants the agent and presumably 'apply' to operate that way.  I suppose the current behavior is more sensible for the 'resource' face.

I think he has a point, and a more general one than just about the Service.ensure property.  The state-probing behavior appropriate for the 'resource' face and maybe some others is not necessarily ideal for the 'agent' and 'apply' faces.  In some cases it may require little or no extra effort to determine the current state of unmanaged properties, but in other cases doing so can be expensive and therefore should be avoided where determining that state is not a desired or needful outcome.


John

[Erik Dalén]

If that behaviour is changed, wouldn't this break the resources type?

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

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/d757615b-f8a0-4a23-983e-f04301f950bc%40googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--
Erik Dalén

[Felix Frank]

On 12/19/2013 12:15 PM, Erik Dal�n wrote:
> If that behaviour is changed, wouldn't this break the resources type?

Hmm, I think not.

Purging of unmanaged resources is pretty much all that the resources
type is currently capable of. If a resource is not "present" in the
ensure sense, it should not get generated in the first place. If it does
get generated, it implicitly requires purging if it's not managed.
Populating the ensure property should not be strictly required.

This is me guessing mostly, I'm not entirely sure.

Regards,
Felix

[Erik Dalén]

You could run it on the service type though to stop services you haven't explicitly set to run. For services even the stopped ones are present on the system.

On 19 December 2013 12:32, Felix Frank <felix...@alumni.tu-berlin.de> wrote:

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

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52B2D933.9030902%40alumni.tu-berlin.de.

For more options, visit https://groups.google.com/groups/opt_out.

--
Erik Dalén

[Felix Frank]

On 12/19/2013 01:20 PM, Erik Dal�n wrote:
> You could run it on the service type though to stop services you haven't
> explicitly set to run. For services even the stopped ones are present on
> the system.

Ahh, via ensure => stopped in the resources instance? Is ensure a
metaparameter? I wasn't aware this was possible - fascinating.

There should be a test for this if there isn't one already.

I'm still not sure this would break, because there *is* a should value
for ensure in this case.

[Andy Parker]

On Thu, Dec 19, 2013 at 3:32 AM, Felix Frank <felix...@alumni.tu-berlin.de> wrote:

On 12/19/2013 12:15 PM, Erik Dalén wrote:
> If that behaviour is changed, wouldn't this break the resources type?

Hmm, I think not.

I took a quick look and I think the resource type is fine. However, since this patch means that we never retrieve the ensure value of the instance the logic around checking how to manage parameters (https://github.com/puppetlabs/puppet/blob/master/lib/puppet/transaction/resource_harness.rb#L80-83) would end up trying to manage resources that might be absent, where it should have done nothing. So for instance, if the service resource in the example

  service { 'ntpd:

    enable => false,

    hasstatus => true,

  }

I think would fail if the ntpd service didn't exist on the system, whereas right now it would be skipped. Disclaimer: I haven't tried this out yet, but reading the code leads me to think this is the case.

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'm not sure how far down inside the APIs that functionality needs to be preserved; however, since the retrieve method has had the current behavior for ~4 years, I'm willing to bet that there are systems that use retrieve directly and expect the current behavior.

 

Purging of unmanaged resources is pretty much all that the resources
type is currently capable of. If a resource is not "present" in the
ensure sense, it should not get generated in the first place. If it does
get generated, it implicitly requires purging if it's not managed.
Populating the ensure property should not be strictly required.

This is me guessing mostly, I'm not entirely sure.

Regards,
Felix

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

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52B2D933.9030902%40alumni.tu-berlin.de.

For more options, visit https://groups.google.com/groups/opt_out.

--

[Felix Frank]

On 12/19/2013 06:06 PM, Andy Parker 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'm not sure
> how far down inside the APIs that functionality needs to be preserved;
> however, since the retrieve method has had the current behavior for ~4
> years, I'm willing to bet that there are systems that use retrieve
> directly and expect the current behavior.

Perhaps it could be accepted iff it needs to be specifically enabled via
configuration?

[DEGREMONT Aurelien]

Le 19/12/2013 18:06, Andy Parker a écrit :

I took a quick look and I think the resource type is fine. However, since this patch means that we never retrieve the ensure value of the instance the logic around checking how to manage parameters (https://github.com/puppetlabs/puppet/blob/master/lib/puppet/transaction/resource_harness.rb#L80-83) would end up trying to manage resources that might be absent, where it should have done nothing. So for instance, if the service resource in the example

  service { 'ntpd:

    enable => false,

    hasstatus => true,

  }

I think would fail if the ntpd service didn't exist on the system, whereas right now it would be skipped. Disclaimer: I haven't tried this out yet, but reading the code leads me to think this is the case.

I've tried with my patch, and it works (I mean no error, it just ran "chkconfig foo")

  service { 'foo':

    enable => false,

    hasstatus => true,

  }

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'm not sure how far down inside the APIs that functionality needs to be preserved; however, since the retrieve method has had the current behavior for ~4 years, I'm willing to bet that there are systems that use retrieve directly and expect the current behavior.

I'm working hard to go back to 4-years old Puppet performances, which was much faster than current version. It could be nice to work together to have the best of each world, features and performances.


Aurélien

[John Bollinger]

On Thursday, December 19, 2013 6:40:00 AM UTC-6, Felix Frank wrote:

On 12/19/2013 01:20 PM, Erik Dal�n wrote:
> You could run it on the service type though to stop services you haven't
> explicitly set to run. For services even the stopped ones are present on
> the system.

Ahh, via ensure => stopped in the resources instance? Is ensure a
metaparameter? I wasn't aware this was possible - fascinating.

No, 'ensure' is not a metaparameter (http://docs.puppetlabs.com/references/3.stable/metaparameter.html).  It is an ordinary parameter that many (but not all) resource types have, and to which both internal and external conventions pertain.  Among those, 'ensure' is the property with which purging interacts (which is why Resources that do not have an 'ensure' parameter cannot be purged).  I can't see how one would use a Resources resource to ensure services 'stopped' instead of 'absent'.

At the same time, it is a reasonable concern that the proposed patch would break purging via Resources instances.  If it did, though, I think it would be possible to work around that.  The general idea is feasible.

The challenge appears to be to continue to provide the current behavior for the 'resource' face while making the 'agent' and 'apply' faces avoid performing the kind of unneeded work that Aurelien identified.  Is there a way for the Puppet internals to determine which face they are servicing?


John

[Jeff Bachtel]

>>
>> 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

[DEGREMONT Aurelien]

Le 19/12/2013 23:39, Jeff Bachtel a �crit :

>>>
>>> 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
>>
>

> 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.

I was also thinking doing the same kind of proof of concept.
But I would have slightly change the way the default values are passed.

Apart those changes, I think this shows we can address the different
concerns which were raised previously.

Assuming this patch is clean/adapted, is this something Puppet could
accept? or there is still other problems?


Aur�lien

[Felix Frank]

On 12/20/2013 03:41 PM, DEGREMONT Aurelien wrote:
> Assuming this patch is clean/adapted, is this something Puppet could
> accept? or there is still other problems?

I can't really give a well founded comment on this.

I like Jeff's approach in spirit, but as far as execution is concerned,
my gut says that something more object oriented would be preferable.
What I'm saying is, instead of enriching interfaces with values that
need passing, an approach based on inheritance and method overrides
might lead to better maintainability later on.

[DEGREMONT Aurelien]

Le 20/12/2013 16:03, Felix Frank a �crit :

Agreed, but I think guidance from Puppet developers is needed for that.

As far as I'm concern, I do not think I know the puppet code well enough
to do this alone.


Aur�lien

[Jeff Bachtel]

+1 - doing a rescue because I can't figure out where
Type.retrieve_resource is going for the retrieve method when called from
puppet agent is a pretty sure warning sign that this isn't the cleanest
approach. I also noticed that my patch breaks application of stored
resources in modules, probably because that's using the RAL resource
indirector as well.

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.

To resolve problems with the patch would I think be intensive, as
anything dealing with stored resources would need to set
ignoreensure=true. It'd be better to go from the opposite direction and
default to the current behavior, but have manifest application invoke
retrieve in a different way (or define its own retrieve).

Jeff

[Andy Parker]

On Fri, Dec 20, 2013 at 7:12 AM, DEGREMONT Aurelien <aurelien....@cea.fr> wrote:

Le 20/12/2013 16:03, Felix Frank a écrit :

On 12/20/2013 03:41 PM, DEGREMONT Aurelien wrote:

Assuming this patch is clean/adapted, is this something Puppet could
accept? or there is still other problems?

I can't really give a well founded comment on this.

If this can be done in a way the doesn't regress other functionality, then we would definitely take a patch. Getting a speed improvement in a common case would be a great thing!

 

I like Jeff's approach in spirit, but as far as execution is concerned,
my gut says that something more object oriented would be preferable.
What I'm saying is, instead of enriching interfaces with values that
need passing, an approach based on inheritance and method overrides
might lead to better maintainability later on.

Agreed, but I think guidance from Puppet developers is needed for that.

Either inheritance or delegation would be good in this case. I had the same reaction as Felix to the parameter being passed.

 

As far as I'm concern, I do not think I know the puppet code well enough to do this alone. 

 

 

This might be a good why to dive in and see if you can figure out a little more :). I think what you put out was a good proof of concept. Then what Jeff followed up with was a nice continuation. If I'm interpreting his last message correctly he has uncovered a few more consequences of changing the behavior. Investigating those might provide some insight into what needs to change and what needs to stay the same.

I'm sorry if that is a little vague, but I'm not overly familiar with this part of the codebase either.

Aurélien

--
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+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52B45E67.2010709%40cea.fr.

For more options, visit https://groups.google.com/groups/opt_out.

[Andy Parker]

On Fri, Dec 20, 2013 at 7:45 AM, Jeff Bachtel <jbac...@bericotechnologies.com> wrote:

On 12/20/2013 10:03 AM, Felix Frank wrote:

On 12/20/2013 03:41 PM, DEGREMONT Aurelien wrote:

Assuming this patch is clean/adapted, is this something Puppet could
accept? or there is still other problems?

I can't really give a well founded comment on this.

I like Jeff's approach in spirit, but as far as execution is concerned,
my gut says that something more object oriented would be preferable.
What I'm saying is, instead of enriching interfaces with values that
need passing, an approach based on inheritance and method overrides
might lead to better maintainability later on.

+1 - doing a rescue because I can't figure out where Type.retrieve_resource is going for the retrieve method when called from puppet agent is a pretty sure warning sign that this isn't the cleanest approach. I also noticed that my patch breaks application of stored resources in modules, probably because that's using the RAL resource indirector as well.

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.

Ah, so there is other code out there that relies on the current behavior of retrieve. I had suspect as much, but it is good to know :)

 

To resolve problems with the patch would I think be intensive, as anything dealing with stored resources would need to set ignoreensure=true. It'd be better to go from the opposite direction and default to the current behavior, but have manifest application invoke retrieve in a different way (or define its own retrieve).

I'm thinking maybe a different `retrieve` might be the way to go. Passing around a boolean just isn't a great way to go.

 

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+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/52B46617.8080202%40bericotechnologies.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Felix Frank]

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

[Romain F.]

Hello,

Necrobumping again this thread. Felix's wishes have been granted in PR-4038 (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, 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 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

[DEGREMONT Aurelien]

Hi,

+1
I really like this approach of this problem. I think Felix's original patch is much better than what I did years ago. It fixes the problem without the regression I had introduced.
Patch seems really clean and as all Puppet tests are passing without problem I'm really confident in this patch.

Aurélien

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

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/d846f39c-753f-4aaa-99f0-947822791c50%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Felix Frank]

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>

[Romain F.]

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,