From 6c73cc0a898df5ae620711edd62988727ed914a7 Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Wed, 24 Jul 2024 13:45:13 -0700 Subject: [PATCH 1/5] Updated the way we collect the zvols. The previous method did not do a depth search. Fixed a few typos in TRUENAS-MIB.txt which includes regenerating TRUENAS-MIB.py. Added a context manager for simple 'file' creations in the filesystem asset. Created a CI test in test_440_snmp.py for this issue. --- src/freenas/usr/local/bin/snmp-agent.py | 9 +- .../local/share/pysnmp/mibs/TRUENAS-MIB.py | 14 +-- .../usr/local/share/snmp/mibs/TRUENAS-MIB.txt | 6 +- .../test/integration/assets/filesystem.py | 22 +++++ tests/api2/test_440_snmp.py | 94 ++++++++++++++++++- 5 files changed, 130 insertions(+), 15 deletions(-) diff --git a/src/freenas/usr/local/bin/snmp-agent.py b/src/freenas/usr/local/bin/snmp-agent.py index d56882f117843..31927431be287 100755 --- a/src/freenas/usr/local/bin/snmp-agent.py +++ b/src/freenas/usr/local/bin/snmp-agent.py @@ -2,13 +2,14 @@ import threading import time import contextlib -import pathlib +import os import libzfs import netsnmpagent import pysnmp.hlapi # noqa import pysnmp.smi +from glob import iglob from truenas_api_client import Client @@ -413,9 +414,9 @@ def get_list_of_zvols(): zvols = set() root_dir = '/dev/zvol/' with contextlib.suppress(FileNotFoundError): # no zvols - for zpool in pathlib.Path(root_dir).iterdir(): - for zvol in filter(lambda x: '@' not in x.name, zpool.iterdir()): - zvol_normalized = zvol.as_posix().removeprefix(root_dir) + for zvol in iglob(root_dir + '**', recursive=True): + if not os.path.isdir(zvol) and '@' not in zvol: + zvol_normalized = zvol.removeprefix(root_dir) zvol_normalized = zvol_normalized.replace('+', ' ') zvols.add(zvol_normalized) diff --git a/src/freenas/usr/local/share/pysnmp/mibs/TRUENAS-MIB.py b/src/freenas/usr/local/share/pysnmp/mibs/TRUENAS-MIB.py index 9e976d5772ab9..c7c1bcdeca1d8 100644 --- a/src/freenas/usr/local/share/pysnmp/mibs/TRUENAS-MIB.py +++ b/src/freenas/usr/local/share/pysnmp/mibs/TRUENAS-MIB.py @@ -1,6 +1,6 @@ # PySNMP SMI module. Autogenerated from smidump -f python TRUENAS-MIB -# by libsmi2pysnmp-0.1.3 at Fri Aug 18 13:42:49 2023, -# Python version sys.version_info(major=2, minor=7, micro=17, releaselevel='final', serial=0) +# by libsmi2pysnmp-0.1.3 at Wed Jul 24 12:51:26 2024, +# Python version sys.version_info(major=3, minor=11, micro=2, releaselevel='final', serial=0) # Imports @@ -13,7 +13,7 @@ # Types class AlertLevelType(Integer): - subtypeSpec = Integer.subtypeSpec+SingleValueConstraint(1,2,3,5,7,4,6,) + subtypeSpec = Integer.subtypeSpec+SingleValueConstraint(1,2,3,4,5,6,7,) namedValues = NamedValues(("info", 1), ("notice", 2), ("warning", 3), ("error", 4), ("critical", 5), ("alert", 6), ("emergency", 7), ) @@ -80,11 +80,11 @@ class AlertLevelType(Integer): zfsArcC = MibScalar((1, 3, 6, 1, 4, 1, 50536, 1, 3, 6), Gauge32()).setMaxAccess("readonly") if mibBuilder.loadTexts: zfsArcC.setDescription("") zfsArcMissPercent = MibScalar((1, 3, 6, 1, 4, 1, 50536, 1, 3, 8), DisplayString()).setMaxAccess("readonly") -if mibBuilder.loadTexts: zfsArcMissPercent.setDescription("Arc Miss Percentage.\n(Note: Floating precision sent across SNMP as a String") +if mibBuilder.loadTexts: zfsArcMissPercent.setDescription("Arc Miss Percentage.\nNote: Floating precision sent across SNMP as a String") zfsArcCacheHitRatio = MibScalar((1, 3, 6, 1, 4, 1, 50536, 1, 3, 9), DisplayString()).setMaxAccess("readonly") -if mibBuilder.loadTexts: zfsArcCacheHitRatio.setDescription("Arc Cache Hit Ration Percentage.\n(Note: Floating precision sent across SNMP as a String") +if mibBuilder.loadTexts: zfsArcCacheHitRatio.setDescription("Arc Cache Hit Ration Percentage.\nNote: Floating precision sent across SNMP as a String") zfsArcCacheMissRatio = MibScalar((1, 3, 6, 1, 4, 1, 50536, 1, 3, 10), DisplayString()).setMaxAccess("readonly") -if mibBuilder.loadTexts: zfsArcCacheMissRatio.setDescription("Arc Cache Miss Ration Percentage.\n(Note: Floating precision sent across SNMP as a String") +if mibBuilder.loadTexts: zfsArcCacheMissRatio.setDescription("Arc Cache Miss Ration Percentage.\nNote: Floating precision sent across SNMP as a String") l2arc = MibIdentifier((1, 3, 6, 1, 4, 1, 50536, 1, 4)) zfsL2ArcHits = MibScalar((1, 3, 6, 1, 4, 1, 50536, 1, 4, 1), Counter32()).setMaxAccess("readonly") if mibBuilder.loadTexts: zfsL2ArcHits.setDescription("") @@ -127,7 +127,7 @@ class AlertLevelType(Integer): # Notifications -alert = NotificationType((1, 3, 6, 1, 4, 1, 50536, 2, 1, 1)).setObjects(*(("TRUENAS-MIB", "alertMessage"), ("TRUENAS-MIB", "alertLevel"), ("TRUENAS-MIB", "alertId"), ) ) +alert = NotificationType((1, 3, 6, 1, 4, 1, 50536, 2, 1, 1)).setObjects(*(("TRUENAS-MIB", "alertId"), ("TRUENAS-MIB", "alertLevel"), ("TRUENAS-MIB", "alertMessage"), ) ) if mibBuilder.loadTexts: alert.setDescription("An alert raised") alertCancellation = NotificationType((1, 3, 6, 1, 4, 1, 50536, 2, 1, 2)).setObjects(*(("TRUENAS-MIB", "alertId"), ) ) if mibBuilder.loadTexts: alertCancellation.setDescription("An alert cancelled") diff --git a/src/freenas/usr/local/share/snmp/mibs/TRUENAS-MIB.txt b/src/freenas/usr/local/share/snmp/mibs/TRUENAS-MIB.txt index 6e12bec5b9935..994705911160a 100644 --- a/src/freenas/usr/local/share/snmp/mibs/TRUENAS-MIB.txt +++ b/src/freenas/usr/local/share/snmp/mibs/TRUENAS-MIB.txt @@ -293,7 +293,7 @@ zfsArcMissPercent OBJECT-TYPE STATUS current DESCRIPTION "Arc Miss Percentage. - (Note: Floating precision sent across SNMP as a String" + Note: Floating precision sent across SNMP as a String" ::= { arc 8 } zfsArcCacheHitRatio OBJECT-TYPE @@ -302,7 +302,7 @@ zfsArcCacheHitRatio OBJECT-TYPE STATUS current DESCRIPTION "Arc Cache Hit Ration Percentage. - (Note: Floating precision sent across SNMP as a String" + Note: Floating precision sent across SNMP as a String" ::= { arc 9 } zfsArcCacheMissRatio OBJECT-TYPE @@ -311,7 +311,7 @@ zfsArcCacheMissRatio OBJECT-TYPE STATUS current DESCRIPTION "Arc Cache Miss Ration Percentage. - (Note: Floating precision sent across SNMP as a String" + Note: Floating precision sent across SNMP as a String" ::= { arc 10 } zfsL2ArcHits OBJECT-TYPE diff --git a/src/middlewared/middlewared/test/integration/assets/filesystem.py b/src/middlewared/middlewared/test/integration/assets/filesystem.py index e0871dbd06e87..a77eef7f22853 100644 --- a/src/middlewared/middlewared/test/integration/assets/filesystem.py +++ b/src/middlewared/middlewared/test/integration/assets/filesystem.py @@ -11,3 +11,25 @@ def directory(path, options=None): yield path finally: ssh(f'rm -rf {path}') + + +@contextlib.contextmanager +def file(path, size=None): + """ + Create a simple file + * path is the full-pathname. e.g. /mnt/tank/dataset/filename + * If size is None then use 'touch', + else create a random filled file of size bytes. + Creation will be faster if size is a power of 2, e.g. 1024 or 1048576 + """ + try: + if size is None: + ssh(f"touch {path}") + else: + t = 1048576 + while t > 1 and size % t != 0: + t = t // 2 + ssh(f"dd if=/dev/urandom of={path} bs={t} count={size // t}") + yield path + finally: + ssh(f"rm -rf {path}") diff --git a/tests/api2/test_440_snmp.py b/tests/api2/test_440_snmp.py index 4ce613203995a..88b6f4fa8093e 100644 --- a/tests/api2/test_440_snmp.py +++ b/tests/api2/test_440_snmp.py @@ -6,7 +6,10 @@ from time import sleep +from contextlib import ExitStack from middlewared.service_exception import ValidationErrors +from middlewared.test.integration.assets.pool import dataset, snapshot +from middlewared.test.integration.assets.filesystem import directory, file from middlewared.test.integration.utils import call, ssh from middlewared.test.integration.utils.client import truenas_server from middlewared.test.integration.utils.system import reset_systemd_svcs @@ -14,7 +17,7 @@ ObjectType, SnmpEngine, UdpTransportTarget, getCmd) -from auto_config import ha, interface, password, user +from auto_config import ha, interface, password, user, pool_name from functions import async_SSH_done, async_SSH_start skip_ha_tests = pytest.mark.skipif(not (ha and "virtual_ip" in os.environ), reason="Skip HA tests") @@ -97,6 +100,66 @@ def add_SNMPv3_user(): yield +@pytest.fixture(scope='function') +def create_nested_structure(): + """ + Create the following structure: + tank -+-> dataset_1 -+-> dataset2 -+-> dataset_3 + |-> zvol_1a |-> zvol_2a |-> zvol_3a + |-> zvol_1b |-> zvol_2b |-> zvol_3b + |-> file_1 |-> file_2 |-> file_3 + |-> dir_1 |-> dir_2 |-> dir_3 + TODO: Make this generic and move to assets + """ + ds_path = "" + ds_list = [] + zv_list = [] + dir_list = [] + file_list = [] + with ExitStack() as es: + + for i in range(1, 4): + preamble = f"{ds_path + '/' if i > 1 else ''}" + vol_path = f"{preamble}zvol_{i}" + + # Create zvols + for c in crange('a', 'b'): + zv = es.enter_context(dataset(vol_path + c, {"type": "VOLUME", "volsize": 1048576})) + zv_list.append(zv) + + # Create directories + d = es.enter_context(directory(f"/mnt/{pool_name}/{preamble}dir_{i}")) + dir_list.append(d) + + # Create files + f = es.enter_context(file(f"/mnt/{pool_name}/{preamble}file_{i}", 1048576)) + file_list.append(f) + + # Create datasets + ds_path += f"{'/' if i > 1 else ''}dataset_{i}" + ds = es.enter_context(dataset(ds_path)) + ds_list.append(ds) + + yield {'zv': zv_list, 'ds': ds_list, 'dir': dir_list, 'file': file_list} + + +def crange(c1, c2): + """ + Generates the characters from `c1` to `c2`, inclusive. + Simple lowercase ascii only. + NOTE: Not safe for runtime code + """ + ord_a = 97 + ord_z = 122 + c1_ord = ord(c1) + c2_ord = ord(c2) + assert c1_ord < c2_ord, f"'{c1}' must be 'less than' '{c2}'" + assert ord_a <= c1_ord <= ord_z + assert ord_a <= c2_ord <= ord_z + for c in range(c1_ord, c2_ord + 1): + yield chr(c) + + def get_systemctl_status(service): """ Return 'RUNNING' or 'STOPPED' """ try: @@ -173,6 +236,20 @@ def user_list_users(snmp_config): return [x.split()[-1].strip('\"') for x in res.splitlines()] +def v2c_snmpwalk(mib): + """ + Run snmpwalk with v2c protocol + mib is the item to be gathered. mib format examples: + iso.3.6.1.6.3.15.1.2.2.1.3 + 1.3.6.1.4.1.50536.1.2 + """ + cmd = f"snmpwalk -v2c -cpublic localhost {mib}" + + # This call will timeout if SNMP is not running + res = ssh(cmd) + return [x.split()[-1].strip('\"') for x in res.splitlines()] + + # ===================================================================== # Tests # ===================================================================== @@ -349,3 +426,18 @@ def test_SNMPv3_user_delete(self): with pytest.raises(Exception) as ve: res = user_list_users(SNMP_USER_CONFIG) assert "Unknown user name" in str(ve.value) + + def test_zvol_reporting(self, create_nested_structure): + """ + The TrueNAS snmp agent should list all zvols. + TrueNAS zvols can be created on any ZFS pool or dataset. + The snmp agent should list them all. + snmpwalk -v2c -cpublic localhost 1.3.6.1.4.1.50536.1.2.1.1.2 + """ + # The expectation is that the snmp agent should list exactly the six zvols. + created_items = create_nested_structure + + # Include a snapshot of one of the zvols + with snapshot(created_items['zv'][0], "snmpsnap01"): + snmp_res = v2c_snmpwalk('1.3.6.1.4.1.50536.1.2.1.1.2') + assert all(v in created_items['zv'] for v in snmp_res), f"expected {created_items['zv']}, but found {snmp_res}" From ed5680205f2fde580dd7755b39c695e664a8cedf Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Thu, 25 Jul 2024 08:04:17 -0700 Subject: [PATCH 2/5] Changed directory tree traverse from glob to walk Changed name of context manager from file to mkfile Added a couple comments --- src/freenas/usr/local/bin/snmp-agent.py | 10 +++------- .../middlewared/test/integration/assets/filesystem.py | 3 ++- tests/api2/test_440_snmp.py | 4 ++-- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/freenas/usr/local/bin/snmp-agent.py b/src/freenas/usr/local/bin/snmp-agent.py index 31927431be287..cd3c7aea1fa62 100755 --- a/src/freenas/usr/local/bin/snmp-agent.py +++ b/src/freenas/usr/local/bin/snmp-agent.py @@ -9,7 +9,6 @@ import pysnmp.hlapi # noqa import pysnmp.smi -from glob import iglob from truenas_api_client import Client @@ -412,13 +411,10 @@ def report_zfs_info(prev_zpool_info): def get_list_of_zvols(): zvols = set() - root_dir = '/dev/zvol/' with contextlib.suppress(FileNotFoundError): # no zvols - for zvol in iglob(root_dir + '**', recursive=True): - if not os.path.isdir(zvol) and '@' not in zvol: - zvol_normalized = zvol.removeprefix(root_dir) - zvol_normalized = zvol_normalized.replace('+', ' ') - zvols.add(zvol_normalized) + for root_dir, unused_dirs, files in os.walk('/dev/zvol/'): + for file in filter(lambda x: '@' not in x, files): + zvols.add(os.path.join(root_dir, file).removeprefix(root_dir).replace('+', ' ')) return list(zvols) diff --git a/src/middlewared/middlewared/test/integration/assets/filesystem.py b/src/middlewared/middlewared/test/integration/assets/filesystem.py index a77eef7f22853..80e325e6b0160 100644 --- a/src/middlewared/middlewared/test/integration/assets/filesystem.py +++ b/src/middlewared/middlewared/test/integration/assets/filesystem.py @@ -14,13 +14,14 @@ def directory(path, options=None): @contextlib.contextmanager -def file(path, size=None): +def mkfile(path, size=None): """ Create a simple file * path is the full-pathname. e.g. /mnt/tank/dataset/filename * If size is None then use 'touch', else create a random filled file of size bytes. Creation will be faster if size is a power of 2, e.g. 1024 or 1048576 + TODO: sparse files, owner, permissions """ try: if size is None: diff --git a/tests/api2/test_440_snmp.py b/tests/api2/test_440_snmp.py index 88b6f4fa8093e..695d479acbc71 100644 --- a/tests/api2/test_440_snmp.py +++ b/tests/api2/test_440_snmp.py @@ -9,7 +9,7 @@ from contextlib import ExitStack from middlewared.service_exception import ValidationErrors from middlewared.test.integration.assets.pool import dataset, snapshot -from middlewared.test.integration.assets.filesystem import directory, file +from middlewared.test.integration.assets.filesystem import directory, mkfile from middlewared.test.integration.utils import call, ssh from middlewared.test.integration.utils.client import truenas_server from middlewared.test.integration.utils.system import reset_systemd_svcs @@ -132,7 +132,7 @@ def create_nested_structure(): dir_list.append(d) # Create files - f = es.enter_context(file(f"/mnt/{pool_name}/{preamble}file_{i}", 1048576)) + f = es.enter_context(mkfile(f"/mnt/{pool_name}/{preamble}file_{i}", 1048576)) file_list.append(f) # Create datasets From 50f08e48767cbac30d7e247df52acc78c090ab2f Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Thu, 25 Jul 2024 16:26:39 -0700 Subject: [PATCH 3/5] Added zvol name with spaces test. That exposed an issue with processing the SNMP output. Fixed processing of the SNMP output. Fixed other items raised in the review. --- .../test/integration/assets/filesystem.py | 2 +- tests/api2/test_440_snmp.py | 20 ++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/middlewared/middlewared/test/integration/assets/filesystem.py b/src/middlewared/middlewared/test/integration/assets/filesystem.py index 80e325e6b0160..9262a0e2bbcae 100644 --- a/src/middlewared/middlewared/test/integration/assets/filesystem.py +++ b/src/middlewared/middlewared/test/integration/assets/filesystem.py @@ -33,4 +33,4 @@ def mkfile(path, size=None): ssh(f"dd if=/dev/urandom of={path} bs={t} count={size // t}") yield path finally: - ssh(f"rm -rf {path}") + ssh(f"rm -f {path}") diff --git a/tests/api2/test_440_snmp.py b/tests/api2/test_440_snmp.py index 695d479acbc71..eaba458e119dd 100644 --- a/tests/api2/test_440_snmp.py +++ b/tests/api2/test_440_snmp.py @@ -104,11 +104,11 @@ def add_SNMPv3_user(): def create_nested_structure(): """ Create the following structure: - tank -+-> dataset_1 -+-> dataset2 -+-> dataset_3 - |-> zvol_1a |-> zvol_2a |-> zvol_3a - |-> zvol_1b |-> zvol_2b |-> zvol_3b - |-> file_1 |-> file_2 |-> file_3 - |-> dir_1 |-> dir_2 |-> dir_3 + tank -+-> dataset_1 -+-> dataset_2 -+-> dataset_3 + |-> zvol_1a |-> zvol-L_2a |-> zvol L_3a + |-> zvol_1b |-> zvol-L_2b |-> zvol L_3b + |-> file_1 |-> file_2 |-> file_3 + |-> dir_1 |-> dir_2 |-> dir_3 TODO: Make this generic and move to assets """ ds_path = "" @@ -116,11 +116,13 @@ def create_nested_structure(): zv_list = [] dir_list = [] file_list = [] + # Test '-' and ' ' in the name (we skip index 0) + zvol_name = ["bogus", "zvol", "zvol-L", "zvol L"] with ExitStack() as es: for i in range(1, 4): preamble = f"{ds_path + '/' if i > 1 else ''}" - vol_path = f"{preamble}zvol_{i}" + vol_path = f"{preamble}{zvol_name[i]}_{i}" # Create zvols for c in crange('a', 'b'): @@ -233,7 +235,7 @@ def user_list_users(snmp_config): # This call will timeout if SNMP is not running res = ssh(cmd) - return [x.split()[-1].strip('\"') for x in res.splitlines()] + return [x.split(':')[-1].strip(' \"') for x in res.splitlines()] def v2c_snmpwalk(mib): @@ -247,14 +249,14 @@ def v2c_snmpwalk(mib): # This call will timeout if SNMP is not running res = ssh(cmd) - return [x.split()[-1].strip('\"') for x in res.splitlines()] + return [x.split(':')[-1].strip(' \"') for x in res.splitlines()] # ===================================================================== # Tests # ===================================================================== -@pytest.mark.usefixtures("initialize_and_start_snmp") class TestSNMP: + def test_configure_SNMP(self, initialize_and_start_snmp): config = initialize_and_start_snmp From c7f6849b128d7f940f0914fca1e58cb195a8fff1 Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Fri, 26 Jul 2024 10:34:10 -0700 Subject: [PATCH 4/5] Fix zvol collection. --- src/freenas/usr/local/bin/snmp-agent.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/freenas/usr/local/bin/snmp-agent.py b/src/freenas/usr/local/bin/snmp-agent.py index cd3c7aea1fa62..2c4540a111977 100755 --- a/src/freenas/usr/local/bin/snmp-agent.py +++ b/src/freenas/usr/local/bin/snmp-agent.py @@ -411,10 +411,11 @@ def report_zfs_info(prev_zpool_info): def get_list_of_zvols(): zvols = set() + root_dir = '/dev/zvol' with contextlib.suppress(FileNotFoundError): # no zvols - for root_dir, unused_dirs, files in os.walk('/dev/zvol/'): + for dir_path, unused_dirs, files in os.walk(root_dir): for file in filter(lambda x: '@' not in x, files): - zvols.add(os.path.join(root_dir, file).removeprefix(root_dir).replace('+', ' ')) + zvols.add(os.path.join(dir_path, file).removeprefix(root_dir).replace('+', ' ')) return list(zvols) From 11346955058876c6ed440310c1db1edf24171e7c Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Fri, 26 Jul 2024 13:01:30 -0700 Subject: [PATCH 5/5] Add trailing slash to root_dir --- src/freenas/usr/local/bin/snmp-agent.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/freenas/usr/local/bin/snmp-agent.py b/src/freenas/usr/local/bin/snmp-agent.py index 2c4540a111977..daa6af5f7f2ed 100755 --- a/src/freenas/usr/local/bin/snmp-agent.py +++ b/src/freenas/usr/local/bin/snmp-agent.py @@ -411,7 +411,7 @@ def report_zfs_info(prev_zpool_info): def get_list_of_zvols(): zvols = set() - root_dir = '/dev/zvol' + root_dir = '/dev/zvol/' with contextlib.suppress(FileNotFoundError): # no zvols for dir_path, unused_dirs, files in os.walk(root_dir): for file in filter(lambda x: '@' not in x, files):