Bug #4111

If "ensure" changes, then no other changes are reported for that resource.

Added by Dan Bode almost 3 years ago. Updated 11 months ago.

Status:Code InsufficientStart date:07/01/2010
Priority:HighDue date:
Assignee:-% Done:

0%

Category:service
Target version:-
Affected Puppet version: Branch:
Keywords:

Description

code:

service { 'httpd':
  ensure => running,
  enable => true,
}

even though chkconfig is set to off in both cases, its only logged when the process isn’t started.

# /sbin/service httpd stop
Stopping httpd:                                            [  OK  ]
[root@dannyboy manifests]# chkconfig httpd off
[root@dannyboy manifests]# puppet myservice.pp 
notice: //Service[httpd]/ensure: ensure changed 'stopped' to 'running'
[root@dannyboy manifests]# chkconfig httpd off
[root@dannyboy manifests]# puppet myservice.pp 
notice: //Service[httpd]/enable: enable changed 'false' to 'true'
[root@dannyboy manifests]#

Related issues

Duplicated by Puppet - Bug #5991: Mounttype doesnt report changes when ensure changes Duplicate 01/24/2011
Duplicated by Puppet - Bug #18716: Service[x]/enable action is not reported on log when Serv... Duplicate

History

#1 Updated by James Turnbull almost 3 years ago

  • Category set to service
  • Status changed from Unreviewed to Accepted
  • Assignee set to Jesse Wolfe
  • Priority changed from Normal to High
  • Target version set to 2.6.0
  • Affected Puppet version changed from 0.25.5 to 2.6alpha1

#2 Updated by Jesse Wolfe almost 3 years ago

  • Subject changed from service does not log event for chkconfig update if service was started to If "ensure" changes, then no other changes are reported for that resource.

#3 Updated by Jesse Wolfe almost 3 years ago

  • Status changed from Accepted to Needs Decision
  • Target version changed from 2.6.0 to 2.7.x

#4 Updated by Jesse Wolfe almost 3 years ago

This is exacerbated in the case of Service in that service’s “ensure” is doing things other than absent/present.

#5 Updated by James Turnbull over 2 years ago

  • Status changed from Needs Decision to Accepted
  • Affected Puppet version deleted (2.6alpha1)

#6 Updated by James Turnbull over 2 years ago

  • Status changed from Accepted to Needs Decision
  • Assignee changed from Jesse Wolfe to Nigel Kersten

#7 Updated by Nigel Kersten over 2 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee deleted (Nigel Kersten)

#8 Updated by Stefan Schulte over 2 years ago

The reason is that we treat the ensure property very special: We always sync ensure first and if ensure is outofsync, we’ll only sync ensure and expect that everything is ok afterwards. This is wrong for every type with properties that do not depend on the ensure property. Another example is the mounttype. If we change ensure from unmounted to mounted we wont report any other change (added #5991 as duplicate).

Currently the only reason why enable is synced at all in the above example is that within the ensure block of the service type puppet tests if the enable property is out of sync.

Possible solution:

  • keep the current “hack” (sync enable within ensure) but add reporting functionality
  • treat ensure as any other property. This will definitly break most of the current types (user, group, host, etc.). We can fix these again if we introduce a new method/flag for properties that says “if you sync me, other properties are automatically in sync”
  • treat ensure special but only stop syncing other properties if ensure was changed to :absent OR ensure was :absent before the change. (This may also break current types like file when ensure changes from file to link but I’m not entirely sure)

Personally, I believe that 2 is the cleanest solution, but I bet there are custom types out there that depend on the current behaviour.

#9 Updated by Thomas Linkin 12 months ago

  • Status changed from Accepted to In Topic Branch Pending Review
  • Target version deleted (2.7.x)

Branch: https://github.com/puppetlabs/puppet/pull/872

In the beginning, there was ensure. And for a time, it was good.

I recently have been trying to write a custom type, and have been caught by this very issue. I understand the original concept, where ‘Ensure’ is the king of properties, however, in practice has not always been the case. For starters, using ensure is optional, so making the behaviour of properties in its presence so rigid seems troublesome. Additionally, I have caught the ‘hack’ mentioned above being used in the ‘Service’ type in puppet core. In the ‘Service’ type, the enable property is being brought in sync by the ensure property, when the ensure is being brought in sync.

Overall, I need to agree with option 2 from above. While I understand what ‘ensure’ is trying to do, I think we need more flexibility. Any types that take advantage of the ‘hack’ mentioned above miss out on having the changes to the system reported or audited.

I have included a possible solution. It allows you to designate properties as being “independent” from the ensure property. I went this route because most of the concepts behind why ensure works the way it does are logical and sound. In the interest of preserving this logic, the change maintains that ensure is always the first property to be brought in sync. The change is also backwards compatible with the ‘hack’ that was mentioned above.

#10 Updated by Daniel Pittman 12 months ago

Thomas Linkin wrote:

Branch: https://github.com/puppetlabs/puppet/pull/872

As I commented on the pull request:

I think this is a mistake: it adds a huge amount of complexity to the model, and doesn’t really address the problem.

A much better strategy would be to sync the ensure property, then check which other properties are out of sync and update those.

That has zero user facing complexity, results in the assurance that a resource is fully in sync after a Puppet run, and is absolutely transparent – it fixes all the existing types.

(Additionally, it has the virtue that it is not a “don’t be stupid” switch – this is a switch to say “don’t be stupid about fixing problems with this property”, which probably should apply to almost every single property of every single resource.)

I would be much more inclined to accept a special switch for “don’t sync this attribute” rather than “do sync this attribute”.

#11 Updated by Thomas Linkin 12 months ago

I think that idea would be just as easy to implement as my solution was. The only thing I’m unsure about is that more often properties will find themselves depending on ensure’s status than not.

#12 Updated by Kelsey Hightower 11 months ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

Also available in: Atom PDF