Puppet

I can’t find the man page where I saw the 32 return value meaning “already mounted” but I can duplicate how the mount command line program does know the difference between a failed mount because it’s already mounted and a failed mount for other reasons (permisssions, no portmap, etc). See the following commands I ran on the same machine as before when trying the same mount command that puppet does (copied from the puppet log)

root@savage-file:/var/log# /bin/mount -o rw,async /mnt/moose_home
mount.nfs: /mnt/moose_home is already mounted or busy
root@savage-file:/var/log# echo $?
32

So mount.nfs is actually getting an EBUSY errno (from the mount syscall) not a EPERM or EINVAL. So it prints the error that it is already mounted, but also has a return value of 32.

Now if I try the exact same mount but give the “remount” option so it tries to remount an existing mount I get success:

root@savage-file:/var/log# /bin/mount -o remount,rw,async /mnt/moose_home
root@savage-file:/var/log# echo $?
0

which would indicate success to Puppet and allow it to mark the nfs partition as in mounted state.

Maybe the linux/mount provider in puppet needs to either check if the partition is already mounted (and then stop trying to mount it) or it should try with the “remount” option first and if that fails (as it is documented in the mount man page to fail with EINVAL if it is attempted on a source that is not already mounted) then it will mount it without the remount option.

Hi, I looked through the code a bit, and it seems that puppet does try to check if the filesystem is mounted first.

I’ve been getting this with 0.25.1 as well, and I think it could be related to SELinux preventing puppet from checking if the filesystem is mounted (I have gotten AVC denials about puppet attempting to redirect output from mount into a temporary file).

Jonathan, do you have SELinux enabled, and can you check if this still happens with it disabled?

I’m not completely sure what the best solution is to this. I guess an SELinux policy fix is in order, but I’m not too familiar with why puppet needs a temporary file to get the output of a command to begin with. There is a comment in util.rb about this:

# There are problems with read blocking with badly behaved children
# read.partialread doesn't seem to capture either stdout or stderr
# We hack around this using a temporary file

# The idea here is to avoid IO#read whenever possible.

it seems that this was added as a somewhat hacky fix for bug #662, so it’s also possible that the fix belongs in puppet.

By the way, this isn’t the first time that we’ve seen bugs involving hanging reads in ruby. I wonder if anything in bug #1963 could be relevant to finding a better solution to bug #662.

I just filed a bug against the puppet SELinux policy in Fedora about this: https://bugzilla.redhat.com/show_bug.cgi?id=546550

Apologies if I jumped the gun a bit and the fix really does belong in puppet.

Here is a potential fix for the the issue. This patch changes the execute function to use a pipe instead of a temporary file for getting command output, which should avoid the SELinux denials. I also haven’t had a chance to test this inside of puppetd yet, so I’m not yet sure that it doesn’t fail as described in bug #662. I’ll get this tested soon, but it would still be good for somebody familiar with the old issue to take a look at this:

http://ricky.fedorapeople.org/0001-Use-a-pipe-instead-of-a-temp-file-for-command-output.patch

My thought on the patch:

• Overall, looks plausible
• The change:

-                puts detail.to_s
+                # Write errors to stderr.
+                $stderr.puts detail.to_s

while arguably “the right way to do it” is inconsistent with all other occurrences and should be rejected on that bases (though I’m going to add that point to my code smell list). * I’m not too crazy about adding all the blank lines. * This is a much further reaching change than the title suggests * I’m also concerned about the #662 question, though not as much as I was at first.

This list started off longer, and shrunk as I dug into the code.

Markus Roberts wrote:

My thought on the patch:

• Overall, looks plausible
• The change: […] while arguably “the right way to do it” is inconsistent with all other occurrences and should be rejected on that bases (though I’m going to add that point to my code smell list).

Ah, the reason I made that change was that I was getting inconsistent output from

Puppet::Util.execute(["does_not_exist"], :failonfail => false)

using the original version versus my version. For some reason, my version would return the “No such file or directory…” error while the current version would return a blank string. I’ll look more into why this happens.

• I’m not too crazy about adding all the blank lines.

I don’t know the first thing about ruby style, so if they’re not helpful, I’m happy to get rid of them :–)

• This is a much further reaching change than the title suggests
• I’m also concerned about the #662 question, though not as much as I was at first.

For what it’s worth, I’ve finished my testing on Fedora 12, and I was able to reproduce the issue and confirm that the patch fixes it for me. I wasn’t able to reproduce on CentOS 5.4 (most likely due to different SELinux policy versions?), but the patch didn’t seem to break anything either.

Like you said, this seems to be a pretty far-reaching change, so the more scrutiny and testing this gets, the better. Thanks for taking a look at this!

Ricky Zhou wrote:

Ah, the reason I made that change was that I was getting inconsistent output from […] using the original version versus my version. For some reason, my version would return the “No such file or directory…” error while the current version would return a blank string. I’ll look more into why this happens.

OK, it looks like this stems from the use of the exit! function, which closed the file before the error message was properly written into it (using the exit function instead made the output consistent with my non-stderr version). I think it’s safe to consider this a bug in the current implementation that this patch would fix as well.

Here’s a new patch with the stderr change and whitespace changes removed.

http://ricky.fedorapeople.org/0001-Use-a-pipe-instead-of-a-temp-file-for-command-output.patch

Here is a new patch that solves a hanging issue when the pipe’s kernel buffer is filled up.

http://ricky.fedorapeople.org/0001-Use-a-pipe-instead-of-a-temp-file-for-command-output.patch (this is on the 0.25.x branch instead of master this time).

The latest version of the patch (same URL) seems to fix the issue mentioned in #2997. Would it be too late at this point for this to make it into 0.25.2?

The fix for this ticket was reverted for 0.25.3 as it caused numerous problems. See #3033 for the resumed search for a solution to the problem of communicating with SELinux processes.