(#6309) Ensure the correct device is mounted when managing mounts

Previously the mount type would only check if anything was mounted at the
desired point, when 'ensure => mounted' was specified.  Now we check not
only whether something is mounted at the desired point, but also that it
is the thing we wish to be mounted there.

There is also a chance that the mount point directory could be
"automagically" removed for us, when unmounting incorrect devices, so we
attempt to re-create the directory after unmounting to give the mount of
the correct device a better chance at succeeding.

Paired-with: Matt Robinson <matt@puppetlabs.com>
Paired-with: Nick Lewis <nick@puppetlabs.com>
Paired-with: Jesse Wolfe <jesse@puppetlabs.com>

7 files changed, 191 insertions, 105 deletions

diff --git a/spec/fixtures/unit/provider/mount/mount-output.darwin.txt b/spec/fixtures/unit/provider/mount/mount-output.darwin.txt
new file mode 100644
index 000000000..fbb9d9832
--- /dev/null
+++ b/spec/fixtures/unit/provider/mount/mount-output.darwin.txt
@@ -0,0 +1,5 @@
+/dev/disk0s2 on / (hfs, local, journaled)
+devfs on /dev (devfs, local, nobrowse)
+map -hosts on /net (autofs, nosuid, automounted, nobrowse)
+map auto_home on /home (autofs, automounted, nobrowse)
+/dev/disk0s3 on /usr (hfs, local, journaled)
diff --git a/spec/fixtures/unit/provider/mount/mount-output.hp-ux.txt b/spec/fixtures/unit/provider/mount/mount-output.hp-ux.txt
new file mode 100644
index 000000000..477926138
--- /dev/null
+++ b/spec/fixtures/unit/provider/mount/mount-output.hp-ux.txt
@@ -0,0 +1,16 @@
+/ on rpool/ROOT/opensolaris read/write/setuid/devices/dev=2d90002 on Wed Dec 31 16:00:00 1969
+/devices on /devices read/write/setuid/devices/dev=4a00000 on Thu Feb 17 14:34:02 2011
+/dev on /dev read/write/setuid/devices/dev=4a40000 on Thu Feb 17 14:34:02 2011
+/system/contract on ctfs read/write/setuid/devices/dev=4ac0001 on Thu Feb 17 14:34:02 2011
+/proc on proc read/write/setuid/devices/dev=4b00000 on Thu Feb 17 14:34:02 2011
+/etc/mnttab on mnttab read/write/setuid/devices/dev=4b40001 on Thu Feb 17 14:34:02 2011
+/etc/svc/volatile on swap read/write/setuid/devices/xattr/dev=4b80001 on Thu Feb 17 14:34:02 2011
+/system/object on objfs read/write/setuid/devices/dev=4bc0001 on Thu Feb 17 14:34:02 2011
+/etc/dfs/sharetab on sharefs read/write/setuid/devices/dev=4c00001 on Thu Feb 17 14:34:02 2011
+/lib/libc.so.1 on /usr/lib/libc/libc_hwcap1.so.1 read/write/setuid/devices/dev=2d90002 on Thu Feb 17 14:34:14 2011
+/dev/fd on fd read/write/setuid/devices/dev=4d00001 on Thu Feb 17 14:34:18 2011
+/tmp on swap read/write/setuid/devices/xattr/dev=4b80002 on Thu Feb 17 14:34:19 2011
+/var/run on swap read/write/setuid/devices/xattr/dev=4b80003 on Thu Feb 17 14:34:19 2011
+/export on rpool/export read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90006 on Thu Feb 17 14:37:48 2011
+/export/home on rpool/export/home read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90007 on Thu Feb 17 14:37:48 2011
+/rpool on rpool read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90009 on Thu Feb 17 14:37:48 2011
diff --git a/spec/fixtures/unit/provider/mount/mount-output.other.txt b/spec/fixtures/unit/provider/mount/mount-output.other.txt
new file mode 100644
index 000000000..0e4dff0c5
--- /dev/null
+++ b/spec/fixtures/unit/provider/mount/mount-output.other.txt
@@ -0,0 +1,14 @@
+/dev/sda1 on / type ext4 (rw,errors=remount-ro,commit=0)
+proc on /proc type proc (rw,noexec,nosuid,nodev)
+none on /sys type sysfs (rw,noexec,nosuid,nodev)
+fusectl on /sys/fs/fuse/connections type fusectl (rw)
+none on /sys/kernel/debug type debugfs (rw)
+none on /sys/kernel/security type securityfs (rw)
+none on /dev type devtmpfs (rw,mode=0755)
+none on /dev/pts type devpts (rw,noexec,nosuid,gid=5,mode=0620)
+none on /dev/shm type tmpfs (rw,nosuid,nodev)
+none on /var/run type tmpfs (rw,nosuid,mode=0755)
+none on /var/lock type tmpfs (rw,noexec,nosuid,nodev)
+none on /proc/fs/vmblock/mountPoint type vmblock (rw)
+binfmt_misc on /proc/sys/fs/binfmt_misc type binfmt_misc (rw,noexec,nosuid,nodev)
+/dev/sda2 on /usr type ext4 (rw,errors=remount-ro,commit=0)
diff --git a/spec/fixtures/unit/provider/mount/mount-output.solaris.txt b/spec/fixtures/unit/provider/mount/mount-output.solaris.txt
new file mode 100644
index 000000000..477926138
--- /dev/null
+++ b/spec/fixtures/unit/provider/mount/mount-output.solaris.txt
@@ -0,0 +1,16 @@
+/ on rpool/ROOT/opensolaris read/write/setuid/devices/dev=2d90002 on Wed Dec 31 16:00:00 1969
+/devices on /devices read/write/setuid/devices/dev=4a00000 on Thu Feb 17 14:34:02 2011
+/dev on /dev read/write/setuid/devices/dev=4a40000 on Thu Feb 17 14:34:02 2011
+/system/contract on ctfs read/write/setuid/devices/dev=4ac0001 on Thu Feb 17 14:34:02 2011
+/proc on proc read/write/setuid/devices/dev=4b00000 on Thu Feb 17 14:34:02 2011
+/etc/mnttab on mnttab read/write/setuid/devices/dev=4b40001 on Thu Feb 17 14:34:02 2011
+/etc/svc/volatile on swap read/write/setuid/devices/xattr/dev=4b80001 on Thu Feb 17 14:34:02 2011
+/system/object on objfs read/write/setuid/devices/dev=4bc0001 on Thu Feb 17 14:34:02 2011
+/etc/dfs/sharetab on sharefs read/write/setuid/devices/dev=4c00001 on Thu Feb 17 14:34:02 2011
+/lib/libc.so.1 on /usr/lib/libc/libc_hwcap1.so.1 read/write/setuid/devices/dev=2d90002 on Thu Feb 17 14:34:14 2011
+/dev/fd on fd read/write/setuid/devices/dev=4d00001 on Thu Feb 17 14:34:18 2011
+/tmp on swap read/write/setuid/devices/xattr/dev=4b80002 on Thu Feb 17 14:34:19 2011
+/var/run on swap read/write/setuid/devices/xattr/dev=4b80003 on Thu Feb 17 14:34:19 2011
+/export on rpool/export read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90006 on Thu Feb 17 14:37:48 2011
+/export/home on rpool/export/home read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90007 on Thu Feb 17 14:37:48 2011
+/rpool on rpool read/write/setuid/devices/nonbmand/exec/xattr/atime/dev=2d90009 on Thu Feb 17 14:37:48 2011
diff --git a/spec/unit/provider/mount/parsed_spec.rb b/spec/unit/provider/mount/parsed_spec.rb
index 5a1c986b1..78a44322e 100755
--- a/spec/unit/provider/mount/parsed_spec.rb
+++ b/spec/unit/provider/mount/parsed_spec.rb
@@ -130,7 +130,7 @@ describe provider_class do
       mount.stubs(:mountcmd) # just so we don't actually try to mount anything
 
       mount.expects(:flush)
-      mount.mount
+      mount.mount!
     end
   end
 
@@ -176,7 +176,7 @@ describe provider_class do
 
     it "should determine that the root fs is mounted" do
       @provider_class.prefetch("/" => @mount)
-      @mount.provider.should be_mounted
+      @mount.provider.should be_anything_mounted
     end
   end
 
diff --git a/spec/unit/provider/mount_spec.rb b/spec/unit/provider/mount_spec.rb
index f567a4a40..1f2501765 100755
--- a/spec/unit/provider/mount_spec.rb
+++ b/spec/unit/provider/mount_spec.rb
@@ -2,28 +2,30 @@
 
 require File.dirname(__FILE__) + '/../../spec_helper'
 
+require 'puppet_spec/files'
 require 'puppet/provider/mount'
 
 describe Puppet::Provider::Mount do
-  before :each do
-    @mounter = Object.new
-    @mounter.extend(Puppet::Provider::Mount)
+  include PuppetSpec::Files
 
+  before :each do
     @name = "/"
 
-    @resource = stub 'resource'
-    @resource.stubs(:[]).with(:name).returns(@name)
+    @resource = Puppet::Type.type(:mount).new(
+      :name => '/',
+      :device => '/dev/sda1',
+      :target => tmpfile("mount_provider")
+    )
 
-    @mounter.stubs(:resource).returns(@resource)
+    @mounter = Puppet::Type.type(:mount).defaultprovider().new(@resource)
   end
 
-  describe Puppet::Provider::Mount, " when mounting" do
-
+  describe "when calling mount!" do
     it "should use the 'mountcmd' method to mount" do
       @mounter.stubs(:options).returns(nil)
       @mounter.expects(:mountcmd)
 
-      @mounter.mount
+      @mounter.mount!
     end
 
     it "should flush before mounting if a flush method exists" do
@@ -32,114 +34,169 @@ describe Puppet::Provider::Mount do
       @mounter.stubs(:mountcmd)
       @mounter.stubs(:options).returns(nil)
 
-      @mounter.mount
+      @mounter.mount!
     end
 
     it "should add the options following '-o' if they exist and are not set to :absent" do
       @mounter.stubs(:options).returns("ro")
       @mounter.expects(:mountcmd).with { |*ary| ary[0] == "-o" and ary[1] == "ro" }
 
-      @mounter.mount
+      @mounter.mount!
     end
 
     it "should specify the filesystem name to the mount command" do
       @mounter.stubs(:options).returns(nil)
       @mounter.expects(:mountcmd).with { |*ary| ary[-1] == @name }
 
-      @mounter.mount
+      @mounter.mount!
     end
   end
 
-  describe Puppet::Provider::Mount, " when remounting" do
-
+  describe "when remounting" do
     it "should use '-o remount' if the resource specifies it supports remounting" do
       @mounter.stubs(:info)
-      @resource.stubs(:[]).with(:remounts).returns(:true)
+      @resource[:remounts] = true
       @mounter.expects(:mountcmd).with("-o", "remount", @name)
       @mounter.remount
     end
 
     it "should unmount and mount if the resource does not specify it supports remounting" do
       @mounter.stubs(:info)
-      @resource.stubs(:[]).with(:remounts).returns(false)
+      @resource[:remounts] = false
       @mounter.expects(:unmount)
       @mounter.expects(:mount)
       @mounter.remount
     end
 
     it "should log that it is remounting" do
-      @resource.stubs(:[]).with(:remounts).returns(:true)
+      @resource[:remounts] = true
       @mounter.stubs(:mountcmd)
       @mounter.expects(:info).with("Remounting")
       @mounter.remount
     end
   end
 
-  describe Puppet::Provider::Mount, " when unmounting" do
-
+  describe "when unmounting" do
     it "should call the :umount command with the resource name" do
       @mounter.expects(:umount).with(@name)
       @mounter.unmount
     end
   end
 
-  describe Puppet::Provider::Mount, " when determining if it is mounted" do
-
-    it "should parse the results of running the mount command with no arguments" do
-      Facter.stubs(:value).returns("whatever")
-      @mounter.expects(:mountcmd).returns("")
-
-      @mounter.mounted?
-    end
-
-    it "should match ' on /private/var/automount<name>' if the operating system is Darwin" do
-      Facter.stubs(:value).with("operatingsystem").returns("Darwin")
-      @mounter.expects(:mountcmd).returns("/dev/whatever on /private/var/automount/\ndevfs on /dev")
-
-      @mounter.should be_mounted
+  %w{Darwin Solaris HP-UX AIX Other}.each do |platform|
+    describe "on #{platform}" do
+      before :each do
+        case platform
+        when 'Darwin'
+          mount_fixture = 'mount-output.darwin.txt'
+          @mount_device = '/dev/disk0s3'
+          @mount_point = '/usr'
+        when 'Solaris'
+          mount_fixture = 'mount-output.solaris.txt'
+          @mount_device = 'swap'
+          @mount_point = '/tmp'
+        when 'HP-UX'
+          mount_fixture = 'mount-output.hp-ux.txt'
+          @mount_device = 'swap'
+          @mount_point = '/tmp'
+        when 'AIX'
+          mount_fixture = 'mount-output.aix.txt'
+          @mount_device = '/dev/hd2'
+          @mount_point = '/usr'
+        when 'Other'
+          mount_fixture = 'mount-output.other.txt'
+          @mount_device = '/dev/sda2'
+          @mount_point = '/usr'
+        end
+        @mount_data = File.read(File.join(File.dirname(__FILE__), '..', '..', 'fixtures', 'unit', 'provider', 'mount', mount_fixture))
+        Facter.stubs(:value).with("operatingsystem").returns(platform)
+      end
+
+      describe "when the correct thing is mounted" do
+        before :each do
+          @mounter.expects(:mountcmd).returns(@mount_data)
+          @resource.stubs(:[]).with(:name).returns(@mount_point)
+          @resource.stubs(:[]).with(:device).returns(@mount_device)
+        end
+
+        it "should say anything_mounted?" do
+          @mounter.should be_anything_mounted
+        end
+
+        it "should say correctly_mounted?" do
+          @mounter.should be_correctly_mounted
+        end
+      end
+
+      describe "when the wrong thing is mounted" do
+        before :each do
+          @mounter.expects(:mountcmd).returns(@mount_data)
+          @resource.stubs(:[]).with(:name).returns(@mount_point)
+          @resource.stubs(:[]).with(:device).returns('/dev/bogus/thing')
+        end
+
+        it "should say anything_mounted?" do
+          @mounter.should be_anything_mounted
+        end
+
+        it "should not say correctly_mounted?" do
+          @mounter.should_not be_correctly_mounted
+        end
+      end
+
+      describe "when nothing is mounted" do
+        before :each do
+          @mounter.expects(:mountcmd).returns(@mount_data)
+          @resource.stubs(:[]).with(:name).returns('/bogus/location')
+          @resource.stubs(:[]).with(:device).returns(@mount_device)
+        end
+
+        it "should not say anything_mounted?" do
+          @mounter.should_not be_anything_mounted
+        end
+
+        it "should not say correctly_mounted?" do
+          @mounter.should_not be_correctly_mounted
+        end
+      end
     end
+  end
 
-    it "should match ' on <name>' if the operating system is Darwin" do
-      Facter.stubs(:value).with("operatingsystem").returns("Darwin")
-      @mounter.expects(:mountcmd).returns("/dev/disk03 on / (local, journaled)\ndevfs on /dev")
-
-      @mounter.should be_mounted
-    end
+  describe "when mounting a device" do
+    it "should not mount! or unmount anything when the correct device is mounted" do
+      @mounter.stubs(:correctly_mounted?).returns(true)
 
-    it "should match '^<name> on' if the operating system is Solaris" do
-      Facter.stubs(:value).with("operatingsystem").returns("Solaris")
-      @mounter.expects(:mountcmd).returns("/ on /dev/dsk/whatever\n/var on /dev/dsk/other")
+      @mounter.expects(:anything_mounted?).never
+      @mounter.expects(:create).once
+      @mounter.expects(:mount!).never
+      @mounter.expects(:unmount).never
+      FileUtils.expects(:mkdir_p).never
 
-      @mounter.should be_mounted
+      @mounter.mount
     end
 
-    it "should match '^<name> on' if the operating system is HP-UX" do
-      Facter.stubs(:value).with("operatingsystem").returns("HP-UX")
-      @mounter.expects(:mountcmd).returns("/ on /dev/dsk/whatever\n/var on /dev/dsk/other")
+    it "should mount the device when nothing is mounted at the desired point" do
+      @mounter.stubs(:correctly_mounted?).returns(false)
+      @mounter.stubs(:anything_mounted?).returns(false)
 
-      @mounter.should be_mounted
-    end
-
-    it "should match mounted devices if the operating system is AIX" do
-      Facter.stubs(:value).with("operatingsystem").returns("AIX")
-      mount_data = File.read(File.join(File.dirname(__FILE__), '..', '..', 'fixtures', 'unit', 'provider', 'mount', 'mount-output.aix.txt'))
-      @mounter.expects(:mountcmd).returns(mount_data)
+      @mounter.expects(:create).once
+      @mounter.expects(:mount!).once
+      @mounter.expects(:unmount).never
+      FileUtils.expects(:mkdir_p).never
 
-      @mounter.should be_mounted
+      @mounter.mount
     end
 
-    it "should match ' on <name>' if the operating system is not Darwin, Solaris, or HP-UX" do
-      Facter.stubs(:value).with("operatingsystem").returns("Debian")
-      @mounter.expects(:mountcmd).returns("/dev/dsk/whatever on / and stuff\n/dev/other/disk on /var and stuff")
+    it "should unmount the incorrect device and mount the correct device" do
+      @mounter.stubs(:correctly_mounted?).returns(false)
+      @mounter.stubs(:anything_mounted?).returns(true)
 
-      @mounter.should be_mounted
-    end
-
-    it "should not be considered mounted if it did not match the mount output" do
-      Facter.stubs(:value).with("operatingsystem").returns("Debian")
-      @mounter.expects(:mountcmd).returns("/dev/dsk/whatever on /something/else and stuff\n/dev/other/disk on /var and stuff")
+      @mounter.expects(:create).once
+      @mounter.expects(:mount!).once
+      @mounter.expects(:unmount).once
+      FileUtils.expects(:mkdir_p).with(@name).returns(true)
 
-      @mounter.should_not be_mounted
+      @mounter.mount
     end
   end
 end
diff --git a/spec/unit/type/mount_spec.rb b/spec/unit/type/mount_spec.rb
index 0d74042e3..c6d2b5ba0 100755
--- a/spec/unit/type/mount_spec.rb
+++ b/spec/unit/type/mount_spec.rb
@@ -74,8 +74,7 @@ describe Puppet::Type.type(:mount)::Ensure do
     end
   end
 
-  describe Puppet::Type.type(:mount)::Ensure, "when retrieving its current state" do
-
+  describe "when retrieving its current state" do
     it "should return the provider's value if it is :absent" do
       @provider.expects(:ensure).returns(:absent)
       @ensure.retrieve.should == :absent
@@ -83,28 +82,27 @@ describe Puppet::Type.type(:mount)::Ensure do
 
     it "should return :mounted if the provider indicates it is mounted and the value is not :absent" do
       @provider.expects(:ensure).returns(:present)
-      @provider.expects(:mounted?).returns(true)
+      @provider.expects(:correctly_mounted?).returns(true)
       @ensure.retrieve.should == :mounted
     end
 
     it "should return :unmounted if the provider indicates it is not mounted and the value is not :absent" do
       @provider.expects(:ensure).returns(:present)
-      @provider.expects(:mounted?).returns(false)
+      @provider.expects(:correctly_mounted?).returns(false)
       @ensure.retrieve.should == :unmounted
     end
   end
 
-  describe Puppet::Type.type(:mount)::Ensure, "when changing the host" do
-
+  describe "when changing the host" do
     it "should destroy itself if it should be absent" do
-      @provider.stubs(:mounted?).returns(false)
+      @provider.stubs(:anything_mounted?).returns(false)
       @provider.expects(:destroy)
       @ensure.should = :absent
       @ensure.sync
     end
 
     it "should unmount itself before destroying if it is mounted and should be absent" do
-      @provider.expects(:mounted?).returns(true)
+      @provider.expects(:anything_mounted?).returns(true)
       @provider.expects(:unmount)
       @provider.expects(:destroy)
       @ensure.should = :absent
@@ -113,9 +111,9 @@ describe Puppet::Type.type(:mount)::Ensure do
 
     it "should create itself if it is absent and should be defined" do
       @provider.stubs(:ensure).returns(:absent)
-      @provider.stubs(:mounted?).returns(true)
+      @provider.stubs(:anything_mounted?).returns(true)
 
-      @provider.stubs(:mounted?).returns(false)
+      @provider.stubs(:anything_mounted?).returns(false)
       @provider.expects(:create)
       @ensure.should = :defined
       @ensure.sync
@@ -123,7 +121,7 @@ describe Puppet::Type.type(:mount)::Ensure do
 
     it "should not unmount itself if it is mounted and should be defined" do
       @provider.stubs(:ensure).returns(:mounted)
-      @provider.stubs(:mounted?).returns(true)
+      @provider.stubs(:anything_mounted?).returns(true)
 
       @provider.stubs(:create)
       @provider.expects(:mount).never
@@ -134,7 +132,7 @@ describe Puppet::Type.type(:mount)::Ensure do
 
     it "should not mount itself if it is unmounted and should be defined" do
       @provider.stubs(:ensure).returns(:unmounted)
-      @provider.stubs(:mounted?).returns(false)
+      @provider.stubs(:anything_mounted?).returns(false)
 
       @ensure.stubs(:syncothers)
       @provider.stubs(:create)
@@ -146,7 +144,7 @@ describe Puppet::Type.type(:mount)::Ensure do
 
     it "should unmount itself if it is mounted and should be unmounted" do
       @provider.stubs(:ensure).returns(:present)
-      @provider.stubs(:mounted?).returns(true)
+      @provider.stubs(:anything_mounted?).returns(true)
 
       @ensure.stubs(:syncothers)
       @provider.expects(:unmount)
@@ -154,34 +152,14 @@ describe Puppet::Type.type(:mount)::Ensure do
       @ensure.sync
     end
 
-    it "should create and mount itself if it does not exist and should be mounted" do
-      @provider.stubs(:ensure).returns(:absent)
-      @provider.stubs(:mounted?).returns(false)
-      @provider.expects(:create)
-      @ensure.stubs(:syncothers)
-      @provider.expects(:mount)
-      @ensure.should = :mounted
-      @ensure.sync
-    end
-
-    it "should mount itself if it is present and should be mounted" do
+    it "should ask the provider to mount itself" do
       @provider.stubs(:ensure).returns(:present)
-      @provider.stubs(:mounted?).returns(false)
       @ensure.stubs(:syncothers)
       @provider.expects(:mount)
       @ensure.should = :mounted
       @ensure.sync
     end
 
-    it "should create but not mount itself if it is absent and mounted and should be mounted" do
-      @provider.stubs(:ensure).returns(:absent)
-      @provider.stubs(:mounted?).returns(true)
-      @ensure.stubs(:syncothers)
-      @provider.expects(:create)
-      @ensure.should = :mounted
-      @ensure.sync
-    end
-
     it "should be insync if it is mounted and should be defined" do
       @ensure.should = :defined
       @ensure.safe_insync?(:mounted).should == true
@@ -203,17 +181,16 @@ describe Puppet::Type.type(:mount)::Ensure do
     end
   end
 
-  describe Puppet::Type.type(:mount), "when responding to events" do
-
+  describe "when responding to events" do
     it "should remount if it is currently mounted" do
-      @provider.expects(:mounted?).returns(true)
+      @provider.expects(:anything_mounted?).returns(true)
       @provider.expects(:remount)
 
       @mount.refresh
     end
 
     it "should not remount if it is not currently mounted" do
-      @provider.expects(:mounted?).returns(false)
+      @provider.expects(:anything_mounted?).returns(false)
       @provider.expects(:remount).never
 
       @mount.refresh
@@ -241,7 +218,8 @@ describe Puppet::Type.type(:mount), "when modifying an existing mount entry" do
       @mount[param] = value
     end
 
-    @mount.provider.stubs(:mounted?).returns true
+    @mount.provider.stubs(:anything_mounted?).returns true
+    @mount.provider.stubs(:correctly_mounted?).returns true
 
     # stub this to not try to create state.yaml
     Puppet::Util::Storage.stubs(:store)