Merge pull request #567 from jaymzh/fs2_mountview_fix

Fix my_mountpoint view in filesystem2; add tests

5 files changed, 149 insertions, 3 deletions

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 70008289..5d1c2996 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -47,7 +47,10 @@ Unlike the 'filesystem' plugin, it provides 3 views into the data:
   be a symlink to /proc/mounts it can still have data loss due to different
   mount options, or multiple virtualfs mounts with the same fake device name.
 * `by_mount` similar to the above but indexed by mountpoint. Won't include
-  unmounted filesystems, of course.
+  unmounted filesystems, of course. Instead of a 'device' entry it has a
+  'devices' entry that is an array. Similar to the 'by_device' view, this extra
+  feature can solve many problems of of the old filesystem plugin, but may still
+  have data loss on things like mount options.
 
 It is recommended to always use `by_pair` when iterating or wanting a full view
 of storage devices. The other two are provided for convenient lookup. Other
diff --git a/lib/ohai/plugins/darwin/filesystem2.rb b/lib/ohai/plugins/darwin/filesystem2.rb
index fe36ae3e..b90d396e 100644
--- a/lib/ohai/plugins/darwin/filesystem2.rb
+++ b/lib/ohai/plugins/darwin/filesystem2.rb
@@ -43,9 +43,13 @@ Ohai.plugin(:Filesystem2) do
       next unless entry[:mount]
       view[entry[:mount]] = Mash.new unless view[entry[:mount]]
       entry.each do |key, val|
-        next if key == 'mount'
+        next if ['mount', 'device'].include?(key)
         view[entry[:mount]][key] = val
       end
+      if entry[:device]
+        view[entry[:mount]][:devices] = [] unless view[entry[:mount]][:devices]
+        view[entry[:mount]][:devices] << entry[:device]
+      end
     end
     view
   end
diff --git a/lib/ohai/plugins/linux/filesystem2.rb b/lib/ohai/plugins/linux/filesystem2.rb
index bad4277a..0e850aa1 100644
--- a/lib/ohai/plugins/linux/filesystem2.rb
+++ b/lib/ohai/plugins/linux/filesystem2.rb
@@ -78,9 +78,13 @@ Ohai.plugin(:Filesystem2) do
       next unless entry[:mount]
       view[entry[:mount]] = Mash.new unless view[entry[:mount]]
       entry.each do |key, val|
-        next if key == 'mount'
+        next if ['mount', 'device'].include?(key)
         view[entry[:mount]][key] = val
       end
+      if entry[:device]
+        view[entry[:mount]][:devices] = [] unless view[entry[:mount]][:devices]
+        view[entry[:mount]][:devices] << entry[:device]
+      end
     end
     view
   end
diff --git a/spec/unit/plugins/darwin/filesystem2_spec.rb b/spec/unit/plugins/darwin/filesystem2_spec.rb
index 85e44de1..e2e7fe6f 100644
--- a/spec/unit/plugins/darwin/filesystem2_spec.rb
+++ b/spec/unit/plugins/darwin/filesystem2_spec.rb
@@ -93,4 +93,47 @@ MOUNT
       expect(@plugin[:filesystem2]["by_pair"]["/dev/disk0s2,/"][:mount_options]).to eq([ "local", "journaled" ])
     end
   end
+
+  describe "when gathering filesystem data with devices mounted more than once" do
+    before(:each) do
+      @dfstdout = <<-DF
+Filesystem           512-blocks      Used Available Capacity  iused    ifree %iused  Mounted on
+/dev/disk0s2          488555536 313696448 174347088    65% 39276054 21793386   64%   /
+devfs                       385       385         0   100%      666        0  100%   /dev
+map /etc/auto.direct          0         0         0   100%        0        0  100%   /mnt/vol
+map -hosts                    0         0         0   100%        0        0  100%   /net
+map -static                   0         0         0   100%        0        0  100%   /mobile_symbol
+deweyfs@osxfuse0              0         0         0   100%        0        0  100%   /mnt/dewey
+/dev/disk0s2          488555536 313696448 174347088    65% 39276054 21793386 64%   /another/mountpoint
+DF
+      allow(@plugin).to receive(:shell_out).with("df -i").and_return(mock_shell_out(0, @dfstdout, ""))
+    end
+
+    it "should provide a devices view with all mountpoints" do
+      @plugin.run
+      expect(@plugin[:filesystem2]["by_device"]["/dev/disk0s2"][:mounts]).to eq(['/', '/another/mountpoint'])
+    end
+  end
+
+  describe "when gathering filesystem data with double-mounts" do
+    before(:each) do
+      @dfstdout = <<-DF
+Filesystem           512-blocks      Used Available Capacity  iused    ifree %iused  Mounted on
+/dev/disk0s2          488555536 313696448 174347088    65% 39276054 21793386   64%   /
+devfs                       385       385         0   100%      666        0  100%   /dev
+map /etc/auto.direct          0         0         0   100%        0        0  100%   /mnt/vol
+map -hosts                    0         0         0   100%        0        0  100%   /net
+map -static                   0         0         0   100%        0        0  100%   /mobile_symbol
+deweyfs@osxfuse0              0         0         0   100%        0        0  100%   /mnt/dewey
+/dev/disk0s3          488555536 313696448 174347088    65% 39276054 21793386 64% /mnt
+/dev/disk0s4          488555536 313696448 174347088    65% 39276054 21793386 64% /mnt
+DF
+      allow(@plugin).to receive(:shell_out).with("df -i").and_return(mock_shell_out(0, @dfstdout, ""))
+    end
+
+    it "should provide a mounts view with all devices" do
+      @plugin.run
+      expect(@plugin[:filesystem2]["by_mountpoint"]["/mnt"][:devices]).to eq(['/dev/disk0s3', '/dev/disk0s4'])
+    end
+  end
 end
diff --git a/spec/unit/plugins/linux/filesystem2_spec.rb b/spec/unit/plugins/linux/filesystem2_spec.rb
index de14225c..b4a18a3d 100644
--- a/spec/unit/plugins/linux/filesystem2_spec.rb
+++ b/spec/unit/plugins/linux/filesystem2_spec.rb
@@ -334,4 +334,96 @@ MOUNTS
     end
   end
 
+  describe "when gathering filesystem data with devices mounted more than once" do
+    before(:each) do
+      # there's a few different examples one can run into in this output:
+      # 1. A device physically mounted in more than one place: /home and /home2
+      # 2. A bind-mounted directory, which shows up as the same device in a
+      # subdir: / and /var/chroot
+      # 3. tmpfs in multiple places.
+      @dfstdout = <<-DF
+Filesystem         1024-blocks      Used Available Capacity Mounted on
+/dev/mapper/sys.vg-root.lv   4805760    378716   4182924       9% /
+tmpfs                  2030944         0   2030944       0% /lib/init/rw
+udev                   2025576       228   2025348       1% /dev
+tmpfs                  2030944      2960   2027984       1% /dev/shm
+/dev/mapper/sys.vg-home.lv  97605056  53563252  44041804      55% /home
+/dev/mapper/sys.vg-home.lv  97605056  53563252  44041804      55% /home2
+/dev/mapper/sys.vg-root.lv  4805760    378716   4182924       9% /var/chroot
+DF
+      allow(@plugin).to receive(:shell_out).with("df -P").and_return(mock_shell_out(0, @dfstdout, ""))
+      
+      @inode_stdout = <<-DFi
+Filesystem      Inodes  IUsed   IFree IUse% Mounted on
+/dev/mapper/sys.vg-root.lv     1310720 107407 1203313    9% /
+tmpfs           126922    273  126649    1% /lib/init/rw
+none            126922      1  126921    1% /dev/shm
+udev            126922      1  126921    1% /dev
+/dev/mapper/sys.vg-home.lv  60891136  4696030 56195106      8% /home
+/dev/mapper/sys.vg-home.lv  60891136  4696030 56195106      8% /home2
+/dev/mapper/sys.vg-root.lv  1310720 107407 1203313       9% /var/chroot
+DFi
+      allow(@plugin).to receive(:shell_out).with("df -iP").and_return(mock_shell_out(0, @inode_stdout, ""))
+
+      allow(File).to receive(:exist?).with("/bin/lsblk").and_return(true)
+      @stdout = <<-BLKID_TYPE
+NAME=\"/dev/mapper/sys.vg-root.lv\" UUID=\"7742d14b-80a3-4e97-9a32-478be9ea9aea\" LABEL=\"/\" FSTYPE=\"ext4\"
+NAME=\"/dev/mapper/sys.vg-home.lv\" UUID=\"d6efda02-1b73-453c-8c74-7d8dee78fa5e\" LABEL=\"/home\" FSTYPE=\"xfs\"
+BLKID_TYPE
+      allow(@plugin).to receive(:shell_out).
+        with("lsblk -n -P -o NAME,UUID,LABEL,FSTYPE").
+        and_return(mock_shell_out(0, @stdout, ""))
+    end
+
+    it "should provide a devices view with all mountpoints" do
+      @plugin.run
+      expect(@plugin[:filesystem2]["by_device"]["/dev/mapper/sys.vg-root.lv"][:mounts]).to eq(['/', '/var/chroot'])
+      expect(@plugin[:filesystem2]["by_device"]["/dev/mapper/sys.vg-home.lv"][:mounts]).to eq(['/home', '/home2'])
+      expect(@plugin[:filesystem2]["by_device"]["tmpfs"][:mounts]).to eq(['/lib/init/rw', '/dev/shm'])
+    end
+  end
+
+  describe "when gathering filesystem data with double-mounts" do
+    before(:each) do
+      @dfstdout = <<-DF
+Filesystem         1024-blocks      Used Available Capacity Mounted on
+/dev/mapper/sys.vg-root.lv   4805760    378716   4182924       9% /
+tmpfs                  2030944         0   2030944       0% /lib/init/rw
+udev                   2025576       228   2025348       1% /dev
+tmpfs                  2030944      2960   2027984       1% /dev/shm
+/dev/mapper/sys.vg-home.lv  97605056  53563252  44041804      55% /home
+/dev/sdb1              97605056  53563252  44041804      55% /mnt
+/dev/sdc1              4805760    378716   4182924       9% /mnt
+DF
+      allow(@plugin).to receive(:shell_out).with("df -P").and_return(mock_shell_out(0, @dfstdout, ""))
+      
+      @inode_stdout = <<-DFi
+Filesystem      Inodes  IUsed   IFree IUse% Mounted on
+/dev/mapper/sys.vg-root.lv     1310720 107407 1203313    9% /
+tmpfs           126922    273  126649    1% /lib/init/rw
+none            126922      1  126921    1% /dev/shm
+udev            126922      1  126921    1% /dev
+/dev/mapper/sys.vg-home.lv  60891136  4696030 56195106      8% /home
+/dev/sdb1       60891136  4696030 56195106      8% /mnt
+/dev/sdc1       1310720 107407 1203313          9% /mnt
+DFi
+      allow(@plugin).to receive(:shell_out).with("df -iP").and_return(mock_shell_out(0, @inode_stdout, ""))
+
+      allow(File).to receive(:exist?).with("/bin/lsblk").and_return(true)
+      @stdout = <<-BLKID_TYPE
+NAME=\"/dev/mapper/sys.vg-root.lv\" UUID=\"7742d14b-80a3-4e97-9a32-478be9ea9aea\" LABEL=\"/\" FSTYPE=\"ext4\"
+NAME=\"/dev/sdb1\" UUID=\"6b559c35-7847-4ae2-b512-c99012d3f5b3\" LABEL=\"/mnt\" FSTYPE=\"ext4\"
+NAME=\"/dev/sdc1\" UUID=\"7f1e51bf-3608-4351-b7cd-379e39cff36a\" LABEL=\"/mnt\" FSTYPE=\"ext4\"
+NAME=\"/dev/mapper/sys.vg-home.lv\" UUID=\"d6efda02-1b73-453c-8c74-7d8dee78fa5e\" LABEL=\"/home\" FSTYPE=\"xfs\"
+BLKID_TYPE
+      allow(@plugin).to receive(:shell_out).
+        with("lsblk -n -P -o NAME,UUID,LABEL,FSTYPE").
+        and_return(mock_shell_out(0, @stdout, ""))
+    end
+
+    it "should provide a mounts view with all devices" do
+      @plugin.run
+      expect(@plugin[:filesystem2]["by_mountpoint"]["/mnt"][:devices]).to eq(['/dev/sdb1', '/dev/sdc1'])
+    end
+  end
 end