[disk] Fix disk check exclusion (#2476)

* fixed exclusion logic

* added tests

* fixed comment string

checks.d/disk.py

@@ -135,13 +135,30 @@ def _exclude_disk_psutil(self, part):
                                           part.fstype == '')) or
                 self._exclude_disk(part.device, part.fstype, part.mountpoint))
 
-    # We don't want all those incorrect devices
     def _exclude_disk(self, name, filesystem, mountpoint):
-        return (((not name or name == 'none') and not self._all_partitions) or
-                name in self._excluded_disks or
-                self._excluded_disk_re.match(name) or
-                self._excluded_mountpoint_re.match(mountpoint) or
-                filesystem in self._excluded_filesystems)
+        """
+        Return True for disks we don't want or that match regex in the config file
+        """
+        name_empty = not name or name == 'none'
+
+        # allow empty names if `all_partitions` is `yes` so we can evaluate mountpoints
+        if name_empty and not self._all_partitions:
+            return True
+        # device is listed in `excluded_disks`
+        elif not name_empty and name in self._excluded_disks:
+            return True
+        # device name matches `excluded_disk_re`
+        elif not name_empty and self._excluded_disk_re.match(name):
+            return True
+        # device mountpoint matches `excluded_mountpoint_re`
+        elif self._excluded_mountpoint_re.match(mountpoint):
+            return True
+        # fs is listed in `excluded_filesystems`
+        elif filesystem in self._excluded_filesystems:
+            return True
+        # all good, don't exclude the disk
+        else:
+            return False
 
     def _collect_part_metrics(self, part, usage):
         metrics = {}

tests/checks/mock/test_disk.py

@@ -61,7 +61,7 @@ class TestCheckDisk(AgentCheckTest):
     def test_device_exclusion_logic(self):
         self.run_check({'instances': [{'use_mount': 'no',
                                        'excluded_filesystems': ['aaaaaa'],
-                                       'excluded_mountpoint_re': '^/z+$',
+                                       'excluded_mountpoint_re': '^/run$',
                                        'excluded_disks': ['bbbbbb'],
                                        'excluded_disk_re': '^tev+$'}]},
                        mocks={'collect_metrics': lambda: None})
@@ -86,16 +86,28 @@ def test_device_exclusion_logic(self):
         self.assertTrue(exclude_disk(MockPart(device='tevvv')))
         self.assertFalse(exclude_disk(MockPart(device='tevvs')))
 
-        # excluded mountpoint regex
-        self.assertTrue(exclude_disk(MockPart(device='sdz', mountpoint='/zz')))
-        self.assertFalse(exclude_disk(MockPart(device='sdz', mountpoint='/zy')))
-
         # and now with all_partitions
         self.check._all_partitions = True
         self.assertFalse(exclude_disk(MockPart(device='')))
         self.assertFalse(exclude_disk(MockPart(device='none')))
         self.assertFalse(exclude_disk(MockPart(device='udev')))
 
+        # excluded mountpoint regex
+        self.assertTrue(exclude_disk(MockPart(device='sdz', mountpoint='/run')))
+        self.assertFalse(exclude_disk(MockPart(device='sdz', mountpoint='/run/shm')))
+
+    def test_device_exclusion_logic_no_name(self):
+        """
+        Same as above but with default configuration values and device='' to expose a bug in #2359
+        """
+        self.run_check({'instances': [{'use_mount': 'yes',
+                                       'excluded_mountpoint_re': '^/run$',
+                                       'all_partitions': 'yes'}]},
+                       mocks={'collect_metrics': lambda: None}, force_reload=True)
+        exclude_disk = self.check._exclude_disk_psutil
+        self.assertTrue(exclude_disk(MockPart(device='', mountpoint='/run')))
+        self.assertFalse(exclude_disk(MockPart(device='', mountpoint='/run/shm')))
+
     @mock.patch('psutil.disk_partitions', return_value=[MockPart()])
     @mock.patch('psutil.disk_usage', return_value=MockDiskMetrics())
     @mock.patch('os.statvfs', return_value=MockInodesMetrics())