In my experience this can hit you quite badly, as RHEL (and its derivates) for example have a symlink for grub.conf. While it worked imho earlier, it doesn’t work anymore nowadays and your manifests will constantly update the wrong config file, also other tools that relay on /etc/grub.conf being a symlink will start to fail.

Dominic, does your assigning this to yourself indicate that you’re working on a patch for this issue? We’ve had some more people run into this issue and are looking into it ourselves.

Max Martin wrote:

Dominic, does your assigning this to yourself indicate that you’re working on a patch for this issue? We’ve had some more people run into this issue and are looking into it ourselves.

Yes, I was going to prepare a patch this week as I’m working on similar things. I’d be equally happy to review somebody else’s if you have somebody ready to work on it.

Just to give an update, I’m actively working on this Dominic so please ping me in #puppet if you start working on this Friday. I’d hate to have you duplicate work.

My strategy is to use Pathname#realpath and Tempfile. I expect the biggest chunk of time on this fix to be an acceptance test.

-Jeff

Update on save_mode¶

Unfortunately the needs_to_run method currently has the behavior of producing a *.augnew file for each file being managed by Augeas. This side effect is relied upon by the sync method of the returns property in the augeas type. In order to implement the strategy Dominic proposed of using the SAVE_BACKUP augeas mode I believe we need to significantly refactor the augeas provider to avoid this side effect and instead implement a more direct method of implementing the type’s retrieve and sync methods.

puppet-dev conversation¶

Dominic: jmccune: around?
[10:56pm] jmccune: Dominic: Yessir
[10:56pm] jmccune: What's up?
[10:56pm] Dominic: ah good.  Just caught your update before I was heading off.
[10:57pm] jmccune: Update on what?
[10:57pm] Dominic: #11988
[10:57pm] gepetto: Dominic: #11988 is http://projects.puppetlabs.com/issues/show/11988 "Puppet - Bug #11988: Augeas provider can clobber symlinks. It has a status of Accepted and is assigned to Dominic Cleal"
[10:57pm] jmccune: Ah yeah
[10:57pm] jmccune: There's a bit more information, the proposed work around can't work unless the grub lens in our PE packages are updated
[10:57pm] Dominic: I think you could squeeze in the use of SAVE_BACKUP without significant refactoring, though I'll admit none of it's very pretty
[10:58pm] Dominic: oh?
[10:58pm] jmccune: Well, the grub lens doesn't work with /boot/grub/grub.conf
[10:58pm] jmccune: Only /etc/grub.conf
[10:59pm] jmccune: Dominic: Yeah, we could but this is something we'd like to fix
[10:59pm] lak left the chat room. (Quit: Leaving.)
[10:59pm] Dominic: (the provider used to fit the retrieve/sync pattern much better, then it was optimised as part of the #2728 ticket to rely on this side effect)
[10:59pm] gepetto: Dominic: #2728 is http://projects.puppetlabs.com/issues/show/2728 "Puppet - Feature #2728: augeas show print file changes as applicable via --show_diff. It has a status of Closed and is assigned to Bryan Kearney"
[10:59pm] jmccune: While we're in there.
[10:59pm] jmccune: Right, that makes sense
[11:00pm] Dominic: the lens will work with just /etc/grub.conf, it'll follow through and load whatever it's pointing at
[11:00pm] jmccune: It just seems like the right thing to do is use NEWFILE for the retrieve method
[11:00pm] jmccune: And BACKUP for the sync method
[11:01pm] jmccune: Dominic: That's correct, but the augnew file is in the wrong directory then
[11:01pm] Dominic: agreed, that's more natural, though it means running the provider twice - which can be slow
[11:01pm] jmccune: Dominic: We want to get away from only running the provider once.
[11:01pm] jmccune: Regardless
[11:01pm] Dominic: ok
[11:02pm] jmccune: Because if we don't we're going to have to re-implement the Augeas permissions management
[11:02pm] jmccune: As you mentioned
[11:02pm] jmccune: It would be ideal
[11:02pm] jmccune: If we could always use the new file save mode
[11:03pm] jmccune: And rely on augeas to set the permissions of the new file to match the source file.
[11:03pm] jmccune: Then we can simply rename it into place from Puppet
[11:03pm] jmccune: From the sync method
[11:03pm] jmccune: We can't do that today because the rename would cross a filesystem boundary
[11:05pm] Dominic: yup, though if BACKUP was used from the retrieve method, then the file itself would be updated solely by Augeas, no rename required
[11:05pm] jmccune: Dominic: But that has the obvious problem of making changes while trying to figure out if changes need to be made
[11:06pm] jmccune: Or do I misunderstand backup?
[11:06pm] jmccune: Doesn't it store the original file in the backup and modify the target file?
[11:06pm] Dominic: Augeas will only write and "save" the file if it's changed
[11:06pm] Dominic: no, you understand backup correctly, it's just that Augeas will only save if needed
[11:06pm] jmccune: Dominic: Right, so what if we're running in Puppet noon mode?
[11:06pm] jmccune: s/noon/noop/
[11:06pm] Dominic: it's ugly, but you could flip the logic to use NEWFILE if resource.noop?
[11:07pm] jmccune: Right, that's the point when I said to myself "This feels like a refactor"
[11:07pm] Dominic:
[11:07pm] jmccune:
[11:07pm] Dominic: you might want to look at the pre-2728 code, that was closer to Puppet's normal retrieve/sync style
[11:08pm] Dominic: it used to use Augeas' noop mode (introduced for Puppet)

Even if you need to make changes between two calls to @aug.save, you should keep the handle open and call @aug.load to reset the tree to its pristine state. @aug.load goes through great length to not reload files unnecessarily (if the mtime of a file hasn’t changed since the last load, and nothing in the corresponding tree was touched, @aug.load is a noop, i.e. for unchanged file all that happens is a stat)

In addition, the tree should be initialized with as few files as possible; IIRC, Dominic wrote some clever code to deduce from the context which file needs to be loaded.