Repository: saltstack/salt
Pull request: Disk grains should not show virtual devices fix issue 57612
### What does this PR do?
Changes the disks.py to populate "disks" and "SSDs" only with real devices, skipping virtual devices like devmaps, loopback and RAM disks.

### What issues does this PR fix or reference?
Fixes: #57612 

### Previous Behavior
In a machine with only one disk the grains shows:

```
# salt-call grains.get disks
local:
    - loop1
    - dm-1
    - loop6
    - dm-6
    - loop4
    - dm-4
    - loop2
    - dm-2
    - loop0
    - dm-0
    - loop7
    - dm-7
    - sda
    - loop5
    - dm-5
    - loop3
    - dm-3
# salt-call grains.get SSDs
local:
    - ram2
    - ram0
    - ram9
    - ram14
    - ram7
    - ram12
    - ram5
    - ram10
    - ram3
    - ram1
    - ram15
    - ram8
    - ram13
    - ram6
    - ram11
    - ram4
```

### New Behavior
Now it shows only the real device:

```
# salt-call grains.get SSDs
local:
# salt-call grains.get disks
local:
    - sda
```

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
[salt-runtests-20200609201317.log](https://github.com/saltstack/salt/files/4755812/salt-runtests-20200609201317.log)

### Commits signed with GPG?
No

Problem statement:
[BUG] Disk grains reports virtual block devices as "disks" or "SSDs"
**Description**
Virtual block devices, like loopbacks, ram disks and device-mappers were show as real devices.

The loopback and device-mapper devices are in the "disks" grain:

```
# salt-call grains.get disks
local:
    - loop1
    - dm-1
    - loop6
    - dm-6
    - loop4
    - dm-4
    - loop2
    - dm-2
    - loop0
    - dm-0
    - loop7
    - dm-7
    - sda
    - loop5
    - dm-5
    - loop3
    - dm-3
```

And the RAM disks in the "SSDs" grain:

```
# salt-call grains.get SSDs
local:
    - ram2
    - ram0
    - ram9
    - ram14
    - ram7
    - ram12
    - ram5
    - ram10
    - ram3
    - ram1
    - ram15
    - ram8
    - ram13
    - ram6
    - ram11
    - ram4
```

**Setup**
Linux machine with only one physical disk, no SSDs:

```
# lsblk -d
NAME MAJ:MIN RM   SIZE RO TYPE MOUNTPOINT
sda    8:0    0 298.1G  0 disk
```

**Steps to Reproduce the behavior**
Any Linux machine with LVM volumes or loopback devices.

**Expected behavior**
The disk grains would return only the existing disk, the sda.

**Versions Report**
<details><summary>salt --versions-report</summary>
(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) 

```
Salt Version:
           Salt: 3001
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.11.2
        libgit2: Not Installed
       M2Crypto: 0.35.2
           Mako: 1.1.3
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: 3.9.7
         pygit2: Not Installed
         Python: 3.8.3 (default, May 15 2020, 02:05:39)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 18.1.1
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.2
 
System Versions:
           dist: slackware 14.2 current
         locale: utf-8
        machine: x86_64
        release: 5.4.43
         system: Linux
        version: Slackware 14.2 current
```
</details>

Hints from issue discussion:
@piterpunk Thank you for reporting this issue and for the PR.

Thanks.
Thank you @piterpunk I have labeled this for the point release to Sodium, since our PR process is lengthy this seems more likely to pass PR testing and be merged for the following release and where we have the severity set to Medium this also seems to fit. Please give us feedback and again thank you for reporting the issue and submitting the PR.
Great! To me it's ok to have a Medium severity. While waiting the next release, I will continue using my custom version.

Commit message: Added unit tests for disks._linux_disks

- Changed the classname to DisksGrainsTestCase. Previously it was
  IscsiGrainsTestCase, but no code related to iscsi is here.
- Added the test__linux_disks to test disks._linux_disks. To do a
  faithful test, it's included an array with data from many real block
  devices: SATA, Fiber Channel, VirtIO, ASM, Loopback, LVM and Ramdisk

Patch to review:
diff --git a/salt/grains/disks.py b/salt/grains/disks.py
index aa72262348be..6de7c8b5ad59 100644
--- a/salt/grains/disks.py
+++ b/salt/grains/disks.py
@@ -133,23 +133,25 @@ def _linux_disks():
     """
     ret = {"disks": [], "SSDs": []}
 
-    for entry in glob.glob("/sys/block/*/queue/rotational"):
+    for entry in glob.glob("/sys/block/*"):
+        virtual = salt.utils.path.readlink(entry).startswith("../devices/virtual/")
         try:
-            with salt.utils.files.fopen(entry) as entry_fp:
-                device = entry.split("/")[3]
-                flag = entry_fp.read(1)
-                if flag == "0":
-                    ret["SSDs"].append(device)
-                    log.trace("Device %s reports itself as an SSD", device)
-                elif flag == "1":
-                    ret["disks"].append(device)
-                    log.trace("Device %s reports itself as an HDD", device)
-                else:
-                    log.trace(
-                        "Unable to identify device %s as an SSD or HDD. It does "
-                        "not report 0 or 1",
-                        device,
-                    )
+            if not virtual:
+                with salt.utils.files.fopen(entry + "/queue/rotational") as entry_fp:
+                    device = entry.split("/")[3]
+                    flag = entry_fp.read(1)
+                    if flag == "0":
+                        ret["SSDs"].append(device)
+                        log.trace("Device %s reports itself as an SSD", device)
+                    elif flag == "1":
+                        ret["disks"].append(device)
+                        log.trace("Device %s reports itself as an HDD", device)
+                    else:
+                        log.trace(
+                            "Unable to identify device %s as an SSD or HDD. It does "
+                            "not report 0 or 1",
+                            device,
+                        )
         except IOError:
             pass
     return ret
diff --git a/tests/unit/grains/test_disks.py b/tests/unit/grains/test_disks.py
index 8a59170f0a57..f2c460a6efa8 100644
--- a/tests/unit/grains/test_disks.py
+++ b/tests/unit/grains/test_disks.py
@@ -12,13 +12,13 @@
 
 # Import Salt Testing Libs
 from tests.support.mixins import LoaderModuleMockMixin
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, mock_open, patch
 from tests.support.unit import TestCase
 
 
-class IscsiGrainsTestCase(TestCase, LoaderModuleMockMixin):
+class DisksGrainsTestCase(TestCase, LoaderModuleMockMixin):
     """
-    Test cases for _windows_disks grains
+    Test cases for disks grains
     """
 
     def setup_loader_modules(self):
@@ -83,3 +83,43 @@ def test__windows_disks_retcode(self):
             result = disks._windows_disks()
             expected = {"SSDs": [], "disks": []}
             self.assertDictEqual(result, expected)
+
+    def test__linux_disks(self):
+        """
+        Test grains._linux_disks, normal return
+        Should return a populated dictionary
+        """
+
+        files = [
+            "/sys/block/asm!.asm_ctl_vbg0",
+            "/sys/block/dm-0",
+            "/sys/block/loop0",
+            "/sys/block/ram0",
+            "/sys/block/sda",
+            "/sys/block/sdb",
+            "/sys/block/vda",
+        ]
+        links = [
+            "../devices/virtual/block/asm!.asm_ctl_vbg0",
+            "../devices/virtual/block/dm-0",
+            "../devices/virtual/block/loop0",
+            "../devices/virtual/block/ram0",
+            "../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda",
+            "../devices/pci0000:35/0000:35:00.0/0000:36:00.0/host2/target2:1:0/2:1:0:0/block/sdb",
+            "../devices/pci0000L00:0000:00:05.0/virtio2/block/vda",
+        ]
+        contents = [
+            "1",
+            "1",
+            "1",
+            "0",
+            "1",
+            "1",
+            "1",
+        ]
+        with patch("glob.glob", MagicMock(return_value=files)), patch(
+            "salt.utils.path.readlink", MagicMock(side_effect=links)
+        ), patch("salt.utils.files.fopen", mock_open(read_data=contents)):
+            ret = disks._linux_disks()
+
+        assert ret == {"disks": ["sda", "sdb", "vda"], "SSDs": []}, ret


Code review comments:
diff_hunk: @@ -83,3 +83,43 @@ def test__windows_disks_retcode(self):
             result = disks._windows_disks()
             expected = {"SSDs": [], "disks": []}
             self.assertDictEqual(result, expected)
+
+    def test__linux_disks(self):
+        """
+        Test grains._linux_disks, normal return
+        Should return a populated dictionary
+        """
+
+        files = [
+            "/sys/block/asm!.asm_ctl_vbg0",
+            "/sys/block/dm-0",
+            "/sys/block/loop0",
+            "/sys/block/ram0",
+            "/sys/block/sda",
+            "/sys/block/sdb",
+            "/sys/block/vda",
+        ]
+        links = [
+            "../devices/virtual/block/asm!.asm_ctl_vbg0",
+            "../devices/virtual/block/dm-0",
+            "../devices/virtual/block/loop0",
+            "../devices/virtual/block/ram0",
+            "../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda",
+            "../devices/pci0000:35/0000:35:00.0/0000:36:00.0/host2/target2:1:0/2:1:0:0/block/sdb",
+            "../devices/pci0000L00:0000:00:05.0/virtio2/block/vda",
+        ]
+        contents = [
+            "1",
+            "1",
+            "1",
+            "0",
+            "1",
+            "1",
+            "1",
+        ]
+        with patch("glob.glob", MagicMock(return_value=files)), patch(
+            "salt.utils.path.readlink", MagicMock(side_effect=links)
+        ), patch("salt.utils.files.fopen", mock_open(read_data=contents)):
path: tests/unit/grains/test_disks.py
text: @user1:
```suggestion
       patch_glob = patch("glob.glob", autospec=True, return_value=files)
       patch_readlink = patch("salt.utils.path.readlink", autospec=True, side_effect=links)
       patch_fopen = patch("salt.utils.files.fopen", mock_open(read_data=contents), autospec=True)
       with patch_glob, patch_readlink, patch_fopen:
```

I'm pretty sure that's the right syntax. Anyway, the most important part of the change is `autospec=True` - basically what that does is ensure that the patched thing looks/behaves like the thing it's patching - so if the underlying API changes then this test will also (correctly) break. IIRC, patch will produce a MagicMock by default - you can just give it a return_value or side_effect.