(PUP-4619) sort fstab entries

lib/puppet/provider/mount/parsed.rb

@@ -195,6 +195,10 @@ def pre_gen(record)
     end
   end
 
+  # Class variable to store the catalog's choice about whether
+  # fstab entries should be sorted.
+  @sort_output = false
+
   # Every entry in fstab is :unmounted until we can prove different
   def self.prefetch_hook(target_records)
     target_records.collect do |record|
@@ -240,6 +244,15 @@ def self.prefetch(resources = nil)
         end
       end
     end
+    # Examine the catalog to determine whether the fstab should be ordered.
+    # Default is no sorting
+    @sort_output = false
+    return if !resources || resources.empty?
+    meta = resources.values[0].catalog.resource('resources', 'mount')
+    return unless meta
+    if meta[:sort_output]
+      @sort_output = true
+    end
   end
 
   def self.mountinstances
@@ -272,6 +285,53 @@ def self.mountinstances
     instances
   end
 
+  # Sort fstab entries so that
+  # a mount point is located inside another mount point appears later and
+  # a device that is located inside a mount point appears later.
+  #
+  # "Stable opportunistic insertion sort"
+  # Copy unsorted list A to sorted B by repeatedly
+  # * removing the first element from A
+  # * compare with each element in B, from first to last
+  # * insert before current element in B if smaller
+  # * insert at the end of B otherwise
+  # * finish if A is empty
+  # * repeat
+  #
+  # Stabilizes itself by inserting as late as possible.
+  #
+  # A record is "smaller" than another if
+  # * the mount point of A is a leading substring of B's mount point
+  # * the mount point of A is a leading substring of B's device (B is a bind mount)
+  #
+  # @note Does not really care about directory names and does substring
+  #   comparison only. So /media will be put before /mediastuff. This is
+  #   not necessary but will be rare and usually cause no harm, either.
+  #
+  # @api private
+  def self.order(records)
+    return records if records.empty?
+    return records unless @sort_output;
+    # insert first record first
+    result = [ ]
+    # iterate over the rest
+    records.each do |a|
+      inserted = false
+      result.each_index do |i|
+        b = result[i]
+        # is a a leading substring of b?
+        if b[:name].index(a[:name]) == 0 || b[:device].index(a[:name])
+          result.insert(i, a)
+          inserted = true
+          break
+        end
+      end
+      next if inserted
+      result.push(a)
+    end
+    result
+  end
+
   def flush
     needs_mount = @property_hash.delete(:needs_mount)
     super

lib/puppet/provider/parsedfile.rb

@@ -391,6 +391,15 @@ def self.to_file(records)
     header + text
   end
 
+  # Hook for ordering records right before writing them back to disk.
+  #
+  # @abstract The default behavior is to keep the original order, which
+  #   is generally preferable. Inheriting providers can override this method
+  #   to satisfy special ordering requirements in the managed files.
+  def self.order(records)
+    return records
+  end
+
   def create
     @resource.class.validproperties.each do |property|
       if value = @resource.should(property)

lib/puppet/type/resources.rb

@@ -87,6 +87,24 @@
     end
   end
 
+  newparam(:sort_output, :boolean => true, :parent => Puppet::Parameter::Boolean) do
+    desc "For resources that are kept in files, this flag controls whether Puppet
+      makes sure that the resources in the file or files are in a sensible order.
+      What this entails is specific to the respective provider.
+
+      This is currently implemented in the default mount provider only."
+
+    defaultto :false
+
+    validate do |value|
+      if munge(value)
+        unless @resource[:name].downcase == 'mount'
+          raise ArgumentError, "The sort_output flag is only supported by mount resources."
+        end
+      end
+    end
+  end
+
   def check(resource)
     @checkmethod ||= "#{self[:name]}_check"
     @hascheck ||= respond_to?(@checkmethod)

lib/puppet/util/fileparsing.rb

@@ -295,7 +295,9 @@ def text_line(name, options, &block)
 
   # Generate a file from a bunch of hash records.
   def to_file(records)
-    text = records.collect { |record| to_line(record) }.join(line_separator)
+    lines = self.order(records).collect { |record| to_line(record) }
+
+    text = lines.join(line_separator)
 
     text += line_separator if trailing_separator
 

spec/fixtures/integration/provider/mount/ordering

@@ -0,0 +1,3 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0

spec/fixtures/integration/provider/mount/ordering-bind

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/dev/vg0/temp	/opt/temp	ext4	defaults	0	0
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0

spec/fixtures/integration/provider/mount/ordering-inner

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/data	/opt/data	ext4	defaults	0	0
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0

spec/fixtures/integration/provider/mount/ordering-unrelated

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0
+/dev/vg0/log_archive	/opt/data/log-archive	ext4	defaults	0	0

spec/fixtures/integration/provider/mount/unordered

@@ -0,0 +1,3 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/data	/opt/data	ext4	defaults	0	2
+/dev/vg0/opt	/opt	none	bind,rw	0	0

spec/fixtures/integration/provider/mount/unordered-bind

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0
+/dev/vg0/temp	/opt/temp	ext4	defaults	0	0

spec/fixtures/integration/provider/mount/unordered-fixed

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/opt	/opt	none	bind,rw	0	0
+/dev/vg0/data	/opt/data	ext4	defaults	0	2
+/dev/vg0/log_archive	/opt/data/log-archive	ext4	defaults	0	0

spec/fixtures/integration/provider/mount/unordered-inner

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/puppetlabs	/opt/data/research/projects/puppetlabs	ext4	defaults	0	2
+/opt/temp/tables	/var/local/mysql-temp	none	bind,rw	0	0
+/dev/vg0/data	/opt/data	ext4	defaults	0	0

spec/fixtures/integration/provider/mount/unordered-unfixed

@@ -0,0 +1,4 @@
+UUID=a5d1054c-caa0-4b80-b12f-bf9449086f8e	/	ext4	defaults,relatime,errors=remount-ro	0	1
+/dev/vg0/data	/opt/data	ext4	defaults	0	2
+/dev/vg0/opt	/opt	none	bind,rw	0	0
+/dev/vg0/log_archive	/opt/data/log-archive	ext4	defaults	0	0

spec/integration/provider/mount_spec.rb

@@ -1,9 +1,11 @@
 require 'spec_helper'
+require 'puppet_spec/compiler'
 
 require 'puppet/file_bucket/dipper'
 
 describe "mount provider (integration)", :unless => Puppet.features.microsoft_windows? do
   include PuppetSpec::Files
+  include PuppetSpec::Compiler
 
   family = Facter.value(:osfamily)
 
@@ -158,4 +160,87 @@ def run_in_catalog(settings)
       expect(check_fstab(true)).to eq("/dev/disk1s1")
     end
   end
+
+  describe "when updating existing fstabs" do
+    let(:tmp_fstab) { tmpfile('fstab_fixture') }
+    let(:resources_manifest) { "resources { 'mount': sort_output => true }" }
+
+    def compare(fixture)
+      wanted = File.read(my_fixture(fixture))
+      current = File.read(tmp_fstab).gsub(/# HEADER[^\n]*\n/, '')
+      expect(current).to eq(wanted)
+    end
+
+    before :each do
+      FileUtils.cp(my_fixture('ordering'), tmp_fstab)
+    end
+
+    { 'with unrelated entries' => {
+        :example => 'should append new entries',
+        :title => '/opt/data/log-archive',
+        :device => '/dev/vg0/log_archive',
+        :result => 'ordering-unrelated',
+        :result_unsorted => 'ordering-unrelated',
+      },
+      'with an inner mount point' => {
+        :example => 'should move the inner mount point',
+        :title => '/opt/data',
+        :device => '/dev/vg0/data',
+        :result => 'ordering-inner',
+        :result_unsorted => 'unordered-inner',
+      },
+      'with a newly contained bind mount' => {
+        :example => 'should move the bind mount',
+        :title => '/opt/temp',
+        :device => '/dev/vg0/temp',
+        :result => 'ordering-bind',
+        :result_unsorted => 'unordered-bind',
+      },
+      'with a previously unsorted fstab' => {
+        :example => 'should fix existing issues',
+        :original => 'unordered',
+        :title => '/opt/data/log-archive',
+        :device => '/dev/vg0/log_archive',
+        :result => 'unordered-fixed',
+        :result_unsorted => 'unordered-unfixed',
+      },
+    }.each do |context_descr, data|
+      context context_descr do
+        [ true, false ].each do |set_order|
+
+          if set_order
+            example_description = "and output ordering #{data[:example]}"
+          else
+            example_description = 'and no ordering should just append new entries'
+          end
+
+          it example_description do
+            if data[:original]
+              FileUtils.cp(my_fixture(data[:original]), tmp_fstab)
+            end
+
+            manifest = <<-MANIFEST
+              mount {
+                  '#{data[:title]}':
+                      ensure => 'present',
+                      device => '#{data[:device]}',
+                      fstype => 'ext4',
+                      options => 'defaults',
+                      target  => '#{tmp_fstab}',
+              }
+            MANIFEST
+
+            if set_order
+              manifest += resources_manifest
+              apply_with_error_check(manifest)
+              compare(data[:result])
+            else
+              apply_with_error_check(manifest)
+              compare(data[:result_unsorted])
+            end
+          end
+        end
+      end
+    end
+  end
 end

spec/unit/provider/mount/parsed_spec.rb

@@ -212,6 +212,13 @@
     expect(instances[11].options).to eq("vers=2,account=false,log=NULL,mount=true")
   end
 
+  describe "::order" do
+    it "should behave correctly with empty input" do
+      result = described_class.order([])
+      expect(result).to be_an Array
+    end
+  end
+
   my_fixtures('*.fstab').each do |fstab|
     platform = File.basename(fstab, '.fstab')
 
@@ -286,8 +293,10 @@
 
         # Simulate transaction.rb:prefetch
         @resource_hash = {}
+        @catalog = Puppet::Resource::Catalog.new
         [@res_ghost, @res_mounted, @res_unmounted, @res_absent].each do |resource|
           @resource_hash[resource.name] = resource
+          resource.stubs(:catalog).returns @catalog
         end
       end
 

spec/unit/provider/parsedfile_spec.rb

@@ -23,6 +23,18 @@
 
   let!(:provider) { parsed_type.provide(:parsedfile_provider, :parent => described_class) }
 
+  it "should have an order method" do
+    expect(described_class).to respond_to :order
+  end
+
+  describe "the order method" do
+    [ nil, 'a string', %w{an array}, { 'a' => 'hash' } ].each do |input|
+      it "should return its input (#{input.class})" do
+        expect(described_class.order(input)).to eq input
+      end
+    end
+  end
+
   describe "when looking up records loaded from disk" do
     it "should return nil if no records have been loaded" do
       expect(provider.record?("foo")).to be_nil
@@ -196,11 +208,17 @@
   end
 
   context "writing file contents back to disk" do
+    let(:input_records) { provider.parse(input_text) }
+
     it "should not change anything except from adding a header" do
-      input_records = provider.parse(input_text)
       expect(provider.to_file(input_records)).
         to match provider.header + input_text
     end
+
+    it "should call the order method" do
+      provider.expects(:order).with(input_records).returns(input_records)
+      provider.to_file(input_records)
+    end
   end
 
   context "rewriting a file containing a native header" do

spec/unit/type/resources_spec.rb

@@ -56,6 +56,31 @@
     end
   end
 
+  context "sort_output" do
+    let (:file) { described_class.new(:name => 'file') }
+    let (:mount) { described_class.new(:name => 'mount') }
+
+    it "defaults to false" do
+      expect(file[:sort_output]).to be_falsey
+      expect(mount[:sort_output]).to be_falsey
+    end
+
+    it "can be set to false" do
+      file[:sort_output] = false
+      mount[:sort_output] = false
+    end
+
+    it "cannot be set to non-boolean values" do
+      expect { file[:sort_output] = '/some/file/path' }.to raise_error Puppet::Error
+      expect { mount[:sort_output] = 'Some["reference"]' }.to raise_error Puppet::Error
+    end
+
+    it "cannot be set to true for resource types besides mount" do
+      expect { file[:sort_output] = true }.to raise_error Puppet::Error
+      expect { mount[:sort_output] = true }.to_not raise_error
+    end
+  end
+
   context "#check_user purge behaviour" do
     context "with unless_system_user => true" do
       before do