Merge ~nkoltsov/charm-nrpe:master into charm-nrpe:master

[Nikita Koltsov]

Add support for nested children sections in lsblk

Modified check_space definitions to support nested children sections
in lsblk output, as well as fixing Jammy support

Fix: LP#1986654, LP#2033521

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:e5277bf27fcce9def6bfa86df08a2b3716805c0d
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/50/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1683/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/470/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/50//rebuild

review: Approve (continuous-integration)

[Robert Gildein (rgildein)]

Overall it looks good to me.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:3b0ba6b46d3ee8f2f45fcc56d586fbf8da69cbb9
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/51/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1684/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/471/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/51//rebuild

review: Approve (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:9075b7e3fad0a516d188228d1b8b714c0bccba3b
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/52/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1685/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/472/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/52//rebuild

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

I really like your recursive solution, it's much cleaner than the previous one!
I have added a couple of improvement suggestions, but +1 overall

review: Needs Fixing

[Eric Chen (eric-chen)]

Pleae check my inline comment. thanks

review: Needs Fixing

[Nikita Koltsov (nkoltsov)]

Ack, will fix all comments soon

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:9747cf9dda16539c075b8a0e15dece93c2254e51
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/53/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1688/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/475/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/53//rebuild

review: Needs Fixing (continuous-integration)

[Eric Chen (eric-chen)]

It's possible to filter the list in opposite without changing the implementation.

https://stackoverflow.com/questions/33989155/is-there-a-filter-opposite-builtin
negative = filter(lambda v: not some_test(v), values)

Therefore, I still prefer is_kubernetes_pv is better than is_not_kubernetes_pv. Sorry that I really don't like a function with opposite logic. When you are tired, it will be a place to push you into hell.

Need 3rd opinion. thanks!

Other than that, the code LGTM.

[Robert Gildein (rgildein)]

To me the whole `is_kubernetes_pv(mountpoint)` is useless, because I would
rather see usage of set comprehension instead of filter. I believe that filter
should be dropped from Python.

But those are just preferences :).

LGTM

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:9747cf9dda16539c075b8a0e15dece93c2254e51
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/54/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1689/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/476/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/54//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

review: Needs Information

[Andrea Ieri (aieri)]

lgtm

@eric-chen: there's also filterfalse from itertools if you want to filter() on a false condition

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision e8acb6598c65094211dc23c672ce713acbb699f3

diff --git a/hooks/nrpe_helpers.py b/hooks/nrpe_helpers.py
index c529ca7..57963a8 100644
--- a/hooks/nrpe_helpers.py
+++ b/hooks/nrpe_helpers.py
@@ -815,9 +815,25 @@ def is_valid_partition(device):
     return True
 
 
-def is_kubernetes_pv(mountpoint):
+def is_not_kubernetes_pv(mountpoint):
     """Check if a mountpoint is Kubernetes persistent volume."""
-    return mountpoint.startswith("/var/lib/kubelet/pods")
+    return not mountpoint.startswith("/var/lib/kubelet/pods")
+
+
+def process_block_devices(devices):
+    """Recursively process all sections in lsblk output."""
+    partitions_to_check = set()
+
+    for dev in devices:
+        # Jammy returns a list of "mountpoints" instead of a single value
+        # in the key "mountpoint"
+        mountpoints = dev.get("mountpoints", []) or [dev.get("mountpoint")]
+        if is_valid_partition(dev):
+            partitions_to_check.update(mountpoints)
+        children = dev.get("children", [])
+        partitions_to_check.update(process_block_devices(children))
+
+    return partitions_to_check
 
 
 def get_partitions_to_check():
@@ -825,27 +841,15 @@ def get_partitions_to_check():
     lsblk_cmd = "lsblk -J".split()
     lsblk_output = subprocess.check_output(lsblk_cmd).decode("UTF-8")
     block_devices = json.loads(lsblk_output).get("blockdevices", [])
-    partitions_to_check = set()
 
-    for dev in block_devices:
-        if not is_valid_partition(dev):
-            continue
-        mnt = dev.get("mountpoint", "")
-        if mnt and not is_kubernetes_pv(mnt):
-            partitions_to_check.add(mnt)
-        for child in dev.get("children", []):
-            if not is_valid_partition(child):
-                continue
-            # Jammy returns a list of "mountpoints" instead of a single value
-            # in the key "mountpoint"
-            mountpoints = child.get("mountpoints", []) or [child.get("mountpoint")]
-            for mnt in filter(bool, mountpoints):
-                if not is_kubernetes_pv(mnt):
-                    partitions_to_check.add(mnt)
+    partitions_to_check = process_block_devices(block_devices)
+
+    skipped_partitions = {"[SWAP]", "/boot/efi", None}
+    partitions_to_check = partitions_to_check - skipped_partitions
 
-    skipped_partitions = {"[SWAP]", "/boot/efi"}
+    partitions_to_check = set(filter(is_not_kubernetes_pv, partitions_to_check))
 
-    return partitions_to_check - skipped_partitions
+    return partitions_to_check
 
 
 def match_cidr_to_ifaces(cidr):
diff --git a/tests/unit/test_nrpe_helpers.py b/tests/unit/test_nrpe_helpers.py
index c7e6cd0..7af52aa 100644
--- a/tests/unit/test_nrpe_helpers.py
+++ b/tests/unit/test_nrpe_helpers.py
@@ -218,7 +218,7 @@ class TestDiskSpaceCheck(unittest.TestCase):
                         "mountpoint": "/boot/efi"
                     },
                     {
-                        "name": "nvme0n1p4",
+                        "name": "nvme0n1p2",
                         "maj:min": "259:4",
                         "rm": false,
                         "size": "100G",
@@ -227,7 +227,7 @@ class TestDiskSpaceCheck(unittest.TestCase):
                         "mountpoint": "/"
                     },
                     {
-                        "name": "nvme0n1p5",
+                        "name": "nvme0n1p3",
                         "maj:min": "259:5",
                         "rm": false,
                         "size": "4G",
@@ -236,58 +236,57 @@ class TestDiskSpaceCheck(unittest.TestCase):
                         "mountpoint": "[SWAP]"
                     },
                     {
-                        "name": "nvme0n1p6",
+                        "name": "nvme0n1p4",
                         "maj:min": "259:6",
                         "rm": false,
                         "size": "1000M",
                         "ro": false,
                         "type": "part",
                         "mountpoint": "/srv/instances"
+                    },
+                    {
+                        "name": "nvme0n1p5",
+                        "maj:min": "259:6",
+                        "rm": false,
+                        "size": "500G",
+                        "ro": false,
+                        "type": "part",
+                        "mountpoints": [
+                            null
+                        ],
+                        "children": [
+                            {
+                                "name": "vg0-var--log--audit",
+                                "maj:min": "253:1",
+                                "rm": "0",
+                                "size": "51.2G",
+                                "ro": "0",
+                                "type": "lvm",
+                                "mountpoint": "/var/log/audit"
+                            },
+                            {
+                                "name": "vg0-var--log",
+                                "maj:min": "253:3",
+                                "rm": "0",
+                                "size": "93.1G",
+                                "ro": "0",
+                                "type": "lvm",
+                                "mountpoint": "/var/log"
+                            }
+                        ]
                     }
                 ]
             },
             {
-             "name": "vda",
-             "maj:min": "252:0",
-             "rm": false,
-             "size": "20G",
-             "ro": false,
-             "type": "disk",
-             "mountpoints": [
-                 null
-             ],
-             "children": [
-                {
-                   "name": "vda1",
-                   "maj:min": "252:1",
-                   "rm": false,
-                   "size": "19.9G",
-                   "ro": false,
-                   "type": "part",
-                   "mountpoints": [
-                       "/srv/jammy"
-                   ]
-                },{
-                   "name": "vda14",
-                   "maj:min": "252:14",
-                   "rm": false,
-                   "size": "4M",
-                   "ro": false,
-                   "type": "part",
-                   "mountpoints": [
-                       null
-                   ]
-                }
-             ]
-            },
-            {
-             "name":"vdb",
-             "maj:min":"252:16",
-             "rm":false,
-             "size":"1G",
-             "ro":false,
-             "type":"disk",
-             "mountpoint": "/var/lib/kubelet/pods/../k..s.io~csi/pvc../mount"
+                "name":"vdb",
+                "maj:min":"252:16",
+                "rm":false,
+                "size":"1G",
+                "ro":false,
+                "type":"disk",
+                "mountpoints": [
+                    "/var/lib/kubelet/pods/../k..s.io~csi/pvc../mount"
+                ]
             }
         ]
     }"""
@@ -295,18 +294,25 @@ class TestDiskSpaceCheck(unittest.TestCase):
     @mock.patch("subprocess.check_output", return_value=lsblk_output)
     def test_get_partitions_to_check(self, lock_lsblk_output):
         """Test the list of partitions to check."""
-        result = nrpe_helpers.get_partitions_to_check()
+        partitions = nrpe_helpers.get_partitions_to_check()
         params = [
-            ("SWAP", False),
+            ("/snap/charm/602", False),
             ("/boot/efi", False),
             ("/", True),
+            ("SWAP", False),
             ("/srv/instances", True),
-            ("/srv/jammy", True),
+            ("/var/log/audit", True),
+            ("/var/log", True),
             ("/var/lib/kubelet/pods/../k..s.io~csi/pvc../mount", False),
         ]
-        for p1, p2 in params:
-            with self.subTest(msg="Validate partition filtering", p1=p1, p2=p2):
-                self.assertEqual(p1 in result, p2)
+        for partition, expected in params:
+            with self.subTest(
+                msg="Validate partition filtering", p1=partition, p2=expected
+            ):
+                if expected:
+                    self.assertIn(partition, partitions)
+                else:
+                    self.assertNotIn(partition, partitions)
 
 
 def load_default_config():