The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Feature #3323

Create hasreload similar to hasrestart

Added by Chuck McIntyre about 6 years ago. Updated over 2 years ago.

Status:Needs More InformationStart date:03/03/2010
Priority:NormalDue date:
Assignee:James Turnbull% Done:

0%

Category:service
Target version:3.x
Affected Puppet version:0.24.8 Branch:https://github.com/jamtur01/puppet/tree/tickets/master/3323
Keywords:hasreload, service

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

Many init scripts allow for a “reload” option in addition to a restart and a stop/start.

It would be useful if this could be defined for a given service generically, rather than having to resort to an Exec resource or other workaround for cases where we want to reload instead of restart (an example could be made using apache or bind configuration changes).

I am attaching an example diff (note this is not a patch, nor has it been tested) to describe in more detail how this might be accomplished.

I am willing to submit a patch for this if the feature is accepted. I would like some guidance as to whether the existing unit test modifications are sufficient or if it is necessary to test that (at least) reload and restart get called appropriately (not simply that stop/start get called when not specified).

puppet_hasreload.diff Magnifier - new feature: service hasreload - untested EXAMPLE diff (4.61 KB) Chuck McIntyre, 03/03/2010 09:54 pm


Related issues

Related to Puppet - Feature #1014: Services should support 'reload' in addition to 'restart' Accepted
Duplicated by Puppet - Feature #16123: hasreload for service resources Duplicate 08/27/2012

History

#1 Updated by James Turnbull about 6 years ago

  • Category set to service
  • Status changed from Unreviewed to Needs Decision
  • Assignee set to Markus Roberts

+1 from me.

#2 Updated by James Turnbull about 5 years ago

  • Assignee changed from Markus Roberts to Nigel Kersten

Hi Chuck – sorry this has launguished for so long! I’ve assigned it our product manager for comment.

Can I also ask that you sign a CLA so that we can merge your code? The CLA link is in the top menu bar of the Redmine site.

#3 Updated by Nigel Kersten over 4 years ago

  • Status changed from Needs Decision to Requires CLA to be signed
  • Assignee changed from Nigel Kersten to Chuck McIntyre

Chuck, any chance of the CLA being signed? I know your employer has approved them in the past :)

#4 Updated by Chuck McIntyre over 4 years ago

On Fri, Aug 12, 2011 at 7:06 AM, tickets@puppetlabs.com wrote:

Chuck, any chance of the CLA being signed? I know your employer has approved them in the past :)

Hi Nigel – I’m not really working on the same project anymore, but I’ll see what I can do.

#5 Updated by James Turnbull over 4 years ago

  • Branch set to https://github.com/jamtur01/puppet/tree/tickets/master/3323

#6 Updated by James Turnbull over 4 years ago

  • Status changed from Requires CLA to be signed to In Topic Branch Pending Review

Hi Chuck – hope you don’t mind I updated and extended your code and submitted it as a pull request. It now supports reload and graceful and I extended the test coverage a little also.

#7 Updated by Chuck McIntyre over 4 years ago

On Fri, Aug 12, 2011 at 12:55 PM, tickets@puppetlabs.com wrote:

Hi Chuck – hope you don’t mind I updated and extended your code and submitted it as a pull request. It now supports reload and graceful and I extended the test coverage a little also.

That’s great, thanks. I had intended to do that when the feature was approved, but things took so long that I totally lost sight of this. I have no issue with modifications or rewrites as the originally submitted code was intended as a proof of concept. In case it still matters I’m working on the CLA now.

#8 Updated by James Turnbull over 4 years ago

Thanks Chuck – Having the CLA signed would still be useful too!

#9 Updated by Ian Delahorne over 4 years ago

Tested on CentOS 5.5, seems to work fine.

#10 Updated by Jacob Helwig over 4 years ago

  • Status changed from In Topic Branch Pending Review to Tests Insufficient
  • Target version set to 3.x

Left some comments on the pull request.

#11 Updated by James Turnbull over 4 years ago

Replied to your comments. Happy to meet and discuss.

#12 Updated by Anonymous over 4 years ago

  • Status changed from Tests Insufficient to Code Insufficient

Given that this only supports one choice, stop/start, restart, reload, or graceful, it seems highly counter-intuitive that you would have to pick one of multiple boolean values. It would, I think, make a great deal more sense recast as a single multi-value field that allowed the selection between them.

That also avoids the obvious failure mode, which is that someone accidentally sets two of the fields true, resulting in unspecified behaviour. (I would object less if the change treated setting multiple values as a hard error, which it does not.)

However, it seems to me that this change is likely really wanting to have a richer set of notification semantics for services, rather than wanting to be able to hard-code the type of restart, no? In that case, is this actually a valuable intermediate step?

#13 Updated by James Turnbull over 4 years ago

Daniel – I’d welcome some help refactoring this then.

#14 Updated by Anonymous over 4 years ago

James Turnbull wrote:

Daniel – I’d welcome some help refactoring this then.

So, my first question is: is there actually value in this change?

Given that hasrestart => true, restart => '... graceful' has the same effect, is it worth adding extra code to make this happen?

Second question, do we want to add more restart modes at this point in time, rather than delaying until we have richer semantics available for notifications. (eg: restart gracefully for these notifications, but full restart for these others, type stuff.)

Assuming the answer is yes to both, I would suggest the API be the option to set graceful for hasrestart, which has the effect of invoking the init script with graceful rather than restart as the option. That seems the minimum change, adds no new fields, and almost no new semantics.

Does that seem reasonable? It feels like the absolute minimum change to support flagging this is a service we would prefer to restart one way rather than another, without having to explicitly spell out that invocation in the restart property.

#15 Updated by Nigel Kersten about 4 years ago

  • Status changed from Code Insufficient to Needs More Information
  • Assignee changed from Chuck McIntyre to James Turnbull

#16 Updated by Chris Wilson over 2 years ago

As noted on #1014, there are situations where it’s necessary to fully restart Apache (for example, “adding a module”:https://code.google.com/p/modwsgi/issues/detail?id=300), and others where it’s preferable to reload (whenever possible, to avoid aborting open connections).

So there is definitely value in having both options available to us. The question is how to deal with it sanely in Puppet. How does puppet know that restarting Apache removes the need to reload it as well? In my view it’s not terrible to do both. That makes the problem just one of sending an arbitrary message to a resource, whether it’s “restart”, “reload”, “go fish” or whatever, and having the resource know how to deal with that message (e.g. by notifying a separate Exec resource to run).

Also available in: Atom PDF