Merge pull request #1454 from opscode/lcg/mount_stuff

Fix some mount provider rage

3 files changed, 187 insertions, 122 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5bfb8b8023..ed50c7d1b1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,7 +24,7 @@
 * [**Nikhil Benesch**](https://github.com/benesch):
   Implemented a threaded download queue for synchronizing cookbooks. (CHEF-4423)
 
-
+* Cleaned up mount provider superclass
 * Added "knife serve" to bring up local mode as a server
 * Print nested LWRPs with indentation in doc formatter output
 * Make local mode stable enough to run chef-pedant
diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index 5f58baa396..b46604260e 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -1,6 +1,7 @@
 #
-# Author:: Joshua Timberman (<joshua@opscode.com>)
-# Copyright:: Copyright (c) 2009 Opscode, Inc
+# Author:: Joshua Timberman (<joshua@getchef.com>)
+# Author:: Lamont Granquist (<lamont@getchef.com>)
+# Copyright:: Copyright (c) 2009-2014 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -26,6 +27,7 @@ class Chef
 
       include Chef::Mixin::Command
 
+      attr_accessor :unmount_retries
 
       def whyrun_supported?
         true
@@ -35,94 +37,134 @@ class Chef
         true
       end
 
+      def initialize(new_resource, run_context)
+        super
+        self.unmount_retries = 20
+      end
+
       def action_mount
-        unless @current_resource.mounted
-          converge_by("mount #{@current_resource.device} to #{@current_resource.mount_point}") do
-            status = mount_fs()
-            if status
-              Chef::Log.info("#{@new_resource} mounted")
-            end
+        unless current_resource.mounted
+          converge_by("mount #{current_resource.device} to #{current_resource.mount_point}") do
+            mount_fs
+            Chef::Log.info("#{new_resource} mounted")
           end
         else
-          Chef::Log.debug("#{@new_resource} is already mounted")
+          Chef::Log.debug("#{new_resource} is already mounted")
         end
       end
 
       def action_umount
-        if @current_resource.mounted
-          converge_by("unmount #{@current_resource.device}") do
-            status = umount_fs()
-            if status
-              Chef::Log.info("#{@new_resource} unmounted")
-            end
+        if current_resource.mounted
+          converge_by("unmount #{current_resource.device}") do
+            umount_fs
+            Chef::Log.info("#{new_resource} unmounted")
           end
         else
-          Chef::Log.debug("#{@new_resource} is already unmounted")
+          Chef::Log.debug("#{new_resource} is already unmounted")
         end
       end
 
       def action_remount
-        unless @new_resource.supports[:remount]
-          raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :remount"
-        else
-          if @current_resource.mounted
-            converge_by("remount #{@current_resource.device}") do
-              status = remount_fs()
-              if status
-                Chef::Log.info("#{@new_resource} remounted")
-              end
+        if current_resource.mounted
+          if new_resource.supports[:remount]
+            converge_by("remount #{current_resource.device}") do
+              remount_fs
+              Chef::Log.info("#{new_resource} remounted")
             end
           else
-            Chef::Log.debug("#{@new_resource} not mounted, nothing to remount")
+            converge_by("unmount #{current_resource.device}") do
+              umount_fs
+              Chef::Log.info("#{new_resource} unmounted")
+            end
+            wait_until_unmounted(unmount_retries)
+            converge_by("mount #{current_resource.device}") do
+              mount_fs
+              Chef::Log.info("#{new_resource} mounted")
+            end
           end
+        else
+          Chef::Log.debug("#{new_resource} not mounted, nothing to remount")
         end
       end
 
       def action_enable
-        unless @current_resource.enabled && mount_options_unchanged?
-          converge_by("remount #{@current_resource.device}") do
-            status = enable_fs
-            if status
-              Chef::Log.info("#{@new_resource} enabled")
-            else
-              Chef::Log.debug("#{@new_resource} already enabled")
-            end
+        unless current_resource.enabled && mount_options_unchanged?
+          converge_by("enable #{current_resource.device}") do
+            enable_fs
+            Chef::Log.info("#{new_resource} enabled")
           end
+        else
+          Chef::Log.debug("#{new_resource} already enabled")
         end
       end
 
       def action_disable
-        if @current_resource.enabled
-          converge_by("remount #{@current_resource.device}") do
-            status = disable_fs
-            if status
-              Chef::Log.info("#{@new_resource} disabled")
-            else
-              Chef::Log.debug("#{@new_resource} already disabled")
-            end
+        if current_resource.enabled
+          converge_by("disable #{current_resource.device}") do
+            disable_fs
+            Chef::Log.info("#{new_resource} disabled")
           end
+        else
+          Chef::Log.debug("#{new_resource} already disabled")
         end
       end
 
+      #
+      # Abstract Methods to be implemented by subclasses
+      #
+
+      # should actually check if the filesystem is mounted (not just return current_resource) and return true/false
+      def mounted?
+        raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not implement #mounted?"
+      end
+
+      # should check new_resource against current_resource to see if mount options need updating, returns true/false
+      def mount_options_unchanged?
+        raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not implement #mount_options_unchanged?"
+      end
+
+      #
+      # NOTE: for the following methods, this superclass will already have checked if the filesystem is
+      # enabled and/or mounted and they will be called in converge_by blocks, so most defensive checking
+      # does not need to be done in the subclass implementation -- just do the thing.
+      #
+
+      # should implement mounting of the filesystem, raises if action does not succeed
       def mount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :mount"
       end
 
+      # should implement unmounting of the filesystem, raises if action does not succeed
       def umount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :umount"
       end
 
+      # should implement remounting of the filesystem (via a -o remount or some other atomic-ish action that isn't
+      # simply a umount/mount style remount), raises if action does not succeed
       def remount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :remount"
       end
 
+      # should implement enabling of the filesystem (e.g. in /etc/fstab), raises if action does not succeed
       def enable_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :enable"
       end
 
+      # should implement disabling of the filesystem (e.g. in /etc/fstab), raises if action does not succeed
       def disable_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :disable"
       end
+
+      private
+
+      def wait_until_unmounted(tries)
+        while mounted?
+          if (tries -= 1) < 0
+            raise Chef::Exceptions::Mount, "Retries exceeded waiting for filesystem to unmount"
+          end
+          sleep 0.1
+        end
+      end
     end
   end
 end
diff --git a/spec/unit/provider/mount_spec.rb b/spec/unit/provider/mount_spec.rb
index d9bfeb3e0f..e9fe3fa050 100644
--- a/spec/unit/provider/mount_spec.rb
+++ b/spec/unit/provider/mount_spec.rb
@@ -1,6 +1,7 @@
 #
-# Author:: Joshua Timberman (<joshua@opscode.com>)
-# Copyright:: Copyright (c) 2008 OpsCode, Inc.
+# Author:: Joshua Timberman (<joshua@getchef.com>)
+# Author:: Lamont Granquist (<lamont@getchef.com>)
+# Copyright:: Copyright (c) 2008-2014 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -19,152 +20,174 @@
 require 'spec_helper'
 
 describe Chef::Provider::Mount do
-  before(:each) do
-    @node = Chef::Node.new
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, {}, @events)
-
-    @new_resource = Chef::Resource::Mount.new('/tmp/foo')
-    @new_resource.device      "/dev/sdz1"
-    @new_resource.name        "/tmp/foo"
-    @new_resource.mount_point "/tmp/foo"
-    @new_resource.fstype      "ext3"
-
-    @current_resource = Chef::Resource::Mount.new('/tmp/foo')
-    @current_resource.device      "/dev/sdz1"
-    @current_resource.name        "/tmp/foo"
-    @current_resource.mount_point "/tmp/foo"
-    @current_resource.fstype      "ext3"
-
-    @provider = Chef::Provider::Mount.new(@new_resource, @run_context)
-    @provider.current_resource = @current_resource
+
+  let(:node) { Chef::Node.new }
+
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+
+  let(:new_resource) do
+    new_resource = Chef::Resource::Mount.new('/tmp/foo')
+    new_resource.device      "/dev/sdz1"
+    new_resource.name        "/tmp/foo"
+    new_resource.mount_point "/tmp/foo"
+    new_resource.fstype      "ext3"
+    new_resource
+  end
+
+  let(:current_resource) do
+    # this abstract superclass has no load_current_resource to call
+    current_resource = Chef::Resource::Mount.new('/tmp/foo')
+    current_resource.device      "/dev/sdz1"
+    current_resource.name        "/tmp/foo"
+    current_resource.mount_point "/tmp/foo"
+    current_resource.fstype      "ext3"
+    current_resource
+  end
+
+  let(:provider) do
+    provider = Chef::Provider::Mount.new(new_resource, run_context)
+    provider.current_resource = current_resource
+    provider
   end
 
   describe "when the target state is a mounted filesystem" do
 
     it "should mount the filesystem if it isn't mounted" do
-      @current_resource.stub(:mounted).and_return(false)
-      @provider.should_receive(:mount_fs).with.and_return(true)
-      @provider.run_action(:mount)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(false)
+      expect(provider).to receive(:mount_fs).and_return(true)
+      provider.run_action(:mount)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not mount the filesystem if it is mounted" do
-      @current_resource.stub(:mounted).and_return(true)
-      @provider.should_not_receive(:mount_fs)
-      @provider.run_action(:mount)
-      @new_resource.should_not be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(true)
+      expect(provider).not_to receive(:mount_fs)
+      provider.run_action(:mount)
+      expect(new_resource).not_to be_updated_by_last_action
     end
 
   end
 
   describe "when the target state is an unmounted filesystem" do
     it "should umount the filesystem if it is mounted" do
-      @current_resource.stub(:mounted).and_return(true)
-      @provider.should_receive(:umount_fs).with.and_return(true)
-      @provider.run_action(:umount)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(true)
+      expect(provider).to receive(:umount_fs).and_return(true)
+      provider.run_action(:umount)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not umount the filesystem if it is not mounted" do
-      @current_resource.stub(:mounted).and_return(false)
-      @provider.should_not_receive(:umount_fs)
-      @provider.run_action(:umount)
-      @new_resource.should_not be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(false)
+      expect(provider).not_to receive(:umount_fs)
+      provider.run_action(:umount)
+      expect(new_resource).not_to be_updated_by_last_action
     end
   end
 
   describe "when the filesystem should be remounted and the resource supports remounting" do
     before do
-      @new_resource.supports[:remount] = true
+      new_resource.supports[:remount] = true
     end
 
     it "should remount the filesystem if it is mounted" do
-      @current_resource.stub(:mounted).and_return(true)
-      @provider.should_receive(:remount_fs).and_return(true)
-      @provider.run_action(:remount)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(true)
+      expect(provider).to receive(:remount_fs).and_return(true)
+      provider.run_action(:remount)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not remount the filesystem if it is not mounted" do
-      @current_resource.stub(:mounted).and_return(false)
-      @provider.should_not_receive(:remount_fs)
-      @provider.run_action(:remount)
-      @new_resource.should_not be_updated_by_last_action
+      allow(current_resource).to receive(:mounted).and_return(false)
+      expect(provider).not_to receive(:remount_fs)
+      provider.run_action(:remount)
+      expect(new_resource).not_to be_updated_by_last_action
     end
   end
+
   describe "when the filesystem should be remounted and the resource does not support remounting" do
     before do
-      @new_resource.supports[:remount] = false
+      new_resource.supports[:remount] = false
+      allow(current_resource).to receive(:mounted).and_return(true)
     end
 
-    it "should fail to remount the filesystem" do
-      @provider.should_not_receive(:remount_fs)
-      lambda {@provider.run_action(:remount)}.should raise_error(Chef::Exceptions::UnsupportedAction)
-      @new_resource.should_not be_updated_by_last_action
+    it "should try a umount/remount of the filesystem" do
+      expect(provider).to receive(:umount_fs)
+      expect(provider).to receive(:mounted?).and_return(true, false)
+      expect(provider).to receive(:mount_fs)
+      provider.run_action(:remount)
+      expect(new_resource).to be_updated_by_last_action
     end
 
+    it "should fail when it runs out of remounts" do
+      provider.unmount_retries = 1
+      expect(provider).to receive(:umount_fs)
+      expect(provider).to receive(:mounted?).and_return(true, true)
+      expect{ provider.run_action(:remount) }.to raise_error(Chef::Exceptions::Mount)
+    end
   end
+
   describe "when enabling the filesystem to be mounted" do
     it "should enable the mount if it isn't enable" do
-      @current_resource.stub(:enabled).and_return(false)
-      @provider.should_not_receive(:mount_options_unchanged?)
-      @provider.should_receive(:enable_fs).and_return(true)
-      @provider.run_action(:enable)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:enabled).and_return(false)
+      expect(provider).not_to receive(:mount_options_unchanged?)
+      expect(provider).to receive(:enable_fs).and_return(true)
+      provider.run_action(:enable)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should enable the mount if it is enabled and mount options have changed" do
-      @current_resource.stub(:enabled).and_return(true)
-      @provider.should_receive(:mount_options_unchanged?).and_return(false)
-      @provider.should_receive(:enable_fs).and_return(true)
-      @provider.run_action(:enable)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:enabled).and_return(true)
+      expect(provider).to receive(:mount_options_unchanged?).and_return(false)
+      expect(provider).to receive(:enable_fs).and_return(true)
+      provider.run_action(:enable)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not enable the mount if it is enabled and mount options have not changed" do
-      @current_resource.stub(:enabled).and_return(true)
-      @provider.should_receive(:mount_options_unchanged?).and_return(true)
-      @provider.should_not_receive(:enable_fs)
-      @provider.run_action(:enable)
-      @new_resource.should_not be_updated_by_last_action
+      allow(current_resource).to receive(:enabled).and_return(true)
+      expect(provider).to receive(:mount_options_unchanged?).and_return(true)
+      expect(provider).not_to receive(:enable_fs)
+      provider.run_action(:enable)
+      expect(new_resource).not_to be_updated_by_last_action
     end
   end
 
   describe "when the target state is to disable the mount" do
     it "should disable the mount if it is enabled" do
-      @current_resource.stub(:enabled).and_return(true)
-      @provider.should_receive(:disable_fs).with.and_return(true)
-      @provider.run_action(:disable)
-      @new_resource.should be_updated_by_last_action
+      allow(current_resource).to receive(:enabled).and_return(true)
+      expect(provider).to receive(:disable_fs).and_return(true)
+      provider.run_action(:disable)
+      expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not disable the mount if it isn't enabled" do
-      @current_resource.stub(:enabled).and_return(false)
-      @provider.should_not_receive(:disable_fs)
-      @provider.run_action(:disable)
-      @new_resource.should_not be_updated_by_last_action
+      allow(current_resource).to receive(:enabled).and_return(false)
+      expect(provider).not_to receive(:disable_fs)
+      provider.run_action(:disable)
+      expect(new_resource).not_to be_updated_by_last_action
     end
   end
 
 
   it "should delegates the mount implementation to subclasses" do
-    lambda { @provider.mount_fs }.should raise_error(Chef::Exceptions::UnsupportedAction)
+    expect { provider.mount_fs }.to raise_error(Chef::Exceptions::UnsupportedAction)
   end
 
   it "should delegates the umount implementation to subclasses" do
-    lambda { @provider.umount_fs }.should raise_error(Chef::Exceptions::UnsupportedAction)
+    expect { provider.umount_fs }.to raise_error(Chef::Exceptions::UnsupportedAction)
   end
 
   it "should delegates the remount implementation to subclasses" do
-    lambda { @provider.remount_fs }.should raise_error(Chef::Exceptions::UnsupportedAction)
+    expect { provider.remount_fs }.to raise_error(Chef::Exceptions::UnsupportedAction)
   end
 
   it "should delegates the enable implementation to subclasses" do
-    lambda { @provider.enable_fs }.should raise_error(Chef::Exceptions::UnsupportedAction)
+    expect { provider.enable_fs }.to raise_error(Chef::Exceptions::UnsupportedAction)
   end
 
   it "should delegates the disable implementation to subclasses" do
-    lambda { @provider.disable_fs }.should raise_error(Chef::Exceptions::UnsupportedAction)
+    expect { provider.disable_fs }.to raise_error(Chef::Exceptions::UnsupportedAction)
   end
 end