Feature #3757

--enable and --disable should be improved

Added by R.I. Pienaar about 2 years ago. Updated 12 days ago.

Status:Merged - Pending Release Start date:05/12/2010
Priority:Normal Due date:
Assignee:- % Done:

0%
Category:executables
Target version:3.0.0
Affected Puppet version:0.25.4 Branch:https://github.com/puppetlabs/puppet/pull/680
Keywords:mcollective enabledisable
Votes: 2

Description

At present the —enable/—disable and associated checks on the puppetd is all done via a lock file /var/lib/puppet/state/puppetdlock.

My investigations reveal the behavior of this file to be:

  • if exist and it’s empty, someone ran —disable
  • if exist and it’s got a PID in it puppetd is doing a manifest run at present
  • if it’s absent its enabled and not running

The problem comes when you want to disable the daemon while it’s running a manifest. The lock file with PID in it will remain in place and once puppetd is done doing its run it will remove the lock file, leaving your client enabled when infact it was asked to disable the client and not run in future.

So I want to be able to disable future runs even while a current run is happening. The easiest might be to have 2 lock files – one to indicate enabled/disabled and one to indicate running dont start another concurrent run.

Related issues

related to Puppet - Feature #4836: puppetd --disable improvements Closed 09/23/2010
related to Puppet - Bug #5139: puppetdlock file can be empty Needs More Information 10/28/2010
related to MCollective Plugins - Feature #9923: mcollective puppetd plugin does not work with daemonized ... Merged - Pending Release 10/05/2011
related to Puppet - Bug #12844: Puppet upgrade can't remove lockfile. Closed 02/27/2012
related to Puppet - Bug #11057: Better debugging output when skipping an agent/master run. Closed 11/28/2011
related to Puppet - Feature #12934: Allow customizable message for administratively disabled ... Merged - Pending Release 03/02/2012

History

#1
Updated by James Turnbull about 2 years ago

  • Category set to executables
  • Status changed from Unreviewed to Needs Decision
  • Assignee set to Luke Kanies

I am happy with the intent – not sure on the implementation but can’t think of a better approach. Luke?

#2
Updated by Luke Kanies about 2 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee deleted (Luke Kanies)

I agree – I’m fine on the feature change, and I like the idea of easily being able to {en,dis}able puppetd from outside of Puppet.

I guess two lock files is the only reasonable way. Anyone else have any suggestions?

#3
Updated by Nigel Kersten over 1 year ago

If we’re going to fix this with two lockfiles, I’d really like to see us address at least the part of #4836 that asks for a feature to specify a lock message.

#4
Updated by R.I. Pienaar about 1 year ago

  • Keywords set to mcollective

#5
Updated by James Turnbull 7 months ago

  • Keywords changed from mcollective to mcollective enabledisable

#6
Updated by Brice Figureau 6 months ago

  • Branch set to https://github.com/puppetlabs/puppet/pull/216

Fixed in pull request 216.

#7
Updated by Brice Figureau 6 months ago

  • Status changed from Accepted to In Topic Branch Pending Review

#8
Updated by Jacob Helwig 5 months ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version set to 2.7.10

This has been merged into 2.7.x in commit:86a806f595f8b7bb280c8c445eef51dfd71bf39d

(#3757) - Move enable/disable to its own lock

This change also contains a refactor split of Pidlock into its
anonymous counterpart.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

#9
Updated by Michael Stahnke 4 months ago

  • Status changed from Merged - Pending Release to Closed

released in 2.7.10rc1

#10
Updated by Chris Price 3 months ago

  • Status changed from Closed to Re-opened
  • Target version changed from 2.7.10 to 3.X

Re-opened because we are reverting this out of 2.7.x

#11
Updated by Brice Figureau 3 months ago

Chris Price wrote:

Re-opened because we are reverting this out of 2.7.x

What’s the underlying reason of the revert?

#12
Updated by Chris Price 2 months ago

This was reverted out of 2.7.x because it broke backwards compatibility with mcollective. It will be re-introduced in some fashion in Telly.

For the play-by-play on what happened with the reversion, see #12844.

#13
Updated by Chris Price about 1 month ago

  • Assignee set to Chris Price

This still needs to get merged back in to Telly. I’ve discussed with Nigel and Daniel, and the summary of current affairs is:

  1. There seems to be general consensus (at least from the dev side) that minimizing file-based API is probably wise
  2. These two cases (“is an agent running?”, and “is an agent administratively disabled, and why?”) seem to qualify as cases where a file-based API is useful enough to accept the negative consequences
  3. These cases need to be documented very clearly, explicitly, publicly… both in the code and wherever we are documenting public API on the web site.
  4. Even though we should attempt to minimize the amount of data that these files contain (so that we can minimize backward-compatibility concerns in the future), if we’re going to put data in these files at all, it would be smart to add a minimal amount of structure to it (key-value pairs in this case) to give us more flexibility down the road.

#14
Updated by Chris Price about 1 month ago

Daniel suggests that JSON is probably the best format to use to express the data in these files.

#15
Updated by Chris Price about 1 month ago

Small update to the plan here…

I have the JSON lockfile path all implemented, but it breaks acceptance tests because they expect to be able to do something like this:

kill `cat /my/pid/file`

This no longer works if the pidfile contains JSON rather than just a simple pid. As a compromise, I’m going to roll back the JSON part for the “agent-is-running” pidfile and restore it to just using a simple pid. The “agent-is-administratively-disabled” lockfile will still use JSON.

#16
Updated by Jo Rhett about 1 month ago

If everyone has agreed that the new system is improved, why not fix the acceptance tests?

#17
Updated by Chris Price about 1 month ago

I am writing acceptance tests for this as we speak. I don’t believe that the existing ones are broken at the moment, though, as the changes described in this ticket have not yet been merged up to master.

#18
Updated by Chris Price about 1 month ago

  • Status changed from Re-opened to In Topic Branch Pending Review

Recording the original pull request URL for the 2.7.x changes here for posterity:

https://github.com/puppetlabs/puppet/pull/216

#19
Updated by Chris Price about 1 month ago

  • Branch changed from https://github.com/puppetlabs/puppet/pull/216 to https://github.com/puppetlabs/puppet/pull/680

#20
Updated by Daniel Pittman 26 days ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Assignee deleted (Chris Price)

This has been merged to Telly, and will be part of the release.

#21
Updated by Daniel Pittman 12 days ago

  • Target version changed from 3.X to 3.0.0

Also available in: Atom PDF