(#6632) Adding a new mount no longer causes error with umount

There were two problems:

* In lib/puppet/type/mount.rb, we were calling provider.mounted? to
  determine whether we needed to execute "mount" after updating the
  in-memory fstab record.  This wasn't working properly because
  provider.mounted? makes its decision based on the data stored in the
  in-memory fstab record.  Since the fstab record had just been
  updated, provider.mounted? was incorrectly returning true even
  though the device wasn't actually mounted.  Fixed this by checking
  provider.mounted? before updating the in-memory fstab record.

* Calling mount from this point in lib/puppet/type/mount.rb is
  actually too early, because even though the in-memory fstab record
  has been created, its contents have not been written to `/etc/fstab`
  yet.  Fixed this by storing a :needs_mount entry in the
  property_hash and checking it at the end of the flush() method.

Reviewed-by: Jacob Helwig <jacob@puppetlabs.com>

4 files changed, 14 insertions, 32 deletions

diff --git a/lib/puppet/provider/mount/parsed.rb b/lib/puppet/provider/mount/parsed.rb
index 42e543c15..11c5e21a9 100755
--- a/lib/puppet/provider/mount/parsed.rb
+++ b/lib/puppet/provider/mount/parsed.rb
@@ -97,4 +97,10 @@ Puppet::Type.type(:mount).provide(
     end
     instances
   end
+
+  def flush
+    needs_mount = @property_hash.delete(:needs_mount)
+    super
+    mount if needs_mount
+  end
 end
diff --git a/lib/puppet/type/mount.rb b/lib/puppet/type/mount.rb
index 98a1f2509..5b8c5ca58 100755
--- a/lib/puppet/type/mount.rb
+++ b/lib/puppet/type/mount.rb
@@ -74,12 +74,13 @@ module Puppet
       newvalue(:mounted, :event => :mount_mounted) do
         # Create the mount point if it does not already exist.
         current_value = self.retrieve
+        currently_mounted = provider.mounted?
         provider.create if [nil, :absent, :ghost].include?(current_value)
 
         syncothers
 
         # The fs can be already mounted if it was absent but mounted
-        provider.mount unless provider.mounted?
+        provider.property_hash[:needs_mount] = true unless currently_mounted
       end
 
       # insync: mounted   -> present
diff --git a/spec/integration/provider/mount_spec.rb b/spec/integration/provider/mount_spec.rb
index c28707dd9..518b2956f 100644
--- a/spec/integration/provider/mount_spec.rb
+++ b/spec/integration/provider/mount_spec.rb
@@ -35,6 +35,7 @@ describe "mount provider (integration)" do
         fail "unexpected umount" unless @umount_permitted
         command.length.should == 2
         command[1].should == '/Volumes/foo_disk'
+        @mounted.should == true # "umount" doesn't work when device not mounted (see #6632)
         @mounted = false
         ''
       else
@@ -77,7 +78,7 @@ describe "mount provider (integration)" do
 
   it "should be able to create and mount a brand new mount point" do
     @mounted = false
-    @umount_permitted = true # Work around bug #6632
+    @umount_permitted = true # Work around bug #6633
     run_in_catalog(:mounted)
     @mounted.should == true
     check_fstab
diff --git a/spec/unit/type/mount_spec.rb b/spec/unit/type/mount_spec.rb
index 7f9a0eba6..fdb67f7d5 100755
--- a/spec/unit/type/mount_spec.rb
+++ b/spec/unit/type/mount_spec.rb
@@ -65,7 +65,8 @@ end
 
 describe Puppet::Type.type(:mount)::Ensure do
   before :each do
-    @provider = stub 'provider', :class => Puppet::Type.type(:mount).defaultprovider, :clear => nil, :satisfies? => true, :name => :mock
+    provider_properties = {}
+    @provider = stub 'provider', :class => Puppet::Type.type(:mount).defaultprovider, :clear => nil, :satisfies? => true, :name => :mock, :property_hash => provider_properties
     Puppet::Type.type(:mount).defaultprovider.stubs(:new).returns(@provider)
     @mount = Puppet::Type.type(:mount).new(:name => "yay", :check => :ensure)
 
@@ -93,11 +94,12 @@ describe Puppet::Type.type(:mount)::Ensure do
       @provider.stubs(:mounted?).returns([:mounted,:ghost].include? options[:from])
       @provider.expects(:create).times(options[:create] || 0)
       @provider.expects(:destroy).times(options[:destroy] || 0)
-      @provider.expects(:mount).times(options[:mount] || 0)
+      @provider.expects(:mount).never
       @provider.expects(:unmount).times(options[:unmount] || 0)
       @ensure.stubs(:syncothers)
       @ensure.should = options[:to]
       @ensure.sync
+      (!!@provider.property_hash[:needs_mount]).should == (!!options[:mount])
    end
 
    it "should create itself when changing from :ghost to :present" do
@@ -285,34 +287,6 @@ describe Puppet::Type.type(:mount), "when modifying an existing mount entry" do
     @catalog.apply
   end
 
-  it "should flush changes before mounting" do
-    syncorder = sequence('syncorder')
-    @mount.provider.expects(:options).returns 'soft'
-    @mount.provider.expects(:ensure).returns :unmounted
-    @mount.provider.expects(:mounted?).returns false
-
-    @mount.provider.expects(:options=).in_sequence(syncorder).with 'hard'
-    @mount.expects(:flush).in_sequence(syncorder) # Have to write with no options
-    @mount.provider.expects(:mount).in_sequence(syncorder)
-    @mount.expects(:flush).in_sequence(syncorder) # Call flush again cause we changed everything
-
-    @mount[:ensure] = :mounted
-    @mount[:options] = 'hard'
-
-    @catalog.apply
-  end
-
-  it "should not flush before mounting if there are no other changes" do
-    syncorder = sequence('syncorder')
-    @mount.provider.expects(:ensure).returns :unmounted
-    @mount.provider.expects(:mounted?).returns false
-    @mount.provider.expects(:mount).in_sequence(syncorder)
-    @mount.expects(:flush).in_sequence(syncorder) # Call flush cause we changed everything
-
-    @mount[:ensure] = :mounted
-    @catalog.apply
-  end
-
   it "should umount before flushing changes to disk" do
     syncorder = sequence('syncorder')
     @mount.provider.expects(:options).returns 'soft'