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

Bug #593

Puppet's cron type struggles with vixie-cron

Added by Peter Abrahamsen over 7 years ago. Updated over 1 year ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:cron
Target version:3.2.0
Affected Puppet version:0.25.1 Branch:https://github.com/puppetlabs/puppet/pull/1479
Keywords:cronfixit

We've Moved!

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

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

After making a few changes to my cron job in my manifest, my crontab begins to look like:

# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Thu Apr 12 12:16:01 2007)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)
# HEADER This file was autogenerated at Thu Apr 12 12:16:01 -0700 2007 by puppet.
# HEADER While it can still be managed manually, it is definitely notrecommended.
# HEADER Note particularly that the comments starting with 'Puppet Name' should
# HEADER not be deleted, as doing so could cause duplicate cron jobs.
# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Wed Apr 11 16:42:40 2007)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)
# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Tue Apr 10 13:49:45 2007)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)
# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Thu Apr  5 17:36:42 2007)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)
# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (/tmp/crontab.XXXXWxMPKB installed on Thu Apr  5 17:08:07 2007)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)
# Puppet Name: websync
*/10 * * * * 

If puppet would put its notice just before the first job it manages, I think it would avoid this problem.


Related issues

Blocks Puppet - Bug #6990: Rehabilitate cron provider Accepted 04/06/2011

History

#1 Updated by Luke Kanies over 7 years ago

  • Status changed from 1 to 2

#2 Updated by Redmine Admin over 6 years ago

  • Status changed from 2 to Accepted

#3 Updated by Luke Kanies almost 6 years ago

  • Affected Puppet version set to 0.24.7

I swear we fixed this problem a long time ago (which is appropriate, given how old the ticket is), but this will have to wait until a later release, when we can assess all of the cron tickets at once.

#4 Updated by Luke Kanies over 5 years ago

  • Target version changed from 0.25.0 to 2.6.0

Yes, I’m bumping again.

Can anyone by chance test this, to make sure it’s still broken? I’m so convinced I fixed it already.

#5 Updated by Alexey Lapitsky over 5 years ago

0.24.7 is affected

#6 Updated by Dan Carley almost 5 years ago

0.25.0 also affected.

#7 Updated by James Turnbull almost 5 years ago

  • Affected Puppet version changed from 0.24.7 to 0.25.1

#8 Updated by James Turnbull almost 5 years ago

  • Target version changed from 2.6.0 to 2.7.x

#9 Updated by Luke Kanies over 4 years ago

  • Assignee deleted (Luke Kanies)

#10 Updated by Nigel Kersten about 4 years ago

  • Status changed from Accepted to Needs More Information
  • Assignee set to Nigel Kersten

Can we see an example manifest that produces this issue so we can be sure we’re fixing it?

It seems surprising that the cron provider would be so broken with vixie-cron.

#11 Updated by Nigel Kersten about 4 years ago

I’m particularly interested in hearing whether the problem is with new lines and thus a duplicate of #656.

#12 Updated by Alan Barrett about 4 years ago

What’s happening here is that both puppet and cron want their special comments to be at the top of the file.

If puppet finds that its special comment is somewhere in the file but not at the top, then it moves the comment to the top.

If cron finds that its special comment is not at the top of the file, then it inserts a new comment at the top, regardless of whether or not the comment already existed elsewhere in the file.

Interaction between these two processes causes multiple copies of cron’s special comment to appear.

#13 Updated by Nigel Kersten about 4 years ago

  • Status changed from Needs More Information to Accepted
  • Assignee deleted (Nigel Kersten)
  • Keywords set to cronfixit

We have a bunch of cron issues. I’m going to tag all the open cron issues with ‘cronfixit’ and we’ll try to address them all at once.

#14 Updated by Felix Frank over 2 years ago

Nigel Kersten wrote:

We have a bunch of cron issues. I’m going to tag all the open cron issues with ‘cronfixit’ and we’ll try to address them all at once.

So, I take it that’s not working out too well for you? ;-S

#15 Updated by Felix Frank over 2 years ago

Uhm, snideness aside, I’ve gone ahead and proposed a fix in https://github.com/ffrank/puppet/tree/ticket/2.7.x/593-vixie-cron-headers.

So, no hard feelings?

#16 Updated by Nigel Kersten over 2 years ago

yeah. clearly that hasn’t worked out too well :( No hard feelings at all :)

Can you do this as a pull request Felix? Looking at your fixes now.

#17 Updated by Felix Frank over 2 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

Done (I assume that is what Status – In Topic Branch… does).

Hadn’t even realized I could do this as an outside contributor. Thanks for the pointer, Nigel!

Just rebased: https://github.com/ffrank/puppet/tree/ticket/2.7.x/593-vixie-cron-headers

Although now, github indicates my history is diverged from the original? Uhm. Hope I didn’t make a mess.

Note on tests: There are none ;) I didn’t find any for the actual crontabs to begin with, so I didn’t bother starting now. (Honestly, the whole issue of tests still baffles me, it’s harder than tweaking the actual providers.)

#18 Updated by eric sorenson over 2 years ago

Hey, great job Felix. Can you please send a pull request to puppetlabs/puppet from your branch?

#19 Updated by Felix Frank about 2 years ago

eric sorenson wrote:

Hey, great job Felix. Can you please send a pull request to puppetlabs/puppet from your branch?

Eric,

just got around to doing just that. I hadn’t been aware that puppetlabs did this simply via github :S

Hope it gets merged soon, thanks for endorsing.

#20 Updated by Felix Frank about 2 years ago

Guys,

there seem to be oddities with the patch, I don’t understand them exactly though.

I’m grateful for any feedback to https://github.com/puppetlabs/puppet/pull/1268 to speed things up.

TIA, Felix

#21 Updated by Anonymous almost 2 years ago

  • Target version deleted (2.7.x)

#22 Updated by Felix Frank almost 2 years ago

So, err, does that mean 2.7.x is no longer receiving patches?

I shall go on a limb, rebase to 3.x and apply what i assume to be a fix to Jeff’s remarks on the pull request.

In all, I have to admit that I’m a bit underwhelmed by the amount of feedback given to both this issue and the pull request itself.

#23 Updated by Jeff McCune almost 2 years ago

  • Assignee set to Jeff McCune

#24 Updated by Jeff McCune almost 2 years ago

Felix Frank wrote:

So, err, does that mean 2.7.x is no longer receiving patches?

Hey Felix, 2.7.x is receiving patches, but only of the “this fixes a bug that causes a crash” or “this fixes a security vulnerability” variety. Bugs like this should target master please.

I shall go on a limb, rebase to 3.x and apply what i assume to be a fix to Jeff’s remarks on the pull request.

When you rebase, please rebase against master and not 3.x. We actually don’t have a 3.x branch right now, just a 3.0.x and a master branch since we’re really close to cutting a 3.1.0 RC from the master branch. Once the pull request is based on master we’ll figure out the exact version to merge into and then merge up from there.

In all, I have to admit that I’m a bit underwhelmed by the amount of feedback given to both this issue and the pull request itself.

I’m really sorry about this, please ping me directly if you find yourself blocked waiting on feedback from us. I’m trying to improve our tooling in an effort to reduce the overall lag time between a contribution and feedback on the contribution. Things like Trello and Travis CI are helping us reduce this time, but I think this issue fell through the cracks a bit.

-Jeff

#25 Updated by Jeff McCune almost 2 years ago

  • Status changed from In Topic Branch Pending Review to Tests Insufficient
  • Assignee deleted (Jeff McCune)

I commented on the pull request, but doing so here as well:

This looks OK, I just need to figure out how to get some test coverage added before we merge this in. Before we merge this, please also make sure the change in behavior regarding the parsed file “API” is documented. That is, parse file providers may implement the native_header_regex and drop_native_header methods.

Felix, would you like to take a stab at adding test coverage and YARD documentation to the parsed file base class and to the crontab provider? I’m happy to help out if you need some help. I think the best place to get started with the behavior tests are in spec/unit/provider/parsedfile_spec.rb, spec/shared_behaviours/all_parsedfile_providers.rb, and spec/unit/provider/cron/crontab_spec.rb.

Next actions are to add API documentation and behavior examples, we can’t merge this without those things in place.

#26 Updated by Felix Frank almost 2 years ago

Jeff,

thanks for following up on both ends. I’m still not sure if I’m on the right track wrt. variable scoping, but I’ll just try and access everything via “self.” if possible.

I’d love to introduce tests, as a matter of fact. I had a hard time getting my head around the whole topic, so the pointers are most appreciated!

API documentation resides in rdoc, right? I shall try and update the pertaining sections.

#27 Updated by Jeff McCune almost 2 years ago

Felix Frank wrote:

Jeff,

thanks for following up on both ends. I’m still not sure if I’m on the right track wrt. variable scoping, but I’ll just try and access everything via “self.” if possible.

In these examples the methods you’ve added are class methods and not instance methods. This should help understand how the scope comes into place. This tutorial seems to be a pretty good description of the various scope considerations in Ruby: http://marakana.com/bookshelf/ruby_tutorial/scope.html

I’d love to introduce tests, as a matter of fact. I had a hard time getting my head around the whole topic, so the pointers are most appreciated!

Sure thing, please start with those files I mentioned and see if you can add some examples to the existing example groups contained in those files. As you run into questions I’ll be happy to review the code if you have it pushed to a topic branch or something.

API documentation resides in rdoc, right? I shall try and update the pertaining sections.

We’re actually using YARD for API documentation, not rdoc. Since you’re adding a method that parse file providers are meant to override please include the @api public keyword. A list of YARD tags is available at: http://rubydoc.info/docs/yard/file/docs/Tags.md Finally, you might want to look at some of the recent commits to Facter and Puppet from Henrik and Patrick for some recent examples of patches that add YARD documentation to the codebase.

#28 Updated by Felix Frank almost 2 years ago

Great, thanks again. I shall follow up outside this bug.

As an aside on topic, this bug does not appear to affect newer cron versions. Affected header:

# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Tue May 22 11:53:15 2012)
# (Cron version V5.0 -- $Id: crontab.c,v 1.12 2004/01/23 18:56:42 vixie Exp $)

Unaffected (as in Debian sid):

# DO NOT EDIT THIS FILE - edit the master and reinstall.
# (- installed on Tue Jan  8 18:08:51 2013)
# (Cron version -- $Id: crontab.c,v 2.13 1994/01/17 03:20:37 vixie Exp $)

That being said, the above example is from SLES11 SP2, which is still a supported platform, so it still warrants fixing IMO.

#29 Updated by Adrien Thebo almost 2 years ago

  • Status changed from Tests Insufficient to In Topic Branch Pending Review
  • Target version set to 3.x
  • Branch set to https://github.com/puppetlabs/puppet/pull/1479

#30 Updated by Adrien Thebo almost 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version changed from 3.x to 3.2.0

Merged into master as a3bee25.

This should be released in 3.2.0.

Thanks again for the contribution!

-Adrien

#31 Updated by Matthaus Owens over 1 year ago

  • Status changed from Merged - Pending Release to Closed

Released in Puppet 3.2.0-rc1

#32 Updated by Matthaus Owens over 1 year ago

Released in Puppet 3.2.0-rc1

Also available in: Atom PDF