Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

(#16809) Use only one record_type in cron provider #1216

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Member

stschulte commented Oct 7, 2012

The cronprovider defines two record_types :crontab and :freebsd_special
to handle the following crontab records:

@daily /bin/my_daily_command
0 0 * * * /bin/my_daily_command

This has a few issues:

a)
The merging of the name (the comment before the cronentry itself) and
the environment variables only happens for the :crontab record_type
but not for the :freebsd_special record_type so information may get
lost (redmine ticket #16809)

b)
When parsing an existing record the record_type can be automatically
determined (depending on which regex matched) but when creating a new
entry, the record_type is undefined and the default :crontab is used.
So if a new cronentry is created with the :special property set it is
still treated as a crontab entry (so the to_line hook of the :crontab
record_type is used and not of the :freebsd_special record_type)
which is kind of confusing.

c)
It is really hard to change the time of a cronentry from the special
format to a normal format (or vise versa) e.g. if we already have the
following cronentry

# Puppet Name: foo
* * * * * /bin/foo

and we have defined the following resource

cron { "foo":
  special => daily,
}

the resource is treatd as in sync because the existing cronentry is
parsed as a :crontab record_type and this record_type doesn't define a
:special field that could be out of sync

This patch now only uses one record_type that internally parses the
timefield as either the old format (minute,hour,monthday,month,weekday)
or the special format. This way the above issues go away (but the
different to_line, post_parse hooks get a bit more difficult to handle
both entries)

= NOTE
After I'm done I'm not so conviced anymore that this was the best approach. I may try the other route: Patching the parsedfile provider to be able to migrate an existing record from one record_type to another and automatically determine the best-fit record_type of newly created records. I'd still like the pull request to be merged because then there are at least some spec tests as a starting point.

(#16809) Use only one record_type in cron provider
The cronprovider defines two record_types :crontab and :freebsd_special
to handle the following crontab records:

    @daily /bin/my_daily_command
    0 0 * * * /bin/my_daily_command

This has a few issues:

a)
The merging of the name (the comment before the cronentry itself) and
the environment variables only happens for the :crontab record_type
but not for the :freebsd_special record_type so information may get
lost (#16809)

b)
When parsing an existing record the record_type can be automatically
determined (depending on which regex matched) but when creating a new
entry the record_type is undefined and the default :crontab is used.
So if a new cronentry is created with the :special property set it is
still treated as a crontab entry (so the to_line hook of the :crontab
record_type is used and not of the :freebsd_special record_type)
which is kind of confusing.

c)
It is really hard to change the time of a cronentry from the special
format to a normal format (or vise versa) e.g. if we already have the
following cronentry

    # Puppet Name: foo
    * * * * * /bin/foo

and we have defined the following resource

    cron { "foo":
      special => daily,
    }

the resource is treatd as in sync because the existing cronentry is
parsed as a :crontab record_type and this record_type doesn't define a
:special field that could be out of sync

This patch now only uses one record_type that internally parses the
timefield as either the old format (minute,hour,monthday,month,weekday)
or the special format. This way the above issues go away (but the
different to_line, post_parse hooks get a bit more difficult to handle
both entries)
Member

stschulte commented Oct 20, 2012

forget to mention: The refactor fixes #3047 and #16121

Contributor

ffrank commented Mar 7, 2013

Stefan,

this is cool stuff. I've been looking into similar issues after filing #19552. I haven't gotten close to unifying the record types, and concluded the same thing you did - parsedfile should be empowered to support this type of file better (or, at all - parsed files are not currently expected to contain different looking records with equal semantics).

Still, I wanted to see what your patch does, but this is what I got:

$ crontab -l
# HEADER: This file was autogenerated at Mon Mar 04 22:46:05 +0100 2013 by puppet.
# HEADER: While it can still be managed manually, it is definitely not recommended.
# HEADER: Note particularly that the comments starting with 'Puppet Name' should
# HEADER: not be deleted, as doing so could cause duplicate cron jobs.
# Puppet Name: usual
@reboot /bin/date
ffrank@geras:~/git/puppet$ RUBYLIB=lib puppet apply -e 'cron { "usual": user => "ffrank", hour => 2, ensure => present, command => "/bin/date", }'  
Could not autoload cron: Could not autoload lib/puppet/provider/cron/crontab.rb: ./lib/puppet/provider/cron/crontab.rb:56: syntax error, unexpected kDO_BLOCK, expecting '}'
      Puppet::Type::Cron::ProviderCrontab::TIME_FIELDS.each do |field|
                                                              ^
./lib/puppet/provider/cron/crontab.rb:60: syntax error, unexpected kEND, expecting '}'
./lib/puppet/provider/cron/crontab.rb:74: syntax error, unexpected kDO_BLOCK, expecting '}'
      str += record.values_at(*fields).map do |field|
                                             ^
./lib/puppet/provider/cron/crontab.rb:80: syntax error, unexpected kEND, expecting '}'
      end.join(self.joiner)
         ^ at line 1 on node geras.localdomain

What I do have is a very simple fix for the vanishing name comment here:
https://github.com/ffrank/puppet/tree/ticket/master/19552-cron-special-schedules

I'll let you know if I make any progress on the parsedfile restructuring.

Member

stschulte commented Mar 12, 2013

@ffrank: What ruby version are you using? Your example puppet apply runs just fine on my box (ruby 1.9.3) so I guess I'm doing something here that was not allowed in earlier versions. But I haven't figured that out yet.

@ffrank: I like your solution in the way that it preserves the two different record types. But we still have the problem, that the freebsd_special type is only used when parsing an existing record and the crontab type is always used when writing new records. So the crontab type currently has to handle "normal" cron records as well as freebsd_special records in the to_line hook. In my opinion this is not really intuitive.

To resolve this issue we need a way to set the correct record_type of a resource that needs to be created in order to pick the right to_line hook. I thought about comparing the resource properties with the defined fields of every record type. If the record has e.g. the special property set and the record type does not define such a field (that's true for the crontab record_type) we can eliminate the record_type. We will then hopefully end up with only one valid record_type. But then again this fails because we have those hacky properties like environment that do not have a corresponding field in any record_type.

Or we check the other way around: How may fields of each record_type can be found as properties in the current resource. So if we got the following resource

cron { 'foo':
  command => '/bin/true',
  special => @daily,
}

we'll first check the crontab record type look and check how many of the defined fields (like day, month, monthday, command etc) can be found as properties in the resource (=1). We'll then check the freebsd_special record type look at the defined fields (special, command) and find two corresponding properties in the the resource.

What do you think of that algorithm? Or do you have other suggestions?

Contributor

ffrank commented Mar 13, 2013

@stefanschulte This would not be too bad, but I fear it won't work very
well at the provider level.

On 03/13/2013 12:03 AM, Stefan Schulte wrote:

Or we check the other way around: How may fields of each record_type can
be found as properties in the current resource. So if we got the
following resource

What I perceive as the central problem at hand is the paradigm of
Util::Fileparsing

  1. each line in a file has exactly one record type
  2. each record type can be recognized through regex matching
  3. after identification, all fields of the record type must be parseable
    from the line

The crontab provider has (with little success) until now sidestepped the
issue that a cronjob does not fit this bill. Basically, the paradigm
would require regular crontab entries and those with special schedules
to be different puppet types (which would be horrible in a whole new
range of ways).

So what I make of this is: The fileparsing paradigm must be extended.
There needs to be a notion of record types that can take alternate
shapes, and have optional fields. Once this is available, all the
schedule fields can become optional and all cronjob line can be regarded
equally, either with the special schedule missing or all numeric fields
missing.

Basically, this is a different approach to the fix you have provided
already, but I hope it would be cleaner and lead to the crontab provider
being less of a hack than it currently is.

Member

adrienthebo commented Mar 13, 2013

For what it's worth I've also refactored this pull request to handle the syntax issues; it's at (https://github.com/adrienthebo/puppet/commits/pull-1216).

My $0.02 is that this modification might suffice for now, but it would be much harder to overhaul the fileparsing behavior in the manner described, because of how deep those assumptions run. In general, the ParsedFile provider and everything really touching that makes a lot of assumptions that you have files with regular, one line contents, and getting away from that will be very difficult. I personally gave up and implemented something similar, but much more generic (https://github.com/adrienthebo/puppet-filemapper).

Member

adrienthebo commented Mar 18, 2013

To update this, I'm interested in any further discussion on this PR to figure out the best implementation. However I think the given implementation is pretty good so if there's no more discussion on this in the next few days I plan to merge the fixed up branch of this and call it good.

Member

stschulte commented Mar 18, 2013

While it would be even better if the parsefile provider could handle different record types and the correct transition between them, the crontab provider may be a little hard to start with because it is abusing the parsed file provider so much (like merging multiple lines and records together). So +1 for merging the pull request (+ your fixes)

Contributor

ffrank commented Mar 19, 2013

Agreed. I had a long and hard staring contest with Util::Fileparsing and
concur that it would be Very Hard to make it do what I had hoped.
Looking at your alternative mixin @adrienthebo, my approach also
contradicts the KISS principle.

I subsequently had a closer look at what @stefanschulte did and believe
that this is really the best way to go about it. Period. Another +1 for
merging.

Down the line, one would probably want to write a new cron provider from
scratch, not based on ParsedFile and replacing Util::Fileparsing with
PuppetX::Filemapper. A little OT: Any insight on why this is not (yet?)
mainline, Adrien?

Member

adrienthebo commented Mar 20, 2013

@ffrank I originally wrote the FileMapper mixin a bit over a year when I was working on types and providers for network configuration and I realized that it would be impossible to use the ParsedFile provider. I was part of the Puppet Labs operations team at that time and didn't think that it would be good enough to be merged to core. Depending on interest I could see about getting it merged in for Puppet 3.3 or Puppet 4.

Contributor

slippycheeze commented Mar 20, 2013

@ffrank I originally wrote the FileMapper mixin a bit over a year when I
was working on types and providers for network configuration and I realized
that it would be impossible to use the ParsedFile provider. I was part of
the Puppet Labs operations team at that time and didn't think that it would
be good enough to be merged to core. Depending on interest I could see about
getting it merged in for Puppet 3.3 or Puppet 4.

Might be better to get it published as a module, so that other
components can in turn depend on it - but it can be updated faster,
yeah?

Contributor

ffrank commented Mar 20, 2013

On 03/20/2013 01:40 AM, Daniel Pittman wrote:

Might be better to get it published as a module, so that other
components can in turn depend on it - but it can be updated faster,
yeah?

Well, cron's in the core, so that's one example for things you would not
want to depend on a module.
I've got to admit though, I'm not quite in the loop on how comfortable
module management is by now (for end users), so I'm sort of impartial.

Member

adrienthebo commented Mar 21, 2013

The filemapper mixin is already on the forge (http://forge.puppetlabs.com/adrien/filemapper) which is why I haven't pushed any harder for it to be merged in.

CLA Signed by stschulte on 2010-11-30 21:00:00 -0800

Member

adrienthebo commented Mar 21, 2013

Merged into master as 5b06686.

This should be released in 3.2.0.

Thanks again for the contribution!

-Adrien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment