diff options
author | Thom May <thom@may.lt> | 2018-03-19 16:11:58 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-03-19 16:11:58 +0000 |
commit | 577c03a04aff973ead3bfdb0592e1fbc79c7db25 (patch) | |
tree | bac9208c3f8f77f1fa1f6ceae159f9fa117b4f1d | |
parent | 0447337cbc103e4e31e20f95fadc270f1a5aa481 (diff) | |
parent | 3402b95505b6c7424d144185be9c281b60e0360b (diff) | |
download | chef-577c03a04aff973ead3bfdb0592e1fbc79c7db25.tar.gz |
Merge pull request #7007 from chef/tm/backport_6851
Backport mount provider fixes to 13
-rw-r--r-- | lib/chef/provider/mount.rb | 10 | ||||
-rw-r--r-- | lib/chef/provider/mount/aix.rb | 2 | ||||
-rw-r--r-- | lib/chef/provider/mount/mount.rb | 15 | ||||
-rw-r--r-- | lib/chef/provider/mount/solaris.rb | 27 | ||||
-rw-r--r-- | lib/chef/resource/mount.rb | 146 | ||||
-rw-r--r-- | spec/unit/provider/mount/mount_spec.rb | 33 | ||||
-rw-r--r-- | spec/unit/provider/mount/solaris_spec.rb | 4 | ||||
-rw-r--r-- | spec/unit/resource/mount_spec.rb | 45 |
8 files changed, 89 insertions, 193 deletions
diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb index 4d0d273ea9..533dd2514b 100644 --- a/lib/chef/provider/mount.rb +++ b/lib/chef/provider/mount.rb @@ -83,7 +83,7 @@ class Chef end def action_enable - unless current_resource.enabled && mount_options_unchanged? + unless current_resource.enabled && mount_options_unchanged? && device_unchanged? converge_by("enable #{current_resource.device}") do enable_fs Chef::Log.info("#{new_resource} enabled") @@ -120,6 +120,14 @@ class Chef raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #mount_options_unchanged?" end + # It's entirely plausible that a site might prefer UUIDs or labels, so + # we need to be able to update fstab to conform with their wishes + # without necessarily needing to remount the device. + # See #6851 for more. + def device_unchanged? + @current_resource.device == @new_resource.device + 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 diff --git a/lib/chef/provider/mount/aix.rb b/lib/chef/provider/mount/aix.rb index ef760b0ba6..4c2ddbe791 100644 --- a/lib/chef/provider/mount/aix.rb +++ b/lib/chef/provider/mount/aix.rb @@ -29,7 +29,7 @@ class Chef super # options and fstype are set to "defaults" and "auto" respectively in the Mount Resource class. These options are not valid for AIX, override them. if @new_resource.options[0] == "defaults" - @new_resource.options.clear + @new_resource.options([]) end if @new_resource.fstype == "auto" @new_resource.send(:clear_fstype) diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb index c3b1cc0141..1435a75ec3 100644 --- a/lib/chef/provider/mount/mount.rb +++ b/lib/chef/provider/mount/mount.rb @@ -57,12 +57,13 @@ class Chef case line when /^[#\s]/ next - when /^#{device_fstab_regex}\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/ + when /^(#{device_fstab_regex})\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/ enabled = true - @current_resource.fstype($1) - @current_resource.options($2) - @current_resource.dump($3.to_i) - @current_resource.pass($4.to_i) + @current_resource.device($1) + @current_resource.fstype($2) + @current_resource.options($3) + @current_resource.dump($4.to_i) + @current_resource.pass($5.to_i) Chef::Log.debug("Found mount #{device_fstab} to #{@new_resource.mount_point} in /etc/fstab") next when /^[\/\w]+\s+#{Regexp.escape(@new_resource.mount_point)}\s+/ @@ -147,7 +148,7 @@ class Chef end def enable_fs - if @current_resource.enabled && mount_options_unchanged? + if @current_resource.enabled && mount_options_unchanged? && device_unchanged? Chef::Log.debug("#{@new_resource} is already enabled - nothing to do") return nil end @@ -253,7 +254,7 @@ class Chef if @new_resource.device_type == :device device_mount_regex else - device_fstab + Regexp.union(device_fstab, device_mount_regex) end end diff --git a/lib/chef/provider/mount/solaris.rb b/lib/chef/provider/mount/solaris.rb index a5a7a327cb..f59a2d2c6a 100644 --- a/lib/chef/provider/mount/solaris.rb +++ b/lib/chef/provider/mount/solaris.rb @@ -121,8 +121,8 @@ class Chef end def mount_options_unchanged? - new_options = options_remove_noauto(options) - current_options = options_remove_noauto(current_resource.nil? ? nil : current_resource.options) + new_options = native_options(options) + current_options = native_options(current_resource.nil? ? nil : current_resource.options) current_resource.fsck_device == fsck_device && current_resource.fstype == fstype && @@ -168,7 +168,8 @@ class Chef def read_vfstab_status # Check to see if there is an entry in /etc/vfstab. Last entry for a volume wins. enabled = false - fstype = options = pass = nil + pass = false + fstype = options = nil ::File.foreach(VFSTAB) do |line| case line when /^[#\s]/ @@ -220,11 +221,7 @@ class Chef end def vfstab_entry - actual_options = unless options.nil? - tempops = options.dup - tempops.delete("noauto") - tempops - end + actual_options = native_options(options) autostr = mount_at_boot? ? "yes" : "no" passstr = pass == 0 ? "-" : pass optstr = (actual_options.nil? || actual_options.empty?) ? "-" : actual_options.join(",") @@ -251,11 +248,15 @@ class Chef contents << vfstab_entry end - def options_remove_noauto(temp_options) - new_options = [] - new_options += temp_options.nil? ? [] : temp_options - new_options.delete("noauto") - new_options + def native_options(temp_options) + if temp_options == %w{defaults} + ["-"] + else + new_options = [] + new_options += temp_options.nil? ? [] : temp_options.dup + new_options.delete("noauto") + new_options + end end def device_regex diff --git a/lib/chef/resource/mount.rb b/lib/chef/resource/mount.rb index 1a1f8c0565..9d8a4089cf 100644 --- a/lib/chef/resource/mount.rb +++ b/lib/chef/resource/mount.rb @@ -24,143 +24,37 @@ class Chef # Use the mount resource to manage a mounted file system. class Mount < Chef::Resource - identity_attr :device - - state_attrs :mount_point, :device_type, :fstype, :username, :password, :domain - default_action :mount allowed_actions :mount, :umount, :unmount, :remount, :enable, :disable # this is a poor API please do not re-use this pattern - property :supports, Hash, default: lazy { { remount: false } }, - coerce: proc { |x| x.is_a?(Array) ? x.each_with_object({}) { |i, m| m[i] = true } : x } + property :supports, Hash, + default: lazy { { remount: false } }, + coerce: proc { |x| x.is_a?(Array) ? x.each_with_object({}) { |i, m| m[i] = true } : x } property :password, String, sensitive: true - def initialize(name, run_context = nil) - super - @mount_point = name - @device = nil - @device_type = :device - @fsck_device = "-" - @fstype = "auto" - @options = ["defaults"] - @dump = 0 - @pass = 2 - @mounted = false - @enabled = false - @username = nil - @password = nil - @domain = nil - end - - def mount_point(arg = nil) - set_or_return( - :mount_point, - arg, - :kind_of => [ String ] - ) - end - - def device(arg = nil) - set_or_return( - :device, - arg, - :kind_of => [ String ] - ) - end - - def device_type(arg = nil) - real_arg = arg.kind_of?(String) ? arg.to_sym : arg - valid_devices = if RUBY_PLATFORM =~ /solaris/i - [ :device ] - else - [ :device, :label, :uuid ] - end - set_or_return( - :device_type, - real_arg, - :equal_to => valid_devices - ) - end - - def fsck_device(arg = nil) - set_or_return( - :fsck_device, - arg, - :kind_of => [ String ] - ) - end - - def fstype(arg = nil) - set_or_return( - :fstype, - arg, - :kind_of => [ String ] - ) - end - - def options(arg = nil) - ret = set_or_return( - :options, - arg, - :kind_of => [ Array, String ] - ) - - if ret.is_a? String - ret.tr(",", " ").split(/ /) - else - ret - end - end - - def dump(arg = nil) - set_or_return( - :dump, - arg, - :kind_of => [ Integer, FalseClass ] - ) - end + property :mount_point, String, name_property: true + property :device, String, identity: true - def pass(arg = nil) - set_or_return( - :pass, - arg, - :kind_of => [ Integer, FalseClass ] - ) - end - - def mounted(arg = nil) - set_or_return( - :mounted, - arg, - :kind_of => [ TrueClass, FalseClass ] - ) - end + property :device_type, [String, Symbol], + coerce: proc { |arg| arg.kind_of?(String) ? arg.to_sym : arg }, + default: :device, + equal_to: RUBY_PLATFORM =~ /solaris/i ? %i{ device } : %i{ device label uuid } - def enabled(arg = nil) - set_or_return( - :enabled, - arg, - :kind_of => [ TrueClass, FalseClass ] - ) - end + property :fsck_device, String, default: "-" + property :fstype, [String, nil], default: "auto" - def username(arg = nil) - set_or_return( - :username, - arg, - :kind_of => [ String ] - ) - end + property :options, [Array, String, nil], + coerce: proc { |arg| arg.kind_of?(String) ? arg.split(",") : arg }, + default: %w{defaults} - def domain(arg = nil) - set_or_return( - :domain, - arg, - :kind_of => [ String ] - ) - end + property :dump, [Integer, FalseClass], default: 0 + property :pass, [Integer, FalseClass], default: 2 + property :mounted, [TrueClass, FalseClass], default: false + property :enabled, [TrueClass, FalseClass], default: false + property :username, String + property :domain, String private diff --git a/spec/unit/provider/mount/mount_spec.rb b/spec/unit/provider/mount/mount_spec.rb index 20a4fd88dd..ca6cf59d71 100644 --- a/spec/unit/provider/mount/mount_spec.rb +++ b/spec/unit/provider/mount/mount_spec.rb @@ -53,13 +53,13 @@ describe Chef::Provider::Mount::Mount do expect(@provider.current_resource.device).to eq("/dev/sdz1") end - it "should accecpt device_type :uuid", :not_supported_on_solaris do + it "should accept device_type :uuid", :not_supported_on_solaris do @status = double(:stdout => "/dev/sdz1\n", :exitstatus => 1) @new_resource.device_type :uuid @new_resource.device "d21afe51-a0fe-4dc6-9152-ac733763ae0a" @stdout_findfs = double("STDOUT", :first => "/dev/sdz1") expect(@provider).to receive(:shell_out).with("/sbin/findfs UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a").and_return(@status) - @provider.load_current_resource() + @provider.load_current_resource @provider.mountable? end @@ -470,5 +470,34 @@ describe Chef::Provider::Mount::Mount do @provider.disable_fs end end + + # the fstab might contain the mount with the device as a device but the resource has a label. + # we should not create two mount lines, but update the existing one + context "when the device is described differently" do + it "should update the existing line" do + @current_resource.enabled(true) + status = double(:stdout => "/dev/sdz1\n", :exitstatus => 1) + expect(@provider).to receive(:shell_out).with("/sbin/findfs UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a").and_return(status) + + filesystems = [%q{/dev/sdy1 /tmp/foo ext3 defaults 1 2}, + %q{/dev/sdz1 /tmp/foo ext3 defaults 1 2}].join("\n") + fstab = StringIO.new filesystems + + fstab_write = StringIO.new + + allow(::File).to receive(:readlines).with("/etc/fstab").and_return(fstab.readlines) + allow(::File).to receive(:open).with("/etc/fstab", "w").and_yield(fstab_write) + allow(::File).to receive(:open).with("/etc/fstab", "a").and_yield(fstab_write) + + @new_resource.device_type :uuid + @new_resource.device "d21afe51-a0fe-4dc6-9152-ac733763ae0a" + @new_resource.dump 1 + + @provider.enable_fs + expect(fstab_write.string).to match(%r{/dev/sdy1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2}) + expect(fstab_write.string).to match(%r{UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2}) + expect(fstab_write.string).not_to match(%r{/dev/sdz1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2}) + end + end end end diff --git a/spec/unit/provider/mount/solaris_spec.rb b/spec/unit/provider/mount/solaris_spec.rb index 264c8b9b36..2ec9feaf3b 100644 --- a/spec/unit/provider/mount/solaris_spec.rb +++ b/spec/unit/provider/mount/solaris_spec.rb @@ -538,7 +538,7 @@ describe Chef::Provider::Mount::Solaris, :unix_only do context "after the mount's state has been discovered" do describe "mount_fs" do it "should mount the filesystem" do - expect(provider).to receive(:shell_out!).with("mount -F #{fstype} -o defaults #{device} #{mountpoint}") + expect(provider).to receive(:shell_out!).with("mount -F #{fstype} #{device} #{mountpoint}") provider.mount_fs() end @@ -600,7 +600,7 @@ describe Chef::Provider::Mount::Solaris, :unix_only do context "in the typical case" do let(:other_mount) { "/dev/dsk/c0t2d0s0 /dev/rdsk/c0t2d0s0 / ufs 2 yes -" } - let(:this_mount) { "/dev/dsk/c0t2d0s7\t/dev/rdsk/c0t2d0s7\t/mnt/foo\tufs\t2\tyes\tdefaults\n" } + let(:this_mount) { "/dev/dsk/c0t2d0s7\t/dev/rdsk/c0t2d0s7\t/mnt/foo\tufs\t2\tyes\t-\n" } let(:vfstab_file_contents) { [other_mount].join("\n") } diff --git a/spec/unit/resource/mount_spec.rb b/spec/unit/resource/mount_spec.rb index da181b7f8c..29a6bdf1f3 100644 --- a/spec/unit/resource/mount_spec.rb +++ b/spec/unit/resource/mount_spec.rb @@ -66,6 +66,10 @@ describe Chef::Resource::Mount do expect(resource.fstype).to eql("nfs") end + it "sets fstype to 'auto' by default" do + expect(resource.fstype).to eql("auto") + end + it "allows you to set the dump attribute" do resource.dump 1 expect(resource.dump).to eql(1) @@ -169,45 +173,4 @@ describe Chef::Resource::Mount do resource.domain("TEST_DOMAIN") expect(resource.domain).to eq("TEST_DOMAIN") end - - describe "when it has mount point, device type, and fstype" do - before do - resource.device("charmander") - resource.mount_point("123.456") - resource.device_type(:device) - resource.fstype("ranked") - end - - it "describes its state" do - state = resource.state_for_resource_reporter - expect(state[:mount_point]).to eq("123.456") - expect(state[:device_type]).to eql(:device) - expect(state[:fstype]).to eq("ranked") - end - - it "returns the device as its identity" do - expect(resource.identity).to eq("charmander") - end - end - - describe "when it has username, password and domain" do - before do - resource.mount_point("T:") - resource.device("charmander") - resource.username("Administrator") - resource.password("Jetstream123!") - resource.domain("TEST_DOMAIN") - end - - it "describes its state" do - state = resource.state_for_resource_reporter - expect(state[:mount_point]).to eq("T:") - expect(state[:username]).to eq("Administrator") - expect(state[:password]).to eq("*sensitive value suppressed*") - expect(state[:domain]).to eq("TEST_DOMAIN") - expect(state[:device_type]).to eql(:device) - expect(state[:fstype]).to eq("auto") - end - - end end |