Merge pull request rockstor#2880 from phillxnet/2844-Disk-activity-wi…

…dget-inactive---Tumbleweed

Disk activity widget inactive - Tumbleweed rockstor#2844

src/rockstor/smart_manager/data_collector.py

@@ -23,6 +23,8 @@
 
 from gevent import monkey
 
+from system.constants import BLOCK_DEV_EXCLUDE
+
 monkey.patch_all()
 
 from fs.btrfs import degraded_pools_found
@@ -616,16 +618,22 @@ def disk_stats(prev_stats):
             cur_stats = {}
             interval = 1
             # TODO: Consider refactoring the following to use Disk.temp_name or
-            # TODO: building byid_disk_map from the same. Ideally we would have
-            # TODO: performance testing in place prior to this move.
+            #  building byid_disk_map from the same. Ideally we would have
+            #  performance testing in place prior to this move.
             # Build a list of our db's disk names, now in by-id type format.
             disks = [d.name for d in Disk.objects.all()]
-            # /proc/diskstats has lines of the following form:
+            # Older Leap kernels: /proc/diskstats has lines of the following form:
             #  8      64 sde 1034 0 9136 702 0 0 0 0 0 548 702
             #  8      65 sde1 336 0 2688 223 0 0 0 0 0 223 223
+            # TW (2024): more fields, and loop block device entries.
+            #  8       0 sda 2507 0 101712 388 0 0 0 0 0 264 388 0 0 0 0 0 0
+            #  7       0 loop0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
             with open(stats_file_path) as stats_file:
                 for line in stats_file.readlines():
                     fields = line.split()
+                    # Sanity check: available fields and block device blacklist
+                    if len(fields) < 14 or fields[0] in BLOCK_DEV_EXCLUDE:
+                        continue
                     # As the /proc/diskstats lines contain transient type names
                     # we need to convert those to our by-id db names.
                     byid_name = self.byid_disk_map[fields[2]]

src/rockstor/system/constants.py

@@ -37,3 +37,10 @@
 SHUTDOWN = "/sbin/shutdown"
 
 TAILSCALE = "/usr/bin/tailscale"
+
+# Major block device number:str to ignore (commonality ordered):
+# https://www.kernel.org/doc/Documentation/admin-guide/devices.txt
+# 7: Loopback
+# 11: SCSI CD-ROM
+# 2: Floppy disks
+BLOCK_DEV_EXCLUDE: list[str] = ["7", "11", "2"]

src/rockstor/system/osi.py

@@ -304,6 +304,7 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]:
     :return: List containing Disk: namedtuple members of interest.
     """
     base_root_disk = root_disk()  # /dev/sda if /dev/sda3, or md126 if md126p2
+    # TODO Add MAJ:MIN in concert with system.constants BLOCK_DEV_EXCLUDE
     cmd = [
         LSBLK,
         "-P",
@@ -353,7 +354,7 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]:
         if dev.name in device_names_seen:
             continue
         device_names_seen.append(dev.name)
-        # We are not interested in CD / DVD rom devices.
+        # We are not interested in CD / DVD rom devices, see: BLOCK_DEV_EXCLUDE.
         if dev.type == "rom":
             continue
         # We are not interested in swap devices.
@@ -373,6 +374,7 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]:
             continue
         # ----- Now we are done with easy exclusions we begin classification.
         # If md device, populate otherwise unused MODEL with basic member/raid summary.
+        # /dev/md devices have major id "9"
         if re.match("/dev/md", dev.name) is not None:
             # cheap way to display our member drives
             dev = dev._replace(model=get_md_members(dev.name))
@@ -1743,20 +1745,20 @@ def get_dev_byid_name(device_name, remove_path=False):
     return return_name, is_byid
 
 
-def get_byid_name_map():
+def get_byid_name_map() -> dict:
     """Simple wrapper around 'ls -lr /dev/disk/by-id' which returns a current
-    mapping of all attached by-id device names to their sdX counterparts. When
-    multiple by-id names are found for the same sdX device then the longest is
+    mapping of all attached by-id device names to their base counterparts. When
+    multiple by-id names are found for the same base device then the longest is
     preferred, or when equal in length then the first listed is used. Intended
-    as a light weight helper for the Dashboard disk activity widget or other
-    non critical components. For critical components use only:
+    as a light-weight helper for the Dashboard disk activity widget or other
+    non-critical components. For critical components use only:
     get_dev_byid_name() and get_devname() as they contain sanity checks and
     validation mechanisms and are intended to have more repeatable behaviour
     but only work on a single device at a time.  A single call to this method
-    can provide all current by-id device names mapped to their sdX counterparts
+    can provide all current by-id device names mapped to their base counterparts
     with the latter being the index.
-    :return: dictionary indexed (keyed) by sdX type names with associated by-id
-    type names as the values, or an empty dictionary if a non zero return code
+    :return: dictionary indexed (keyed) by canonical (base) names with associated by-id
+    type names as the values, or an empty dictionary if a non-zero return code
     was encountered by run_command or no by-id type names were encountered.
     """
     byid_name_map = {}
@@ -1776,8 +1778,7 @@ def get_byid_name_map():
             # Split the line by spaces and '/' chars
             line_fields = each_line.replace("/", " ").split()
             # Grab every sda type name from the last field in the line and add
-            # it as a dictionary key with it's value as the by-id type name so
-            # we can index by sda type name and retrieve the by-id. As there
+            # it as a dictionary key with its value as the by-id type name. As there
             # are often multiple by-id type names for a given sda type name we
             # gain consistency in mapped by-id value by always preferring the
             # longest by-id for a given sda type name key.

src/rockstor/system/tests/test_osi.py

@@ -3585,6 +3585,31 @@ def test_get_byid_name_map(self):
                 "sda": "wwn-0x690b11c0223db60025bb4a6c79bc0d52",
             }
         )
+        # TW 20240716 VM with virtio & SCSI disks, plus SCSI CDROM.
+        out.append(
+            [
+                "total 0",
+                "lrwxrwxrwx 1 root root 10 Jul 23 10:37 virtio-112358-part3 -> ../../vda3",
+                "lrwxrwxrwx 1 root root 10 Jul 23 10:37 virtio-112358-part2 -> ../../vda2",
+                "lrwxrwxrwx 1 root root 10 Jul 23 10:37 virtio-112358-part1 -> ../../vda1",
+                "lrwxrwxrwx 1 root root  9 Jul 23 10:37 virtio-112358 -> ../../vda",
+                "lrwxrwxrwx 1 root root  9 Jul 23 10:37 ata-QEMU_HARDDISK_QM00003 -> ../../sda",
+                "lrwxrwxrwx 1 root root  9 Jul 23 10:37 ata-QEMU_DVD-ROM_QM00001 -> ../../sr0",
+                "",
+            ]
+        )
+        err.append([""])
+        rc.append(0)
+        expected_result.append(
+            {
+                "vda3": "virtio-112358-part3",
+                "vda2": "virtio-112358-part2",
+                "vda1": "virtio-112358-part1",
+                "vda": "virtio-112358",
+                "sda": "ata-QEMU_HARDDISK_QM00003",
+                "sr0": "ata-QEMU_DVD-ROM_QM00001",
+            }
+        )
         for o, e, r, expected in zip(out, err, rc, expected_result):
             self.mock_run_command.return_value = (o, e, r)
             returned = get_byid_name_map()