Merge lp:~tribaal/landscape-client/fix-1045374-mount-storm-in-client-take-2 into lp:~landscape/landscape-client/trunk

[Chris Glass]

After further testing, the filtering on device type is less interesting than filtering on filesystem type (autofs filesystems are shown as being of "autofs" type).

Filtering on device does not work since the autofs device name is actually the name of the map (as appearing in /etc/auto.master), and is therefore "auto_direct" only as a convention.

Since there was already a filesystem type whitelist in the code I refactored it to be in the right module, and made the whitelist the default choice.

[Thomas Herve (therve)]

[1]
+ if filesystems_whitelist is None:
+ filesystems_whitelist = STABLE_FILESYSTEMS

You can directly make it the default of the function. Maybe use a frozen set to feel more comfortable.

[2]
+from landscape.lib.disk import (get_mount_info, get_filesystem_for_path,
+STABLE_FILESYSTEMS)

You don't need to pass the value anymore, so you don't have to import STABLE_FILESYSTEMS.

[3] pep8

landscape/sysinfo/disk.py:12:25: E225 missing whitespace around operator
landscape/sysinfo/disk.py:13:37: E225 missing whitespace around operator
landscape/sysinfo/disk.py:15:37: E225 missing whitespace around operator

Thanks!

review: Approve

[lp:~tribaal/landscape-client/fix-1045374-mount-storm-in-client-take-2]

577. By Chris Glass on 2012-09-14

Made the STABLE_FILESYSTEM list a frozenset, passed it as method default and
removed it from the various places where it was imported and used for no reason.

Also fixed some lint.

[Chris Glass (tribaal)]

All comments should be addressed.

[Geoff Teale (tealeg)]

+1 Looks Good

review: Approve

[lp:~tribaal/landscape-client/fix-1045374-mount-storm-in-client-take-2]

578. By Chris Glass on 2012-09-17

Removed default filesystem whitelist in get_filesystem_for_path

=== modified file 'landscape/lib/disk.py'
--- landscape/lib/disk.py	2010-06-07 18:04:31 +0000
+++ landscape/lib/disk.py	2012-09-17 13:15:25 +0000
@@ -4,7 +4,14 @@
 import statvfs
 
 
-def get_mount_info(mounts_file, statvfs_, filesystems_whitelist=None):
+# List of filesystem types authorized when generating disk use statistics.
+STABLE_FILESYSTEMS = frozenset(
+    ["ext", "ext2", "ext3", "ext4", "reiserfs", "ntfs", "msdos", "dos", "vfat",
+     "xfs", "hpfs", "jfs", "ufs", "hfs", "hfsplus"])
+
+
+def get_mount_info(mounts_file, statvfs_,
+                   filesystems_whitelist=STABLE_FILESYSTEMS):
     """
     This is a generator that yields information about mounted filesystems.
 
@@ -40,8 +47,7 @@
                "free-space": free_space}
 
 
-def get_filesystem_for_path(path, mounts_file, statvfs_,
-                            filesystems_whitelist=None):
+def get_filesystem_for_path(path, mounts_file, statvfs_):
     """
     Tries to determine to which of the mounted filesystem C{path} belongs to,
     and then returns information about that filesystem or C{None} if it
@@ -61,7 +67,7 @@
     candidate = None
     path = os.path.realpath(path)
     path_segments = path.split("/")
-    for info in get_mount_info(mounts_file, statvfs_, filesystems_whitelist):
+    for info in get_mount_info(mounts_file, statvfs_):
         mount_segments = info["mount-point"].split("/")
         if path.startswith(info["mount-point"]):
             if ((not candidate)

=== modified file 'landscape/lib/tests/test_disk.py'
--- landscape/lib/tests/test_disk.py	2011-07-05 05:09:11 +0000
+++ landscape/lib/tests/test_disk.py	2012-09-17 13:15:25 +0000
@@ -1,6 +1,6 @@
 import os
 
-from landscape.lib.disk import get_filesystem_for_path
+from landscape.lib.disk import get_filesystem_for_path, get_mount_info
 from landscape.tests.helpers import LandscapeTest
 
 
@@ -32,7 +32,7 @@
         yield a permission denied error when inspected.
         """
         self.read_access = read_access
-        content = "\n".join("/dev/sda%d %s fsfs rw 0 0" % (i, point)
+        content = "\n".join("/dev/sda%d %s ext4 rw 0 0" % (i, point)
                             for i, point in enumerate(points))
         f = open(self.mount_file, "w")
         f.write(content)
@@ -70,15 +70,6 @@
                                        self.mount_file, self.statvfs)
         self.assertEqual(info["mount-point"], "/foo")
 
-    def test_whitelist(self):
-        self.set_mount_points(["/"])
-        info = get_filesystem_for_path(
-            "/", self.mount_file, self.statvfs, ["ext3"])
-        self.assertIdentical(info, None)
-        info = get_filesystem_for_path(
-            "/", self.mount_file, self.statvfs, ["ext3", "fsfs"])
-        self.assertNotIdentical(info, None)
-
     def test_ignore_unreadable_mount_point(self):
         """
         We should ignore mountpoints which are unreadable by the user who
@@ -88,3 +79,25 @@
         info = get_filesystem_for_path(
             "/secret", self.mount_file, self.statvfs)
         self.assertIdentical(info, None)
+
+    def test_ignore_unmounted_and_virtual_mountpoints(self):
+        """
+        Make sure autofs and virtual mountpoints are ignored. This is to
+        ensure non-regression on bug #1045374.
+        """
+        self.read_access = True
+        content = "\n".join(["auto_direct /opt/whatever autofs",
+                             "none /run/lock tmpfs",
+                             "proc /proc proc",
+                             "/dev/sda1 /home ext4"])
+
+        f = open(self.mount_file, "w")
+        f.write(content)
+        f.close()
+
+        self.stat_results["/home"] = (4096, 0, 1000, 500, 0, 0, 0, 0, 0)
+
+        result = [x for x in get_mount_info(self.mount_file, self.statvfs)]
+        expected = {"device": "/dev/sda1", "mount-point": "/home",
+                    "filesystem": "ext4", "total-space": 3, "free-space": 1}
+        self.assertEqual([expected], result)

=== modified file 'landscape/sysinfo/disk.py'
--- landscape/sysinfo/disk.py	2010-03-24 15:28:22 +0000
+++ landscape/sysinfo/disk.py	2012-09-17 13:15:25 +0000
@@ -4,20 +4,14 @@
 
 from twisted.internet.defer import succeed
 
-from landscape.lib.disk import get_mount_info, get_filesystem_for_path
-
-
-# List of filesystem types authorized when generating disk use statistics.
-STABLE_FILESYSTEMS = set(
-    ["ext", "ext2", "ext3", "ext4", "reiserfs", "ntfs", "msdos", "dos", "vfat",
-     "xfs", "hpfs", "jfs", "ufs", "hfs", "hfsplus"])
+from landscape.lib.disk import (get_mount_info, get_filesystem_for_path)
 
 
 def format_megabytes(megabytes):
-    if megabytes >= 1024*1024:
-        return "%.2fTB" % (megabytes/(1024*1024))
+    if megabytes >= 1024 * 1024:
+        return "%.2fTB" % (megabytes / (1024 * 1024))
     elif megabytes >= 1024:
-        return "%.2fGB" % (megabytes/1024)
+        return "%.2fGB" % (megabytes / 1024)
     else:
         return "%dMB" % (megabytes)
 
@@ -39,12 +33,12 @@
 
     def run(self):
         main_info = get_filesystem_for_path("/home", self._mounts_file,
-                                            self._statvfs, STABLE_FILESYSTEMS)
+                                            self._statvfs)
         if main_info is not None:
             total = main_info["total-space"]
             if total <= 0:
                 root_main_info = get_filesystem_for_path(
-                    "/", self._mounts_file, self._statvfs, STABLE_FILESYSTEMS)
+                    "/", self._mounts_file, self._statvfs)
                 if root_main_info is not None:
                     total = root_main_info["total-space"]
                     main_info = root_main_info
@@ -59,8 +53,7 @@
 
         seen_mounts = set()
         seen_devices = set()
-        infos = list(get_mount_info(self._mounts_file, self._statvfs,
-                                    STABLE_FILESYSTEMS))
+        infos = list(get_mount_info(self._mounts_file, self._statvfs))
         infos.sort(key=lambda i: len(i["mount-point"]))
         for info in infos:
             total = info["total-space"]