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

Bug #2521

file{} should update all timestamps when renaming files from temp file.

Added by R.I. Pienaar over 4 years ago. Updated about 3 years ago.

Status:ClosedStart date:08/10/2009
Priority:NormalDue date:
Assignee:Nigel Kersten% Done:

0%

Category:file
Target version:-
Affected Puppet version:0.24.8 Branch:
Keywords:

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

At present puppet writes new files made with the file resource type using a temp file then renames the temp file. Ruby rename only updates change time not mtime.

This is a problem when interacting with daemons that monitor directories for new files, like /etc/cron.d:

1) puppet creates /etc/cron.d/foo.zxxx 2) crond checks for new files matching its rules, the temp file does not match does not get noticed, crond saves last checked timestamp 3) puppet renames /etc/cron.d/foo.zxxx –> /etc/cron.d/foo and only ctime gets updated not mtime 4) crond checks for new files since last check, finds none.

I think puppet should probably touch both mtime and ctime after renaming a file.

History

#1 Updated by James Turnbull over 4 years ago

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

Luke – I agree this is an issue? Your thoughts?

#2 Updated by Luke Kanies over 4 years ago

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

#3 Updated by Markus Roberts almost 4 years ago

  • Target version set to 49

#4 Updated by R.I. Pienaar almost 4 years ago

Is there any chance this bug can get some love prior to 2.6?

#5 Updated by Kjetil Torgrim Homme almost 4 years ago

After looking at the source code to cronie (the new name for vixie-cron), I think there is something missing in the analysis.

First, a temporary filename foo.xxx will not be ignored directly, it will log that it is skipped due to the user not existing. To avoid this, use a temporary filename starting with “.” or ending with “~” (see not_a_crontab in database.c)

Furthermore, mtime is remembered for each user’s crontab, so even if cron processed foo.xxx, it would not update the mtime remembered for the last version of “foo”.

I haven’t experienced Volcane’s problem myself, but my theory is that the problem only surfaces when multiple crontabs are added in the same second. cron may read the first version, but due to lack of precision, it will not know that the second change with same mtime warrants reading the file afresh. If this is the case, Puppet should batch up changes and write them all at once, or possibly simpler, Puppet should sleep a little between crontab updates.

The best solution would be to use the published API, ie. write to a temporary file and use crontab(1) to install it. On Solaris, cron(1m) does not poll for changes to directories, but crontab(1) sends a notification over a FIFO to the daemon that it should reread crontabs. (Is there Puppet code to handle this today?) I guess that is hard to integrate into ParsedFile, though.

#6 Updated by Kjetil Torgrim Homme almost 4 years ago

Uh, I didn’t notice the bug is about File. Why aren’t you using Cron, Volcane?

#7 Updated by R.I. Pienaar almost 4 years ago

they share the same mechanisms.

I didnt look at the crond source code, my observations was just based on observing the behavior not by looking at the crond code.

#8 Updated by Kjetil Torgrim Homme almost 4 years ago

I don’t think it would hurt anything to call utimes(2) after rename(2), but as far as I can see, it would not solve the problem with cron.

Since there is only one write of a specific file in the /etc/cron.d case, the explanation has to be different. My best shot at it is that cron was started in the same second as the edit was made.

#9 Updated by R.I. Pienaar almost 4 years ago

Kjetil Torgrim Homme wrote:

Since there is only one write of a specific file in the /etc/cron.d case, the explanation has to be different. My best shot at it is that cron was started in the same second as the edit was made.

that’s not the case, crond’s dont restart during the day

#10 Updated by James Turnbull over 3 years ago

  • Target version deleted (49)

#11 Updated by James Turnbull about 3 years ago

Kjetil Torgrim Homme wrote:

I don’t think it would hurt anything to call utimes(2) after rename(2), but as far as I can see, it would not solve the problem with cron.

Doesn’t utime just change atime and mtime? It doesn’t change ctime right?

#12 Updated by Daniel Pittman about 3 years ago

This ticket should be rejected: mtime reflects the time of changes to the content of the file, which rename is not. The fact that only ctime changes is, in fact, courtesy the kernel, not Ruby, which would have to do strange, non-POSIX things to modify that behaviour.

I don’t understand the root cause of the problem, but I certainly think we are better off tracking that down and understanding it, not just papering over it by adding non-POSIX semantics to file manipulation in puppet.

#13 Updated by Kjetil Torgrim Homme about 3 years ago

James Turnbull wrote:

Kjetil Torgrim Homme wrote:

I don’t think it would hurt anything to call utimes(2) after rename(2), but as far as I can see, it would not solve the problem with cron.

Doesn’t utime just change atime and mtime? It doesn’t change ctime right?

all system calls which changes the inode metadata (except atime), update ctime to “now”.

#14 Updated by Kjetil Torgrim Homme about 3 years ago

Daniel Pittman wrote:

This ticket should be rejected: mtime reflects the time of changes to the content of the file, which rename is not. The fact that only ctime changes is, in fact, courtesy the kernel, not Ruby, which would have to do strange, non-POSIX things to modify that behaviour.

rename(2) doesn’t change ctime of the file, but it affects ctime and mtime for the parent directory. utime(2) is part of POSIX, it’s not a strange, non-POSIX things to do. it should not be necessary, though.

I don’t understand the root cause of the problem, but I certainly think we are better off tracking that down and understanding it, not just papering over it by adding non-POSIX semantics to file manipulation in puppet.

I wouldn’t call it non-POSIX semantics, but clearly the daemon in question is making assumptions which do not hold in all POSIX systems. it would be nice if Volcane could identify the errant daemon more accurately so we could investigate the root cause.

#15 Updated by R.I. Pienaar about 3 years ago

Kjetil Torgrim Homme wrote:

I wouldn’t call it non-POSIX semantics, but clearly the daemon in question is making assumptions which do not hold in all POSIX systems. it would be nice if Volcane could identify the errant daemon more accurately so we could investigate the root cause.

I mentioned crond. But really as this ticket has gone for almost a year and a half I dont recall the specifics.

But it will affect many things, if you’re creating files and moving them into place there’s a race condition between your creation time and the time the daemon checked for files matching its expected filename pattern.

#16 Updated by Kjetil Torgrim Homme about 3 years ago

R.I. Pienaar wrote:

Kjetil Torgrim Homme wrote:

I wouldn’t call it non-POSIX semantics, but clearly the daemon in question is making assumptions which do not hold in all POSIX systems. it would be nice if Volcane could identify the errant daemon more accurately so we could investigate the root cause.

I mentioned crond. But really as this ticket has gone for almost a year and a half I dont recall the specifics.

well, “crond” could be a dozen implementations. I looked at cronie and it should handle Puppet’s handling fine.

But it will affect many things, if you’re creating files and moving them into place there’s a race condition between your creation time and the time the daemon checked for files matching its expected filename pattern.

we get a problem if the daemon filters files by comparing the file’s mtime to the directory’s mtime. this is a dubious practice, and it should keep more state to work more reliably (e.g., remembering a timestamp for the last processed file will solve the problem wrt. Puppet. to handle rename(2)d files in general, you need more state, perhaps the way cronie does it.)

#17 Updated by R.I. Pienaar about 3 years ago

Kjetil Torgrim Homme wrote:

we get a problem if the daemon filters files by comparing the file’s mtime to the directory’s mtime. this is a dubious practice, and it should keep more state to work more reliably (e.g., remembering a timestamp for the last processed file will solve the problem wrt. Puppet. to handle rename(2)d files in general, you need more state, perhaps the way cronie does it.)

how will remembering the last processed time help? consider this:

  • puppet writes a temp file
  • cron or whatever checks for files, remembers the timestamp
  • puppet renames the file now it will be included in crons checks for files thats new/changed
  • cron checks for files since timestamp and miss the file because the timestamp is earlier than before as rename doesnt change it

This is exactly where I recall the problem was but dont have the exact details, or version of cron it was or anything more useful.

#18 Updated by Kjetil Torgrim Homme about 3 years ago

R.I. Pienaar wrote:

Kjetil Torgrim Homme wrote:

e.g., remembering a timestamp for the last processed file will solve the problem wrt. Puppet.

how will remembering the last processed time help? consider this:

  • puppet writes a temp file
  • cron or whatever checks for files, remembers the timestamp

such a temp file isn’t “processed”, it is ignored, so the daemon’s internal timestamp isn’t updated. sorry about the unclear terminology.

#19 Updated by Daniel Pittman about 3 years ago

Kjetil Torgrim Homme wrote:

Daniel Pittman wrote:

This ticket should be rejected: mtime reflects the time of changes to the content of the file, which rename is not. The fact that only ctime changes is, in fact, courtesy the kernel, not Ruby, which would have to do strange, non-POSIX things to modify that behaviour.

rename(2) doesn’t change ctime of the file, but it affects ctime and mtime for the parent directory. utime(2) is part of POSIX, it’s not a strange, non-POSIX things to do. it should not be necessary, though.

ctime of the file and mtime of the directory, on Linux 2.6.32; this is explicitly an “implementation” behaviour. The parent directory is required to be updated by POSIX. Thank you for forcing clarification of that.

Meanwhile, indeed, hitting the renamed file with utime is odd – and Ruby 1.8.7 is definitely not doing anything crazy; it just issues a rename syscall as expected.

I don’t understand the root cause of the problem, but I certainly think we are better off tracking that down and understanding it, not just papering over it by adding non-POSIX semantics to file manipulation in puppet.

I wouldn’t call it non-POSIX semantics, but clearly the daemon in question is making assumptions which do not hold in all POSIX systems. it would be nice if Volcane could identify the errant daemon more accurately so we could investigate the root cause.

Since the request was for mtime, and POSIX is silent about the topic of any platform updating mtime on change (plus, it not being the semantics of the field), I would call the request for non-POSIX semantics.

Anyway, I don’t think trying to work around every poorly coded daemon is a long term winner, and still want this rejected.

#20 Updated by Nigel Kersten about 3 years ago

Daniel Pittman wrote:

Anyway, I don’t think trying to work around every poorly coded daemon is a long term winner, and still want this rejected.

I don’t believe we should work around every poorly coded daemon either.

If this is a problem for a common method of interacting with the most common crond (still not sure which one we’re talking about in this thread) then I think we can make an argument for working around those specific issues.

A reproducible case would be brilliant here.

#21 Updated by Nigel Kersten about 3 years ago

  • Status changed from Accepted to Needs Decision

#22 Updated by Nigel Kersten about 3 years ago

  • Assignee set to Nigel Kersten

#23 Updated by Daniel Pittman about 3 years ago

Nigel Kersten wrote:

Daniel Pittman wrote:

Anyway, I don’t think trying to work around every poorly coded daemon is a long term winner, and still want this rejected.

I don’t believe we should work around every poorly coded daemon either.

…and, just to be clear, we are also of the firm opinion that manually touching these times is not going to break other services, like daemons that monitor configuration files and restart every time they change? Not that I had a huge, heavy, painful Java stack that would actually do that, except it failed if you restarted too quickly and all. :)

If this is a problem for a common method of interacting with the most common crond (still not sure which one we’re talking about in this thread) then I think we can make an argument for working around those specific issues.

A reproducible case would be brilliant here.

Well, IMO the cron provider should be rewritten entirely at this point, so most of these issues would just vanish anyhow. Reproduction instructions would certainly help convince me we should special-case this particular “special” daemon.

#24 Updated by Nigel Kersten about 3 years ago

Daniel Pittman wrote:

Nigel Kersten wrote:

I don’t believe we should work around every poorly coded daemon either.

…and, just to be clear, we are also of the firm opinion that manually touching these times is not going to break other services, like daemons that monitor configuration files and restart every time they change? Not that I had a huge, heavy, painful Java stack that would actually do that, except it failed if you restarted too quickly and all. :)

If we’re aiming to just fix the cron situation, then I would expect we would only work around this issue in the cron provider, not at the more general level.

#25 Updated by Nigel Kersten about 3 years ago

  • Status changed from Needs Decision to Closed

I’m closing this because it’s an old bug, we don’t have clear details around the actual situation, and thus the path forward isn’t clear.

Also available in: Atom PDF