[GSoC'25]: Replace bottleneck multiprocessing with async for device polling #334
Conversation
|
Warning Rate limit exceeded@abbi4code has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces multiprocessing-based polling with an asynchronous poller using asyncio and aiohttp. Adds async control flow, updates daemon invocation, adjusts SNMP async utilities, and removes legacy synchronous SNMP manager/info/poller modules and a test script. requirements.txt adds aiohttp. Public APIs shift from poll.* to async_poll.* and configuration concurrency accessor is renamed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Operator
participant Daemon as systemd switchmap_poller
participant AsyncPoller as async_poll.run_devices()
participant SNMP as SNMP Async Layer
participant REST as REST API
Operator->>Daemon: Start service loop
Daemon->>AsyncPoller: Resolve max_concurrent via ConfigPoller.agent_subprocesses()
AsyncPoller->>AsyncPoller: Build device list per zone
AsyncPoller->>AsyncPoller: Create Semaphore(limit) + aiohttp Session
par For each device (bounded by Semaphore)
AsyncPoller->>SNMP: Initialize per-host SNMP and query
SNMP-->>AsyncPoller: SNMP data or error
alt Valid data
AsyncPoller->>REST: POST payload (JSON)
REST-->>AsyncPoller: HTTP 200/!200
else No data / error
AsyncPoller-->>AsyncPoller: Log failure
end
end
AsyncPoller-->>Daemon: Summary counts (success/fail/exception)
Daemon-->>Operator: Continue loop (next interval)
sequenceDiagram
autonumber
actor User
participant CLI as async_poll.run_cli_device(host)
participant SNMP as SNMP Async Layer
User->>CLI: Invoke CLI device
CLI->>CLI: Find hostname across zones
alt Found
CLI->>SNMP: Query host (single-concurrency)
SNMP-->>CLI: Data
CLI-->>User: Pretty-print data
else Not found
CLI-->>User: Log not found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if either of these two conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
🎯 Contacting the person who assigned the mentors is not advised unless they make a request. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🔭 Outside diff range comments (9)
switchmap/poller/snmp/mib/generic/mib_if_64.py (1)
73-87: Convert entiremib_if_64.pyto async to avoid blocking the event loopThis module currently mixes one async method with many sync ones, which will block when used in the async poller:
• File:
switchmap/poller/snmp/mib/generic/mib_if_64.py
– Line 73:async def system(self)
– Line 89:def layer1(self)remains synchronous
– Lines 132–372: helper methods (e.g.ifhighspeed,ifhcinucastpkts, …) callself.snmp_object.swalk(...)synchronously
– Line 406:_get_data(title, func, dest)is also syncAction items:
- Change
layer1toasync def layer1(self)and have itawaiteach helper.- Convert all helper functions to
async def, callingresults = await self.snmp_object.swalk(...).- Replace
_get_datawith anasync def _get_data_async(title, func, dest)thatawaits the helper and merges results.- Update
system()toawait layer1()instead of returning an empty dict.With these changes, the poller can run end-to-end without blocking the event loop.
switchmap/poller/snmp/base_query.py (1)
47-66: Fix missing awaits on async supported()The
supported()method is now asynchronous, but multiple call sites still invoke it withoutawait, causing coroutines to be used in boolean contexts and list comprehensions instead of actualboolvalues. Please update the following locations:• In
switchmap/poller/snmp/async_snmp_info.py:
– Lines 103, 154, 211, 272: change
python *[item.supported() for item, _ in query_items]
to something like
python *await asyncio.gather(*(item.supported() for item, _ in query_items))• In
switchmap/poller/snmp/snmp_info.py:
– Lines 99, 128, 155, 182: change
python if item.supported():
to
python if await item.supported():• Update tests that call
testobj.supported()synchronously (e.g. intests/.../test_*_mib_*.pyandtests/switchmap_/poller/snmp/test_base_query.py):
replaceself.assertEqual(testobj.supported(), True)with
self.assertTrue(await testobj.supported())(and similarly for
Falseassertions).These changes will ensure
supported()is properly awaited and returns the intended boolean results.switchmap/poller/snmp/mib/cisco/mib_ciscoietfip.py (2)
71-90: Avoid truncating MACs; store canonical valueSlicing to mac_value[:12] may truncate colon-separated forms or future formats. Prefer storing the canonical hex without separators.
- for key, mac_value in values.items(): - final["cInetNetToMediaPhysAddress"][key] = mac_value[:12] + for key, mac_value in values.items(): + # Store canonical 12-hex MAC (no separators) + mac_hex = mac_value.replace(":", "").lower() if isinstance(mac_value, str) else str(mac_value) + final["cInetNetToMediaPhysAddress"][key] = mac_hex[:12]
106-124: Fix IPv6 formatting: should be 8 groups (2 bytes each), not 16 single-byte groupsCurrent code joins 16 one-byte hex tokens, producing an invalid IPv6 format. Build 8 groups by pairing bytes.
- # Convert IP address from decimal to hex + # Convert IP address from decimal to hex nodes = key.split(".") ipv6decimal = nodes[-16:] ipv6hex = [] for value in ipv6decimal: - # Convert deximal value to hex, + # Convert decimal value to hex, # then zero fill to ensure hex is two characters long hexbyte = "{}".format(hex(int(value)))[2:] ipv6hex.append(hexbyte.zfill(2)) - # Create IPv6 string - ipv6 = ":".join(ipv6hex) + # Pair bytes into 8 groups (two bytes per group) + ipv6_groups = [ + f"{ipv6hex[i]}{ipv6hex[i+1]}" for i in range(0, len(ipv6hex), 2) + ] + # Create IPv6 string (uncompressed) + ipv6 = ":".join(ipv6_groups)switchmap/poller/snmp/async_snmp_info.py (1)
473-475: Return consistent type from _add_layer3 on exceptionSame issue as _add_layer2.
except Exception as e: log.log2warning(1308, f" Error in _add_layer3: {e}") + return dataswitchmap/poller/snmp/mib/juniper/mib_junipervlan.py (2)
193-207: Prevent KeyError in jnxexvlantag when mappings are incompleteUse .get() and skip missing entries for robustness across device variations.
- for key in sorted(results.keys()): + for key in sorted(results): # The key is the full OID. Split this into its component nodes nodes = key.split(".") # Get the VLAN ID and corresponding VLAN tag vlan_id = nodes[-2] - vlan_tag = self.vlan_map[int(vlan_id)] + vlan_tag = self.vlan_map.get(int(vlan_id)) + if vlan_tag is None: + continue # Get dot1dbaseport value and it's corresponding ifindex - baseport_value = nodes[-1] - ifindex = self.baseportifindex[int(baseport_value)] + baseport_value = int(nodes[-1]) + ifindex = self.baseportifindex.get(baseport_value) + if ifindex is None: + continue
228-234: Prevent KeyError in jnxexvlanname when VLAN map is sparseGuard lookups with .get().
- vlan_tag = self.vlan_map[int(vlan_id)] + vlan_tag = self.vlan_map.get(int(vlan_id)) + if vlan_tag is None: + continueswitchmap/poller/snmp/mib/generic/mib_lldp.py (1)
315-321: Avoid KeyError when mapping baseport to ifIndex in lldpLocPortDescUse .get() and continue if mapping is missing.
- else: - ifindex = self._baseportifindex[key_index] + else: + ifindex = self._baseportifindex.get(key_index) + if ifindex is None: + continueswitchmap/poller/snmp/mib/generic/mib_bridge.py (1)
180-186: KeyError risks when indexing dictsBoth lookups can raise KeyError. Use membership/get checks to avoid crashes under partial SNMP data.
- if bool(dot1dtpfdbport[decimal_macaddress]) is False: + if decimal_macaddress not in dot1dtpfdbport: continue ... - ifindex = baseportifindex[dot1dbaseport] + ifindex = baseportifindex.get(dot1dbaseport) + if ifindex is None: + continue
🧹 Nitpick comments (39)
requirements.txt (2)
17-19: Pin pyasn1 for pysnmp 6.2.6 compatibility (per team learnings).Team standard is pysnmp==6.2.6; pyasn1 0.6.1+ breaks compat. When you re-introduce pysnmp via the approved process, also add:
+# pysnmp 6.2.6 requires older pyasn1 +pyasn1<0.6.1This avoids runtime import errors around pyasn1.compat.octets.
I can open a follow-up PR to add the correct pins once the sensitive-file label is applied. Do you want me to proceed?
17-19: Minor: fix comment typos and style.
- Add a space after '#'
- “availble” → “available”
-#Async SNMP polling +# Async SNMP polling ... -# pysnmp 7.x also availble but 6.2.6 is more stable as of now +# pysnmp 7.x also available but 6.2.6 is more stable as of nowswitchmap/poller/snmp/mib/generic/mib_etherlike.py (1)
91-118: Awaiting swalk is correct; consider minor hardeningCasting keys to int is fine with normalized=True. If any device returns unexpected keys, consider skipping non-int keys to avoid ValueError.
- for key, value in results.items(): - data_dict[int(key)] = value + for key, value in results.items(): + try: + data_dict[int(key)] = value + except (TypeError, ValueError): + # Skip unexpected keys + continuedebug_async_flow.py (4)
5-7: Clean up unused imports (Ruff F401)os and asyncio are unused.
-import sys -import os -import asyncio +import sys
23-27: Avoid hardcoding IP and ensure correct invocation for async functions
- Hardcoding a public IP is brittle. Read from CLI args or env (e.g., SWITCHMAP_DEBUG_HOST).
- If async_poll.run_cli_device is a coroutine, use asyncio.run; otherwise call directly.
# Example approach: import asyncio, inspect, os hostname = os.getenv("SWITCHMAP_DEBUG_HOST", "127.0.0.1") if inspect.iscoroutinefunction(async_poll.run_cli_device): asyncio.run(async_poll.run_cli_device(hostname=hostname)) else: async_poll.run_cli_device(hostname=hostname)
28-29: Typo in error messageFix “duing” → “during”.
- print(f"Error duing polling: {e}") + print(f"Error during polling: {e}")
1-34: Consider excluding this debug script from the packageThis looks like a local debugging aid. Keep it under scripts/, .gitignore it, or guard it under a dedicated dev-only entry point to avoid shipping it.
switchmap/poller/snmp/mib/generic/mib_ipv6.py (1)
92-138: IPv6 formatting logic is correct and consistentPairing bytes into 8 groups is the right approach. Minor typo in comment: “deximal” → “decimal”.
- # Convert deximal value to hex, + # Convert decimal value to hex,switchmap/poller/snmp/mib/cisco/mib_ciscovlaniftablerelationship.py (1)
115-121: Standardize types and avoid wrapping single VLAN in a listCurrent code stores a single VLAN as a list under ifIndex, which is inconsistent with other MIB methods that map keys to scalar values. Also, ensure ifIndex is consistently an int key.
Apply:
- results = await self.snmp_object.swalk(oid, normalized=False) - for oid, ifindex in results.items(): - nodes = oid.split(".") - vlan = int(nodes[-2]) - data_dict[ifindex] = [vlan] + results = await self.snmp_object.swalk(oid, normalized=False) + for oid, ifindex in results.items(): + nodes = oid.split(".") + vlan = int(nodes[-2]) + data_dict[int(ifindex)] = vlanIf multiple VLANs can map to a single ifIndex on some platforms, we should instead accumulate into a list via defaultdict(list). Otherwise, keep it scalar as above.
switchmap/poller/snmp/async_poller.py (5)
3-3: Remove unused importasyncio is not used in this module.
-import asyncio
13-27: Replace placeholder and inaccurate class docstringDocstring references non-existent functions (populate, post) and contains a placeholder. Replace with accurate description reflecting initialize_snmp and query.
-#!add better description class Poll: - """Switchmap-NG agent that gathers data. - - Args: - None - - Returns: - None - - Functions: - __init__: - populate: - post: - """ + """Asynchronous SNMP poller for switchmap-ng. + + This class initializes SNMP credentials asynchronously and queries devices + for topology data using async SNMP interactions. + + Args: + hostname (str): The hostname or IP address of the device to poll. + + Methods: + initialize_snmp() -> bool: Validates credentials and creates an async SNMP object. + query() -> dict | None: Asynchronously queries the device and returns collected data. + """
60-75: Tighten log message and wordingMinor grammar fix for the failure message.
- "credentials found in it.".format(self._hostname) + "credentials found for it.".format(self._hostname)
122-126: Simplify nested conditionalsCombine nested ifs into a single expression.
- if bool(authorization) is True: - if isinstance(authorization, SNMP) is True: - poll = bool(authorization.enabled) + if authorization and isinstance(authorization, SNMP): + poll = bool(authorization.enabled)
109-127: DRY: _do_poll duplicated with sync pollerThis helper mirrors switchmap/poller/snmp/poller.py. Consider moving to a shared utility to keep behavior consistent.
switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (1)
70-95: Parallelize independent SNMP calls in layer1vmVlan and vmPortStatus are independent; run them concurrently to reduce round-trips.
- # Get interface vmVlan data - values = await self.vmvlan() - for key, value in values.items(): - final[key]["vmVlan"] = value - - # Get interface vmPortStatus data - values = await self.vmportstatus() - for key, value in values.items(): - final[key]["vmPortStatus"] = value + # Fetch both concurrently + import asyncio + vmvlan_values, vmportstatus_values = await asyncio.gather( + self.vmvlan(), self.vmportstatus() + ) + for key, value in vmvlan_values.items(): + final[key]["vmVlan"] = value + for key, value in vmportstatus_values.items(): + final[key]["vmPortStatus"] = valueIf you prefer not to import inside the function, add
import asyncioat the top of the module.switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
91-120: Parallelize portIfIndex fetch and portDuplex walk_portifindex and the portDuplex swalk are independent; fetch both concurrently, then join results. Also guard against missing map entries.
- data_dict = defaultdict(dict) - dot1dbaseport = await self._portifindex() + import asyncio + data_dict = defaultdict(dict) # Process OID oid = ".1.3.6.1.4.1.9.5.1.4.1.1.10" # Return OID value. Used for unittests if oidonly is True: return oid - results = await self.snmp_object.swalk(oid, normalized=True) + # Fetch both concurrently + dot1dbaseport_task = asyncio.create_task(self._portifindex()) + duplex_walk_task = asyncio.create_task( + self.snmp_object.swalk(oid, normalized=True) + ) + dot1dbaseport, results = await asyncio.gather( + dot1dbaseport_task, duplex_walk_task + ) for key, value in results.items(): # Assign duplex value to ifindex key - ifindex = dot1dbaseport[int(key)] - data_dict[ifindex] = value + d1p = int(key) + if d1p in dot1dbaseport: + ifindex = dot1dbaseport[d1p] + data_dict[ifindex] = valueIf you prefer, move the import to the module top.
snmp_test.py (3)
25-25: Remove f-strings without placeholders (Ruff F541) and fix minor typosThese lines raise F541 and have minor wording issues. Remove the extraneous f and clean up messages.
- print(f"Getting SNMP configuration...") + print("Getting SNMP configuration...") @@ - print(f"Testing device connectivity...") + print("Testing device connectivity...") @@ - print(f"Device info:") + print("Device info:") @@ - print(f"Calling everything() method...") - print(f"wait a little...") + print("Calling everything() method...") + print("Please wait...") @@ - print(f"\nSUCCESS! everything() returned data :)))))))") - print(f"Data str:") + print("\nSUCCESS! everything() returned data") + print("Data summary:") @@ - print(f"everything() returned nonee result") + print("everything() returned None") @@ - print(f"ERROR: :(((((({e}") - print(f"Full traceback:") + print(f"ERROR: {e}") + print("Full traceback:") @@ - print(f"\nTest completed successfully!") + print("\nTest completed successfully!") @@ - print(f"\nTest failed!") + print("\nTest failed!")Also applies to: 49-49, 59-59, 65-66, 76-77, 93-93, 99-99, 112-112, 115-115
68-73: Prefer time.perf_counter() for timingHigher resolution and monotonic. Minor tweak.
- start_time = time.time() + start_time = time.perf_counter() everything_data = await query_obj.everything() - end_time = time.time() + end_time = time.perf_counter()
9-11: Avoid sys.path hacks in committed scriptsUsing sys.path.insert(0, '.') masks import issues. If this is only for local debugging, consider guarding it or removing before commit.
switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py (1)
83-91: Parallelize independent SNMP walks for better latencyc2900PortDuplexStatus and c2900PortLinkbeatStatus can be queried concurrently.
- # Get interface c2900PortDuplexStatus data - values = await self.c2900portduplexstatus() - for key, value in values.items(): - final[key]["c2900PortDuplexStatus"] = value - - # Get interface c2900PortLinkbeatStatus data - values = await self.c2900portlinkbeatstatus() - for key, value in values.items(): - final[key]["c2900PortLinkbeatStatus"] = value + # Query both OIDs concurrently + values_duplex, values_linkbeat = await asyncio.gather( + self.c2900portduplexstatus(), + self.c2900portlinkbeatstatus(), + ) + for key, value in values_duplex.items(): + final[key]["c2900PortDuplexStatus"] = value + for key, value in values_linkbeat.items(): + final[key]["c2900PortLinkbeatStatus"] = valueAdd missing import at top of file:
import asyncioswitchmap/poller/snmp/mib/generic/mib_ip.py (2)
98-106: Run both IP-MIB tables concurrentlyTwo independent tables can be walked in parallel to reduce latency.
- # Get interface ipNetToMediaTable data - values = await self.ipnettomediatable() - for key, value in values.items(): - final["ipNetToMediaTable"][key] = value - - # Get interface ipNetToPhysicalPhysAddress data - values = await self.ipnettophysicalphysaddress() - for key, value in values.items(): - final["ipNetToPhysicalPhysAddress"][key] = value + # Walk both tables concurrently + v4_task = self.ipnettomediatable() + v6_task = self.ipnettophysicalphysaddress() + v4_values, v6_values = await asyncio.gather(v4_task, v6_task) + for key, value in v4_values.items(): + final["ipNetToMediaTable"][key] = value + for key, value in v6_values.items(): + final["ipNetToPhysicalPhysAddress"][key] = valueAdd missing import at top if not present:
import asyncio
68-83: supported() always returning True: confirm intended behaviorIf any device lacks IP-MIB, this will waste time/raise errors downstream. Consider checking the existence of at least one target OID before returning True.
Do you want me to wire this to check existence of one of:
- .1.3.6.1.2.1.4.22.1.2 (IPv4 ARP)
- .1.3.6.1.2.1.4.35.1.4 (IPv6 ARP)
switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py (2)
84-98: Parallelize CDP table walksRun cdpCacheDeviceId, cdpCachePlatform, and cdpCacheDevicePort concurrently.
- # Get interface cdpCacheDeviceId data - values = await self.cdpcachedeviceid() - for key, value in values.items(): - final[key]["cdpCacheDeviceId"] = value - - # Get interface cdpCachePlatform data - values = await self.cdpcacheplatform() - for key, value in values.items(): - final[key]["cdpCachePlatform"] = value - - # Get interface cdpCacheDevicePort data - values = await self.cdpcachedeviceport() - if values is not None: - for key, value in values.items(): - final[key]["cdpCacheDevicePort"] = value + dev_id_task = self.cdpcachedeviceid() + platform_task = self.cdpcacheplatform() + port_task = self.cdpcachedeviceport() + dev_ids, platforms, ports = await asyncio.gather( + dev_id_task, platform_task, port_task + ) + + for key, value in dev_ids.items(): + final[key]["cdpCacheDeviceId"] = value + for key, value in platforms.items(): + final[key]["cdpCachePlatform"] = value + for key, value in ports.items(): + final[key]["cdpCacheDevicePort"] = valueAdd missing import at top if not present:
import asyncio
123-129: Make OctetString decoding robust to non-UTF8 payloadsCDP fields often contain bytes that aren’t valid UTF-8. Avoid exceptions with errors='replace'.
- data_dict[ifindex] = str(bytes(value), encoding="utf-8") + data_dict[ifindex] = bytes(value).decode("utf-8", errors="replace") @@ - data_dict[ifindex] = str(bytes(value), encoding="utf-8") + data_dict[ifindex] = bytes(value).decode("utf-8", errors="replace") @@ - data_dict[ifindex] = str(bytes(value), encoding="utf-8") + data_dict[ifindex] = bytes(value).decode("utf-8", errors="replace")Also applies to: 151-156, 181-187
switchmap/poller/snmp/async_snmp_info.py (5)
50-59: Run all sections concurrently in everything() to reduce wall timeThe 5 awaits are sequential. Use asyncio.gather to overlap IO while respecting the SNMP manager’s internal semaphore limits (per team guidance: 10).
- data["misc"] = await self.misc() - - data["system"] = await self.system() - - data["layer1"] = await self.layer1() - - data["layer2"] = await self.layer2() - - data["layer3"] = await self.layer3() + misc, system, layer1, layer2, layer3 = await asyncio.gather( + self.misc(), + self.system(), + self.layer1(), + self.layer2(), + self.layer3(), + ) + data["misc"] = misc + data["system"] = system + data["layer1"] = layer1 + data["layer2"] = layer2 + data["layer3"] = layer3
350-352: Prefer get_running_loop() in async contextsget_event_loop() is deprecated for this use. Use get_running_loop().
- loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop()
354-367: Tighten dict iteration (avoid .keys()) and handle non-dict branchesMinor readability/perf improvement; your logic otherwise looks fine.
- for primary in result.keys(): - if isinstance(result[primary], dict): - for secondary in result[primary].keys(): - if isinstance(result[primary][secondary], dict): - for tertiary, value in result[primary][ - secondary - ].items(): + for primary in result: + if isinstance(result[primary], dict): + for secondary in result[primary]: + if isinstance(result[primary][secondary], dict): + for tertiary, value in result[primary][secondary].items(): data[primary][secondary][tertiary] = value else: data[primary][secondary] = result[primary][secondary]
101-129: Consider short-circuit return on empty supported items for clarityOptional: If no supported MIBs, return None directly to skip processed flag.
310-329: _add_data: iterate dicts directly (avoid .keys())Minor readability.
- for primary in source.keys(): - for secondary, value in source[primary].items(): + for primary in source: + for secondary, value in source[primary].items(): target[primary][secondary] = valueswitchmap/poller/snmp/mib/generic/mib_qbridge.py (2)
75-87: Lazy-load bridge data: good; minor nitPattern looks correct. Small nit: consider defining self.bridge_mib in init (set to None) for readability, though current usage is safe.
181-184: Decode bytes more directlyUse bytes.decode for clarity.
- data_dict[key] = str(bytes(value), encoding="utf-8") + data_dict[key] = value.decode("utf-8")switchmap/poller/snmp/mib/generic/mib_entity.py (1)
143-154: Minor string decode nitOptional: replace str(bytes(value), encoding="utf-8").strip() with value.decode("utf-8", errors="ignore").strip() for brevity.
Also applies to: 167-178, 191-202, 215-226, 239-250, 263-274, 287-298, 311-322
switchmap/poller/snmp/async_snmp_manager.py (3)
766-779: Rename unused loop variableerror_indexSilence B007 and clarify intent.
- error_index, + _error_index,
1010-1012: Typo in log messageMinor typo: “valye” -> “value”.
- log_message = f"Failed to convert pysnmp integer valye: {value_type}, prettyPrint'{value_str}" + log_message = f"Failed to convert pysnmp integer value: {value_type}, prettyPrint '{value_str}'"
1051-1059: Use startswith for OID filterContainment check can pass on unrelated OIDs. Use startswith for precision.
- if filter_normalized not in oid_normalized: + if not oid_normalized.startswith(filter_normalized): continueswitchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (2)
99-105: Drop unused enumerate index
iis unused. Simplify the loop to avoid B007.- for i, (method_name, values) in enumerate(zip(method_names, results)): + for method_name, values in zip(method_names, results):
125-133: Remove unused exception variable
eisn’t used. Drop binding or log it.- except Exception as e: + except Exception: return name, {}switchmap/poller/snmp/mib/generic/mib_if.py (2)
102-115: Align per-device concurrency with global guidanceYou additionally throttle to 8 here, while the device-level Interact semaphore is 10. Double throttling can reduce throughput. Consider removing this local semaphore or setting it to 10 to match the tested limit.
- # Limit concurrent SNMP queries - semaphore = asyncio.Semaphore(8) + # Optional: rely on device-level semaphore (10). If keeping a local one, match 10. + semaphore = asyncio.Semaphore(10)
234-241: Redundant safe branchBoth branches call the same
swalk. Collapse for simplicity.- if safe is False: - results = await self.snmp_object.swalk(oid, normalized=True) - else: - results = await self.snmp_object.swalk(oid, normalized=True) + results = await self.snmp_object.swalk(oid, normalized=True)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (29)
debug_async_flow.py(1 hunks)requirements.txt(1 hunks)snmp_test.py(1 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/snmp/async_poller.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(1 hunks)switchmap/poller/snmp/async_snmp_manager.py(1 hunks)switchmap/poller/snmp/base_query.py(2 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py(4 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py(5 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscoietfip.py(3 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py(6 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscovlaniftablerelationship.py(3 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py(4 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py(13 hunks)switchmap/poller/snmp/mib/generic/mib_bridge.py(18 hunks)switchmap/poller/snmp/mib/generic/mib_entity.py(12 hunks)switchmap/poller/snmp/mib/generic/mib_essswitch.py(3 hunks)switchmap/poller/snmp/mib/generic/mib_etherlike.py(3 hunks)switchmap/poller/snmp/mib/generic/mib_if.py(22 hunks)switchmap/poller/snmp/mib/generic/mib_if_64.py(1 hunks)switchmap/poller/snmp/mib/generic/mib_ip.py(6 hunks)switchmap/poller/snmp/mib/generic/mib_ipv6.py(3 hunks)switchmap/poller/snmp/mib/generic/mib_lldp.py(15 hunks)switchmap/poller/snmp/mib/generic/mib_qbridge.py(5 hunks)switchmap/poller/snmp/mib/generic/mib_snmpv2.py(2 hunks)switchmap/poller/snmp/mib/juniper/mib_junipervlan.py(9 hunks)switchmap/poller/snmp/poller.py(1 hunks)test_all_devices.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: requirements.txt:17-19
Timestamp: 2025-08-09T22:52:35.889Z
Learning: In the switchmap-ng project, pysnmp 6.2.6 is deliberately chosen over 7.x due to: PEP 8 refactoring breaking method names (getCmd → get_cmd), removal of asyncore-based sync API, pyasn1 0.6.1+ dropping pyasn1.compat.octets, breaking changes in MIB instrumentation API, potential asyncio.hlapi import path changes, and proven production stability of 6.2.6. The team is considering future migration to gufo_snmp (a Go-based SNMP library) as an alternative.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/snmp/async_poller.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-09T22:52:35.889Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: requirements.txt:17-19
Timestamp: 2025-08-09T22:52:35.889Z
Learning: In the switchmap-ng project, pysnmp 6.2.6 is deliberately chosen over 7.x due to: PEP 8 refactoring breaking method names (getCmd → get_cmd), removal of asyncore-based sync API, pyasn1 0.6.1+ dropping pyasn1.compat.octets, breaking changes in MIB instrumentation API, potential asyncio.hlapi import path changes, and proven production stability of 6.2.6. The team is considering future migration to gufo_snmp (a Go-based SNMP library) as an alternative.
Applied to files:
switchmap/poller/snmp/mib/generic/mib_ipv6.pyrequirements.txt
🧬 Code Graph Analysis (10)
switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (10)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (1)
layer1(109-160)switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py (1)
layer1(70-94)switchmap/poller/snmp/async_snmp_info.py (1)
layer1(131-187)switchmap/poller/snmp/mib/generic/mib_if.py (1)
layer1(89-151)switchmap/poller/snmp/mib/generic/mib_lldp.py (1)
layer1(94-136)switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py (1)
layer1(70-100)switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
layer1(77-88)switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (1)
layer1(70-94)switchmap/poller/snmp/mib/generic/mib_essswitch.py (1)
layer1(70-89)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/mib/generic/mib_snmpv2.py (3)
switchmap/poller/snmp/async_snmp_info.py (1)
system(78-129)switchmap/poller/snmp/mib/generic/mib_entity.py (1)
system(78-129)switchmap/poller/snmp/snmp_manager.py (1)
get(410-447)
switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (5)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (1)
layer1(109-160)switchmap/poller/snmp/async_snmp_info.py (1)
layer1(131-187)switchmap/poller/snmp/mib/generic/mib_if.py (1)
layer1(89-151)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
layer1(70-89)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py (7)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (1)
layer1(109-160)switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py (1)
layer1(70-94)switchmap/poller/snmp/async_snmp_info.py (1)
layer1(131-187)switchmap/poller/snmp/mib/generic/mib_if.py (2)
layer1(89-151)ifindex(451-478)switchmap/poller/snmp/mib/generic/mib_lldp.py (1)
layer1(94-136)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
layer1(70-89)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/mib/generic/mib_ip.py (3)
switchmap/poller/snmp/base_query.py (1)
supported(47-65)switchmap/poller/snmp/async_snmp_info.py (1)
layer3(248-308)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py (8)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (1)
layer1(109-160)switchmap/poller/snmp/mib/generic/mib_if.py (1)
layer1(89-151)switchmap/poller/snmp/mib/generic/mib_etherlike.py (1)
layer1(70-89)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
layer1(70-89)switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
layer1(77-88)switchmap/poller/snmp/mib/generic/mib_essswitch.py (1)
layer1(70-89)switchmap/poller/snmp/async_snmp_manager.py (1)
swalk(458-484)tests/switchmap_/poller/snmp/mib/cisco/test_mib_ciscoc2900.py (1)
swalk(97-106)
switchmap/poller/snmp/mib/generic/mib_entity.py (7)
switchmap/poller/snmp/async_snmp_info.py (1)
system(78-129)switchmap/poller/snmp/mib/generic/mib_if.py (1)
system(72-87)switchmap/poller/snmp/mib/generic/mib_snmpv2.py (1)
system(79-117)switchmap/poller/snmp/mib/generic/mib_if_64.py (1)
system(73-87)switchmap/poller/snmp/async_snmp_manager.py (1)
swalk(458-484)tests/switchmap_/poller/snmp/mib/cisco/test_mib_ciscoc2900.py (1)
swalk(97-106)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/async_snmp_info.py (24)
switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)switchmap/poller/snmp/base_query.py (2)
Query(4-65)supported(47-65)switchmap/poller/snmp/mib/generic/mib_if.py (2)
system(72-87)layer1(89-151)switchmap/poller/snmp/mib/generic/mib_entity.py (1)
system(78-129)switchmap/poller/snmp/mib/generic/mib_snmpv2.py (1)
system(79-117)switchmap/poller/snmp/mib/generic/mib_if_64.py (2)
system(73-87)layer1(89-130)switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (2)
layer1(109-160)layer2(75-107)switchmap/poller/snmp/mib/generic/mib_qbridge.py (2)
layer1(88-109)layer2(111-130)switchmap/poller/snmp/mib/juniper/mib_junipervlan.py (2)
layer1(100-128)layer2(130-151)switchmap/poller/snmp/mib/cisco/mib_ciscoc2900.py (1)
layer1(70-94)switchmap/poller/snmp/mib/generic/mib_lldp.py (1)
layer1(94-136)switchmap/poller/snmp/mib/generic/mib_etherlike.py (1)
layer1(70-89)switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py (1)
layer1(70-100)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
layer1(70-89)switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
layer1(77-88)switchmap/poller/snmp/mib/cisco/mib_ciscovlaniftablerelationship.py (1)
layer1(70-89)switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (1)
layer1(70-94)switchmap/poller/snmp/mib/generic/mib_essswitch.py (1)
layer1(70-89)switchmap/poller/snmp/mib/cisco/mib_ciscoietfip.py (1)
layer3(71-90)switchmap/poller/snmp/mib/generic/mib_ip.py (2)
layer3(84-108)supported(68-82)switchmap/poller/snmp/mib/generic/mib_ipv6.py (1)
layer3(71-90)switchmap/poller/snmp/async_snmp_manager.py (3)
hostname(185-194)sysobjectid(231-259)query(486-591)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)switchmap/core/log.py (3)
log2exception(273-307)log2debug(198-210)log2warning(183-195)
switchmap/poller/snmp/mib/generic/mib_bridge.py (10)
switchmap/poller/snmp/mib/generic/mib_lldp.py (2)
_ifindex(367-393)layer1(94-136)switchmap/poller/snmp/mib/cisco/mib_ciscocdp.py (2)
_ifindex(190-205)layer1(70-100)switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (3)
layer1(109-160)vtpvlantype(306-332)vtpvlanstate(334-360)switchmap/poller/snmp/mib/generic/mib_qbridge.py (1)
layer1(88-109)switchmap/poller/snmp/mib/generic/mib_if.py (2)
layer1(89-151)ifindex(451-478)switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (1)
layer1(70-89)switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (1)
layer1(70-94)switchmap/poller/snmp/mib/generic/mib_essswitch.py (1)
layer1(70-89)switchmap/poller/snmp/mib/generic/mib_if_64.py (1)
layer1(89-130)switchmap/poller/snmp/async_snmp_manager.py (2)
oid_exists(261-298)swalk(458-484)
switchmap/poller/snmp/async_snmp_manager.py (5)
switchmap/poller/configuration.py (1)
ConfigPoller(8-175)switchmap/core/files.py (1)
snmp_file(378-392)switchmap/core/log.py (3)
log2die(243-254)log2warning(183-195)log2info(213-225)switchmap/poller/snmp/async_snmp_info.py (1)
Query(12-308)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)
🪛 Ruff (0.12.2)
debug_async_flow.py
6-6: os imported but unused
Remove unused import: os
(F401)
7-7: asyncio imported but unused
Remove unused import: asyncio
(F401)
switchmap/poller/snmp/async_poller.py
3-3: asyncio imported but unused
Remove unused import: asyncio
(F401)
122-123: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
snmp_test.py
25-25: f-string without any placeholders
Remove extraneous f prefix
(F541)
49-49: f-string without any placeholders
Remove extraneous f prefix
(F541)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
65-65: f-string without any placeholders
Remove extraneous f prefix
(F541)
66-66: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
77-77: f-string without any placeholders
Remove extraneous f prefix
(F541)
93-93: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
112-112: f-string without any placeholders
Remove extraneous f prefix
(F541)
115-115: f-string without any placeholders
Remove extraneous f prefix
(F541)
switchmap/poller/snmp/async_snmp_info.py
231-231: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
323-323: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
354-354: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
356-356: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
switchmap/poller/snmp/async_snmp_manager.py
134-135: Use a single if statement instead of nested if statements
(SIM102)
222-222: Do not use bare except
(E722)
284-290: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
769-769: Loop control variable error_index not used within loop body
Rename unused error_index to _error_index
(B007)
1063-1067: Use ternary operator key = oid_str.split(".")[-1] if normalized is True else oid_str instead of if-else-block
(SIM108)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py
99-99: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
131-131: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🪛 GitHub Actions: PR Workflow
snmp_test.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black --write' to fix code style issues.
requirements.txt
[error] 1-1: File 'requirements.txt' is unauthorized to change/delete. To override this, apply the 'ignore-sensitive-files-pr' label.
🔇 Additional comments (18)
requirements.txt (1)
17-20: CI Failure: Protected File Change Requires ActionThe PR is blocked by the requirements.txt guard in
.github/workflows/pull-request.yml. You must either revert the dependency bump or have a maintainer apply the ignore-sensitive-files-pr label to proceed.• File:
requirements.txt(Lines 17–20)
- Revert these additions:
- #Async SNMP polling - pysnmp==6.2.6 - # pysnmp 7.x also availble but 6.2.6 is more stable as of now• Or ask a repo maintainer to add the ignore-sensitive-files-pr label to this PR and rerun CI.
⛔ Skipped due to learnings
Learnt from: abbi4code PR: PalisadoesFoundation/switchmap-ng#328 File: requirements.txt:17-19 Timestamp: 2025-08-09T22:52:35.889Z Learning: In the switchmap-ng project, pysnmp 6.2.6 is deliberately chosen over 7.x due to: PEP 8 refactoring breaking method names (getCmd → get_cmd), removal of asyncore-based sync API, pyasn1 0.6.1+ dropping pyasn1.compat.octets, breaking changes in MIB instrumentation API, potential asyncio.hlapi import path changes, and proven production stability of 6.2.6. The team is considering future migration to gufo_snmp (a Go-based SNMP library) as an alternative.Learnt from: abbi4code PR: PalisadoesFoundation/switchmap-ng#328 File: switchmap/poller/snmp/async_snmp_manager.py:160-160 Timestamp: 2025-08-09T22:25:34.114Z Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.switchmap/poller/snmp/mib/generic/mib_essswitch.py (1)
70-118: Async conversion looks correct and consistent.
layer1()is async and awaitsswportduplexstatus().swportduplexstatus()awaitssnmp_object.swalk(...).LGTM.
switchmap/poller/snmp/mib/generic/mib_etherlike.py (1)
70-89: Async conversion of layer1 looks goodThe coroutine conversion and await usage are correct, with no behavioral changes. Ensure all call sites await this method.
switchmap/poller/snmp/mib/generic/mib_snmpv2.py (1)
79-117: Refactor SNMPv2 system query to run concurrently and harden decodingThe
getmethod onself.snmp_objectis indeed declared asasync def getin async_snmp_manager.py, so you can safely switch from sequential awaits toasyncio.gather. Also corrected TODO typos and added defensive decoding.Places to update in switchmap/poller/snmp/mib/generic/mib_snmpv2.py:
@@ async def system(self): - # Process - #! after checking every sys n layers working in async (work on major improvements) - #! use gather intead of going seqeuncial - oidroot = ".1.3.6.1.2.1.1" - for node in range(1, 7): - oid = "{}.{}.0".format(oidroot, node) - results = await self.snmp_object.get(oid, normalized=True) - for value in results.values(): - getvalues.append(value) + # Process sys.* OIDs concurrently + import asyncio + oids = [ + ".1.3.6.1.2.1.1.1.0", # sysDescr + ".1.3.6.1.2.1.1.2.0", # sysObjectID + ".1.3.6.1.2.1.1.3.0", # sysUpTime + ".1.3.6.1.2.1.1.4.0", # sysContact + ".1.3.6.1.2.1.1.5.0", # sysName + ".1.3.6.1.2.1.1.6.0", # sysLocation + ] + results_list = await asyncio.gather( + *(self.snmp_object.get(oid, normalized=True) for oid in oids) + ) + getvalues = [0] + [next(iter(r.values()), None) for r in results_list] @@ async def system(self): - data_dict["sysDescr"][key] = general.cleanstring( - getvalues[1].decode("utf-8") - ) - data_dict["sysObjectID"][key] = getvalues[2].decode("utf-8") - data_dict["sysUpTime"][key] = int(getvalues[3]) - data_dict["sysContact"][key] = getvalues[4].decode("utf-8") - data_dict["sysName"][key] = getvalues[5].decode("utf-8") - data_dict["sysLocation"][key] = getvalues[6].decode("utf-8") + def _dec(v): + if isinstance(v, (bytes, bytearray)): + try: + return v.decode("utf-8", errors="replace") + except Exception: + return str(v) + return str(v) + + data_dict["sysDescr"][key] = general.cleanstring(_dec(getvalues[1])) + data_dict["sysObjectID"][key] = _dec(getvalues[2]) + data_dict["sysUpTime"][key] = int(getvalues[3] or 0) + data_dict["sysContact"][key] = _dec(getvalues[4]) + data_dict["sysName"][key] = _dec(getvalues[5]) + data_dict["sysLocation"][key] = _dec(getvalues[6])With this change, all six
sys.*GETs fire in parallel, and byte decoding is now robust.switchmap/poller/snmp/mib/generic/mib_ipv6.py (1)
71-90: Async conversion of layer3 looks goodAwait usage and return shape are consistent with other async MIBs.
switchmap/poller/snmp/mib/cisco/mib_ciscovlaniftablerelationship.py (1)
70-89: Async conversion looks correctMethod correctly migrated to async; awaiting cviroutedvlanifindex and building final matches the project pattern.
switchmap/poller/snmp/mib/cisco/mib_ciscovlanmembership.py (1)
117-123: LGTM on async SNMP walksAwaiting swalk with normalized=True and int keys keeps types consistent across modules.
Also applies to: 145-151
switchmap/poller/snmp/mib/cisco/mib_ciscostack.py (2)
70-89: Async conversion looks correctlayer1 migration to async and awaiting portduplex aligns with the new async pattern.
141-146: LGTM on async swalk and typingAwaiting swalk, normalizing keys, and converting to int keeps key typing consistent.
switchmap/poller/async_poll.py (2)
206-208: log2see exists; no replacement neededThe
log2seemethod is defined inswitchmap/core/log.pyand is used elsewhere in the codebase (e.g.,switchmap/poller/poll.pyand in tests), so keepinglog.log2see(1413, log_message)is correct. No changes required.
20-55: SNMP request concurrency is correctly limited to 10
Verified in switchmap/poller/snmp/async_snmp_manager.py (line 157) thatself._semaphore = asyncio.Semaphore(10)matches the team guideline. No changes needed.switchmap/poller/snmp/mib/generic/mib_qbridge.py (1)
88-110: Q-Bridge async refactor looks soundLayer1 and Layer2 methods await SNMP walks and map indices correctly; supported gating via _get_bridge_data avoids unnecessary calls. No functional issues spotted.
Also applies to: 111-131, 132-160
switchmap/poller/snmp/mib/juniper/mib_junipervlan.py (1)
100-152: Async conversion and dependency loading look correctGood use of deferred bridge and VLAN map initialization with awaits in layer1/layer2.
switchmap/poller/snmp/mib/generic/mib_entity.py (1)
78-129: Concurrent fetch in system() is well-structuredUsing asyncio.gather to parallelize entPhysical* is appropriate. Return shape matches prior expectations.
switchmap/poller/snmp/mib/generic/mib_lldp.py (1)
79-93: Bridge data lazy-load is solidEnsures dependency readiness. Good gating on MIB support.
Also applies to: 94-118
switchmap/poller/snmp/async_snmp_manager.py (1)
156-158: Concurrency limit matches team conventionDevice-level semaphore is set to 10, which aligns with the project’s tested limit for stability/performance.
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (1)
388-399: Confirm value type before hexlify in vlanTrunkPortVlansEnabled
valueshould be bytes; if not,binascii.hexlifywill fail. Defensive cast avoids surprises.- vlans_hex = binascii.hexlify(value).decode("utf-8") + buf = value if isinstance(value, (bytes, bytearray)) else bytes(value) + vlans_hex = binascii.hexlify(buf).decode("utf-8")Please verify on hardware where this OID is present whether
valueis bytes already.switchmap/poller/snmp/mib/generic/mib_if.py (1)
268-274: Ensure bytes-to-str conversion is robust
valueshould be bytes (per async manager). If it can be None or already str, guard conversion to avoid exceptions.- data_dict[int(key)] = str(bytes(value), encoding="utf-8") + if isinstance(value, (bytes, bytearray)): + data_dict[int(key)] = value.decode("utf-8", errors="ignore") + else: + data_dict[int(key)] = "" if value is None else str(value)Please confirm observed types during polling to set the minimal necessary handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
bin/systemd/switchmap_poller (1)
88-96: Ensure lockfile removal even on exceptions during pollingUse try/finally around the polling call so the lockfile is always removed; otherwise a crash leaves a stale lock and blocks subsequent runs.
Apply this diff:
- # Create lockfile - open(self.lockfile, "a").close() - - # Poll after sleeping - async_poll.run_devices(max_concurrent_devices=max_concurrent) - - # Delete lockfile - os.remove(self.lockfile) + # Create lockfile + open(self.lockfile, "a").close() + try: + # Poll devices + async_poll.run_devices(max_concurrent_devices=max_concurrent) + finally: + # Ensure lockfile is removed even on failure + try: + os.remove(self.lockfile) + except FileNotFoundError: + pass
♻️ Duplicate comments (5)
switchmap/poller/snmp/mib/generic/mib_bridge.py (2)
167-174: Add failure isolation around asyncio.gather for Cisco MAC pathA single failing task will currently abort the entire Cisco MAC table fetch. Degrade gracefully by allowing other tasks to proceed.
Apply this diff:
- macs, dot1dtpfdbport, baseportifindex = await asyncio.gather(*tasks) + results = await asyncio.gather(*tasks, return_exceptions=True) + if any(isinstance(r, Exception) for r in results): + # Optionally: log a warning here if you later add log in this module + return final + macs, dot1dtpfdbport, baseportifindex = results
371-389: Avoid offset heuristic; map dot1dBasePort→ifIndex directly from tableThe offset approach can be wrong on devices where mapping isn’t linear. Use the dot1dBasePortIfIndex table directly.
Apply this diff:
- # Get ifindex data directly - oid = ".1.3.6.1.2.1.2.2.1.1" - ifindex_results = await self.snmp_object.swalk(oid, normalized=True) - ifindex_data = {int(k): v for k, v in ifindex_results.items()} - # Get the difference between ifIndex and dot1dBasePortIfIndex - oid = ".1.3.6.1.2.1.17.1.4.1.2" - results = await self.snmp_object.swalk(oid, normalized=True) - for _bridge_index, ifindex in results.items(): - bridge_index = int(_bridge_index) - offset = int(ifindex) - bridge_index - break - - # Populate the dictionary keyed by dot1dBasePortIfIndex - for ifindex, _ in sorted(ifindex_data.items()): - bridge_index = ifindex - offset - data_dict[bridge_index] = ifindex + # Populate mapping directly from dot1dBasePortIfIndex table + oid = ".1.3.6.1.2.1.17.1.4.1.2" + results = await self.snmp_object.swalk(oid, normalized=True) + for bridge_port, ifindex in results.items(): + data_dict[int(bridge_port)] = int(ifindex)snmp_test.py (1)
15-21: Parameterize target host instead of hardcoding an IP (avoid leaking real targets, improve reusability)Pass the hostname via CLI args and thread it through
test_everything(). This also addresses PII concerns noted in prior reviews.Apply:
@@ -import asyncio +import asyncio +import argparse @@ -async def test_everything(): +async def test_everything(hostname: str): @@ - hostname = "162.249.37.218" + # hostname is provided by caller/CLI @@ async def main(): """Main test function.""" print("Async SNMP Info Test (Proper Credentials)") print("=" * 50) - result = await test_everything() + parser = argparse.ArgumentParser(description="Test async_snmp_info.everything()") + parser.add_argument("-H", "--host", required=True, help="Target hostname/IP") + args = parser.parse_args() + result = await test_everything(args.host) @@ if __name__ == "__main__": print(" Running Async SNMP Test with Proper Credentials...") success = asyncio.run(main())Also applies to: 100-107, 115-124, 4-8
switchmap/poller/snmp/async_poller.py (1)
85-87: Do not terminate the entire poller when a single host lacks an SNMP object
log.log2diewill exit the process; in an async multi-host run this is too harsh. Warn and return None so orchestration can continue.- if bool(self.snmp_object) is False: - log.log2die(1001, f"No valid SNMP object for {self._hostname} ") - return _data + if not self.snmp_object: + log.log2warning(1001, f"No valid SNMP object for {self._hostname}") + return Noneswitchmap/poller/snmp/mib/generic/mib_lldp.py (1)
120-135: LGTM: fixed the broken zip/enumerate while merging LLDP resultsIterating directly over
zip(method_names, results)restores correct pairing and merging.
🧹 Nitpick comments (22)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (2)
301-307: Harden vtpVlanName decoding for robustness across pysnmp typesPrefer .asOctets() when available, and fall back safely. Keeps behavior stable if value is a pysnmp OctetString, bytes, or otherwise.
Apply this diff:
- results = await self.snmp_object.swalk(oid, normalized=True) - for key, value in results.items(): - data_dict[int(key)] = str(bytes(value), encoding="utf-8") + results = await self.snmp_object.swalk(oid, normalized=True) + for key, value in results.items(): + try: + raw = value.asOctets() + except AttributeError: + raw = ( + value + if isinstance(value, (bytes, bytearray)) + else bytes(value) + ) + data_dict[int(key)] = raw.decode("utf-8", errors="replace")
384-419: Make trunk VLAN parsing resilient and type-safe
- Use defaultdict(list) for list accumulation.
- Guard trunkstatus lookups and compare on int to avoid type mismatches.
- Simplify list appends.
Note: VLAN-bit offset/order can vary by vendor/MIB semantics; see next comment to validate mapping.
Apply this diff:
- data_dict = defaultdict(dict) + data_dict = defaultdict(list) @@ - trunkstatus = await self.vlantrunkportdynamicstatus() + trunkstatus = await self.vlantrunkportdynamicstatus() @@ - for key, value in results.items(): + for key, value in results.items(): # Get the ifindex value ifindex = int(key) @@ # Convert hex value to right justified 1024 character binary string vlans_hex = binascii.hexlify(value).decode("utf-8") binary_string = bin(int(vlans_hex, base))[2:].zfill(length_in_bits) # Assign flag vlans on interface - if trunkstatus[ifindex] == 1: - for svlan, state in enumerate(binary_string): - vlan = int(svlan) - if int(state) == 1: - if ifindex in data_dict: - data_dict[ifindex].append(vlan) - else: - data_dict[ifindex] = [vlan] + trunk_state = int(trunkstatus.get(ifindex, 0)) + if trunk_state == 1: + for svlan, bit in enumerate(binary_string): + if bit == "1": + data_dict[ifindex].append(svlan)switchmap/poller/snmp/mib/generic/mib_qbridge.py (1)
180-183: Make dot1qVlanStaticName decoding robust across typesUse .asOctets() when present and a safe fallback for non-OctetString values for consistency with pysnmp return types.
Apply this diff:
- for key, value in results.items(): - data_dict[key] = str(bytes(value), encoding="utf-8") + for key, value in results.items(): + try: + raw = value.asOctets() + except AttributeError: + raw = ( + value + if isinstance(value, (bytes, bytearray)) + else bytes(value) + ) + data_dict[key] = raw.decode("utf-8", errors="replace")switchmap/poller/snmp/mib/juniper/mib_junipervlan.py (3)
79-86: Initialize vlan_map when unsupported to avoid None usage downstreamIf the MIB isn’t supported, vlan_map stays None and later lookups could break if methods are invoked out of sequence. Default to {}.
Apply this diff:
- if await self.supported() is True: - self.vlan_map = await self._vlanid2tag() + if await self.supported() is True: + self.vlan_map = await self._vlanid2tag() + else: + self.vlan_map = {}
191-207: Guard against missing vlan_map/baseportifindex entries to prevent KeyErrorsUse .get() and skip missing mappings. This keeps the method resilient to partial data or transient SNMP gaps.
Apply this diff:
- for key in sorted(results.keys()): + for key in sorted(results.keys()): # The key is the full OID. Split this into its component nodes nodes = key.split(".") @@ - vlan_tag = self.vlan_map[int(vlan_id)] + vlan_tag = self.vlan_map.get(int(vlan_id)) @@ - ifindex = self.baseportifindex[int(baseport_value)] + ifindex = self.baseportifindex.get(int(baseport_value)) + if vlan_tag is None or ifindex is None: + continue
226-233: Harden jnxExVlanName decoding for pysnmp OctetString vs bytesSame rationale as elsewhere: prefer .asOctets() and a safe fallback for consistent behavior.
Apply this diff:
- results = await self.snmp_object.swalk(oid, normalized=True) - for vlan_id, value in results.items(): + results = await self.snmp_object.swalk(oid, normalized=True) + for vlan_id, value in results.items(): # Get VLAN tag vlan_tag = self.vlan_map[int(vlan_id)] # Assign value (Convert to string) - data_dict[vlan_tag] = str(bytes(value), encoding="utf-8") + try: + raw = value.asOctets() + except AttributeError: + raw = ( + value + if isinstance(value, (bytes, bytearray)) + else bytes(value) + ) + data_dict[vlan_tag] = raw.decode("utf-8", errors="replace")snmp_test.py (1)
23-23: Remove f-strings without placeholders and fix minor typosThese trigger Ruff F541 and add noise. Also fix "nonee" typo.
- print(f"Getting SNMP configuration...") + print("Getting SNMP configuration...") - print(f"Testing device connectivity...") + print("Testing device connectivity...") - print(f"Device info:") + print("Device info:") - print(f"Calling everything() method...") + print("Calling everything() method...") - print(f"wait a little...") + print("wait a little...") - print(f"\nSUCCESS! everything() returned data :)))))))") - print(f"Data str:") + print("\nSUCCESS! everything() returned data") + print("Data structure:") - print(f"everything() returned nonee result") + print("everything() returned None") - print(f"Full traceback:") + print("Full traceback:") - print(f"\nTest completed successfully!") + print("\nTest completed successfully!") - print(f"\nTest failed!") + print("\nTest failed!")Also applies to: 43-43, 53-53, 59-60, 70-71, 89-89, 95-95, 108-108, 111-111
switchmap/poller/snmp/async_poller.py (1)
116-118: Simplify nested conditions in _do_poll for readabilityEquivalent logic, shorter and clearer.
- if bool(authorization) is True: - if isinstance(authorization, SNMP) is True: - poll = bool(authorization.enabled) + if authorization and isinstance(authorization, SNMP): + poll = bool(authorization.enabled)switchmap/poller/snmp/async_snmp_info.py (2)
74-77: Guard against missing sysObjectID when resolving enterprise numberAvoids exceptions if
sysobjectid()returns None.- sysobjectid = await self.snmp_object.sysobjectid() - vendor = iana_enterprise.Query(sysobjectid=sysobjectid) - data["IANAEnterpriseNumber"] = vendor.enterprise() + sysobjectid = await self.snmp_object.sysobjectid() + if sysobjectid: + vendor = iana_enterprise.Query(sysobjectid=sysobjectid) + data["IANAEnterpriseNumber"] = vendor.enterprise() + else: + data["IANAEnterpriseNumber"] = None
326-328: Minor: iterate dict directly instead of .keys()Slightly cleaner and faster.
- for primary in source.keys(): + for primary in source: for secondary, value in source[primary].items(): target[primary][secondary] = valueswitchmap/poller/snmp/async_snmp_manager.py (3)
127-135: Flatten nested condition in Validate.validation for clarityKeep the same behavior while reducing nesting.
- if group is None: - if await device.contactable() is True: - result = authorization - break - else: - if authorization.group == group: - if await device.contactable() is True: - result = authorization + if group is None: + if await device.contactable(): + result = authorization + break + elif authorization.group == group and await device.contactable(): + result = authorization
350-353: Nit: inline boolean return in _oid_exists_walkLogic is correct (thanks for fixing the earlier inversion). This form is a touch simpler.
- if exists and isinstance(results, dict) and results: - return True - return False + return bool(exists and isinstance(results, dict) and results)
1102-1106: Nit: prefer a ternary for key selection in _format_resultsSlightly tighter.
- if normalized is True: - # use only the last node of the OID - key = oid_str.split(".")[-1] - else: - key = oid_str + key = oid_str.split(".")[-1] if normalized else oid_strswitchmap/poller/snmp/mib/generic/mib_lldp.py (1)
168-168: Prefer bytes.decode() over str(bytes, encoding) for clarityEquivalent behavior with clearer intent; add errors='ignore' to be defensive with malformed strings coming from devices.
- data_dict[ifindex] = str(bytes(value), encoding="utf-8") + data_dict[ifindex] = value.decode("utf-8", errors="ignore") @@ - data_dict[ifindex] = general.cleanstring( - str(bytes(value), encoding="utf-8") - ) + data_dict[ifindex] = general.cleanstring( + value.decode("utf-8", errors="ignore") + ) @@ - data_dict[ifindex] = general.cleanstring( - str(bytes(value), encoding="utf-8") - ) + data_dict[ifindex] = general.cleanstring( + value.decode("utf-8", errors="ignore") + ) @@ - data_dict[ifindex] = general.cleanstring( - str(bytes(value), encoding="utf-8") - ) + data_dict[ifindex] = general.cleanstring( + value.decode("utf-8", errors="ignore") + )Also applies to: 246-247, 282-283, 327-328
switchmap/poller/snmp/mib/generic/mib_if.py (8)
134-149: Remove redundant return_exceptions and dead branch
limited_queryalready catches and logs exceptions and returns(name, {}). Keepingreturn_exceptions=Trueand checkingisinstance(result, Exception)is redundant and never triggers.Apply this diff to simplify:
- # Execute all queries concurrently with rate limit - results = await asyncio.gather( - *[limited_query(method, name) for method, name in queries], - return_exceptions=True, - ) + # Execute all queries concurrently with rate limit + results = await asyncio.gather( + *[limited_query(method, name) for method, name in queries] + ) @@ - for result in results: - if isinstance(result, Exception): - continue - - method_name, values = result + for method_name, values in results: for key, value in values.items(): final[key][method_name] = value
233-238: Redundant safe-branch in ifOutOctetsBoth branches call the same awaited method, so the
safeconditional is a no-op. This will reduce branching noise.Apply this diff:
- # Process results - if safe is False: - results = await self.snmp_object.swalk(oid, normalized=True) - else: - results = await self.snmp_object.swalk(oid, normalized=True) + # Process results + results = await self.snmp_object.swalk(oid, normalized=True)
181-191: Parameter-doc inconsistency (safe is unused)
ifinoctetsadvertises asafeparameter but does not use it. Consider either removing the parameter or updating the docstring to avoid confusion. Similar cleanup could apply across methods wheresafeis no longer meaningful in the async manager.
267-273: Robustly handle OctetString/bytes vs str for ifDescr
str(bytes(value), encoding="utf-8")will raise for an already-strvalue. Prefer decoding whenvalueis bytes-like, elsestr(value). This aligns with our prior finding that pysnmp returns bytes for OctetStrings, but keeps us safe if a different manager returnsstr.Apply this diff:
- for key, value in results.items(): - # Process OID - data_dict[int(key)] = str(bytes(value), encoding="utf-8") + for key, value in results.items(): + # Process OID + if isinstance(value, (bytes, bytearray)): + data_dict[int(key)] = value.decode("utf-8", errors="ignore") + else: + data_dict[int(key)] = str(value)
413-417: Same decoding concern for ifAliasMirror the safer decoding approach used for ifDescr to prevent TypeErrors and tolerate undecodable bytes.
Apply this diff:
- for key, value in results.items(): - # Process OID - data_dict[int(key)] = str(bytes(value), encoding="utf-8") + for key, value in results.items(): + # Process OID + if isinstance(value, (bytes, bytearray)): + data_dict[int(key)] = value.decode("utf-8", errors="ignore") + else: + data_dict[int(key)] = str(value)
442-446: Same decoding concern for ifNameApply the same bytes-safe decoding pattern to ifName.
Apply this diff:
- for key, value in results.items(): - # Process OID - data_dict[int(key)] = str(bytes(value), encoding="utf-8") + for key, value in results.items(): + # Process OID + if isinstance(value, (bytes, bytearray)): + data_dict[int(key)] = value.decode("utf-8", errors="ignore") + else: + data_dict[int(key)] = str(value)
499-505: ifPhysAddress: avoid UTF-8 decoding traps; render MAC deterministicallyConverting MAC bytes via
general.octetstr_2_string(value)risks decode issues and non-canonical output. Render MACs directly from bytes to a lowercase hex string (colon-separated), which is robust for arbitrary octets and consistent across managers.Apply this diff:
- for key, value in results.items(): - # Process OID to get MAC address - data_dict[int(key)] = general.octetstr_2_string(value) + for key, value in results.items(): + # Process OID to get MAC address + b = value if isinstance(value, (bytes, bytearray)) else bytes(value) + data_dict[int(key)] = ":".join(f"{_b:02x}" for _b in b)Note: If you must retain
general.octetstr_2_stringfor compatibility, consider updating it to accept bytes and perform the same bytes-to-hex logic internally.
682-709: Use a more appropriate container for ifStackStatus
finalis used as a mapping oflower_ifindex -> list[higher_ifindex]. Initialize it asdefaultdict(list), and minor iteration cleanup.Apply this diff:
- final = defaultdict(lambda: defaultdict(dict)) + final = defaultdict(list) @@ - results = await self.snmp_object.swalk(oid, normalized=False) - for key in results.keys(): + results = await self.snmp_object.swalk(oid, normalized=False) + for key in results: @@ - if ifstacklowerlayer in final: - final[ifstacklowerlayer].append(ifstackhigherlayer) - else: - final[ifstacklowerlayer] = [ifstackhigherlayer] + final[ifstacklowerlayer].append(ifstackhigherlayer)Optional: Update the return doc to explicitly state that values are lists of higher-layer ifIndexes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
bin/systemd/switchmap_poller(3 hunks)snmp_test.py(1 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/snmp/async_poller.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(1 hunks)switchmap/poller/snmp/async_snmp_manager.py(1 hunks)switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py(13 hunks)switchmap/poller/snmp/mib/generic/mib_bridge.py(18 hunks)switchmap/poller/snmp/mib/generic/mib_if.py(22 hunks)switchmap/poller/snmp/mib/generic/mib_ip.py(6 hunks)switchmap/poller/snmp/mib/generic/mib_ipv6.py(3 hunks)switchmap/poller/snmp/mib/generic/mib_lldp.py(15 hunks)switchmap/poller/snmp/mib/generic/mib_qbridge.py(5 hunks)switchmap/poller/snmp/mib/generic/mib_snmpv2.py(2 hunks)switchmap/poller/snmp/mib/juniper/mib_junipervlan.py(9 hunks)switchmap/poller/snmp/poller.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- switchmap/poller/snmp/mib/generic/mib_ip.py
- switchmap/poller/snmp/mib/generic/mib_snmpv2.py
- switchmap/poller/snmp/poller.py
- switchmap/poller/async_poll.py
- switchmap/poller/snmp/mib/generic/mib_ipv6.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:270-273
Timestamp: 2025-08-10T14:53:39.577Z
Learning: In the switchmap-ng SNMP poller migration from easysnmp to pysnmp, the pysnmp library returns bytes for OctetString values in SNMP responses for OIDs like ifDescr, ifAlias, and ifName in the IF-MIB module. The team prefers to complete the async migration first before addressing potential edge cases in string decoding.
📚 Learning: 2025-08-10T14:53:39.577Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:270-273
Timestamp: 2025-08-10T14:53:39.577Z
Learning: In the switchmap-ng SNMP poller migration from easysnmp to pysnmp, the pysnmp library returns bytes for OctetString values in SNMP responses for OIDs like ifDescr, ifAlias, and ifName in the IF-MIB module. The team prefers to complete the async migration first before addressing potential edge cases in string decoding.
Applied to files:
switchmap/poller/snmp/mib/generic/mib_lldp.pyswitchmap/poller/snmp/mib/generic/mib_if.pyswitchmap/poller/snmp/mib/generic/mib_bridge.py
📚 Learning: 2025-08-14T15:07:19.914Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.914Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/snmp/mib/generic/mib_if.py
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/snmp/async_poller.pyswitchmap/poller/snmp/async_snmp_info.py
🧬 Code Graph Analysis (8)
switchmap/poller/snmp/mib/generic/mib_qbridge.py (4)
switchmap/poller/snmp/mib/generic/mib_bridge.py (3)
BridgeQuery(34-414)dot1dbaseport_2_ifindex(355-389)layer1(77-88)switchmap/poller/snmp/mib/generic/mib_ip.py (1)
supported(68-82)switchmap/poller/snmp/base_query.py (1)
supported(47-65)switchmap/poller/snmp/async_snmp_manager.py (2)
swalk(443-469)get(360-396)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (4)
switchmap/poller/snmp/mib/generic/mib_if.py (2)
layer1(89-150)limited_query(106-113)switchmap/core/log.py (1)
log2warning(183-195)switchmap/poller/snmp/async_snmp_manager.py (1)
swalk(443-469)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)
switchmap/poller/snmp/mib/juniper/mib_junipervlan.py (2)
switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
dot1dbaseport_2_ifindex(355-389)switchmap/poller/snmp/async_snmp_manager.py (2)
swalk(443-469)get(360-396)
switchmap/poller/snmp/mib/generic/mib_lldp.py (7)
switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
dot1dbaseport_2_ifindex(355-389)switchmap/poller/snmp/mib/generic/mib_ip.py (1)
supported(68-82)switchmap/poller/snmp/base_query.py (1)
supported(47-65)switchmap/poller/snmp/mib/generic/mib_if.py (2)
ifindex(450-477)ifdescr(245-274)switchmap/core/log.py (1)
log2warning(183-195)switchmap/poller/snmp/async_snmp_manager.py (2)
swalk(443-469)get(360-396)switchmap/poller/snmp/snmp_manager.py (2)
swalk(334-363)get(410-447)
switchmap/poller/snmp/mib/generic/mib_if.py (5)
switchmap/poller/snmp/async_snmp_info.py (2)
system(80-130)layer1(132-189)switchmap/core/log.py (1)
log2warning(183-195)switchmap/poller/snmp/async_snmp_manager.py (1)
swalk(443-469)switchmap/poller/snmp/snmp_manager.py (1)
swalk(334-363)switchmap/core/general.py (1)
octetstr_2_string(87-102)
switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
switchmap/poller/snmp/async_snmp_manager.py (2)
oid_exists(252-289)swalk(443-469)
switchmap/poller/snmp/async_poller.py (5)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)snmp_auth(75-119)switchmap/poller/snmp/poller.py (2)
Poll(11-93)_do_poll(96-114)switchmap/poller/snmp/async_snmp_manager.py (4)
hostname(184-193)Validate(44-136)credentials(58-98)Interact(139-589)switchmap/core/log.py (2)
log2info(213-225)log2die(243-254)switchmap/poller/snmp/async_snmp_info.py (2)
Query(12-312)everything(34-64)
switchmap/poller/snmp/async_snmp_info.py (4)
switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)switchmap/poller/snmp/async_snmp_manager.py (2)
hostname(184-193)query(471-589)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)switchmap/poller/snmp/async_poller.py (1)
query(72-101)
🪛 Ruff (0.12.2)
switchmap/poller/snmp/async_snmp_manager.py
132-133: Use a single if statement instead of nested if statements
(SIM102)
275-281: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
351-353: Return the condition directly
Inline condition
(SIM103)
1102-1106: Use ternary operator key = oid_str.split(".")[-1] if normalized is True else oid_str instead of if-else-block
(SIM108)
snmp_test.py
23-23: f-string without any placeholders
Remove extraneous f prefix
(F541)
43-43: f-string without any placeholders
Remove extraneous f prefix
(F541)
53-53: f-string without any placeholders
Remove extraneous f prefix
(F541)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
60-60: f-string without any placeholders
Remove extraneous f prefix
(F541)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
71-71: f-string without any placeholders
Remove extraneous f prefix
(F541)
89-89: f-string without any placeholders
Remove extraneous f prefix
(F541)
95-95: f-string without any placeholders
Remove extraneous f prefix
(F541)
108-108: f-string without any placeholders
Remove extraneous f prefix
(F541)
111-111: f-string without any placeholders
Remove extraneous f prefix
(F541)
switchmap/poller/snmp/async_poller.py
116-117: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
switchmap/poller/snmp/async_snmp_info.py
326-326: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (15)
switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py (3)
91-106: Async gather usage and method order are correctThe asyncio.gather call and the method_names ordering align correctly (vtpVlanName, vtpVlanState, vtpVlanType) with the gathered results, addressing the prior mismatch. Looks good.
Also applies to: 98-106
123-136: Semaphore limit aligns with agreed concurrency policyUsing asyncio.Semaphore(10) adheres to the team’s chosen concurrency limit for async SNMP requests and matches previous learnings. Good choice for stability vs throughput.
405-417: Confirm VLAN bitstring mapping (offset and bit order) for vlanTrunkPortVlansEnabledPotential off-by-one or MSB/LSB ordering issues exist with bitfields. Please verify that bit index N maps to VLAN N (and whether VLAN 1 corresponds to the first or second bit) for CISCO-VTP-MIB’s vlanTrunkPortVlansEnabled on your target devices.
Would you confirm on a sample device that:
- The bit-to-VLAN mapping used here matches the device’s reported allowed VLANs, and
- Whether the bitstring is MSB-first or LSB-first per Cisco’s definition for this OID?
Optionally, we can add a small runtime check that validates a couple of known trunk ports against expected VLAN lists and logs a warning if mismatched. Let me know if you want me to draft that helper.
switchmap/poller/snmp/mib/generic/mib_bridge.py (1)
77-89: Async layer1 handoff is cleanReturning await self._macaddresstable() is a straightforward async migration and maintains prior behavior.
switchmap/poller/snmp/mib/generic/mib_qbridge.py (2)
75-86: Lazy bridge data load is a good optimizationDeferring BridgeQuery and dot1dbaseport→ifIndex mapping until needed is solid, and you guard it with supported() checks. Nice.
151-156: dot1qPvid mapping safely handles missing bridge mappingsUsing baseportifindex.get and skipping missing entries prevents KeyErrors. Good defensive practice.
switchmap/poller/snmp/mib/juniper/mib_junipervlan.py (1)
99-128: Async layer1/layer2 dependency setup is correctExplicitly awaiting _get_vlan_map() and _get_bridge_data() before data retrieval prevents race conditions and missing mapping lookups. Good sequencing.
Also applies to: 129-151
bin/systemd/switchmap_poller (2)
77-79: Migration to async poller and concurrency config looks correctSwitching to async_poll and using agent_subprocesses() as the concurrency source matches the PR goals and the new API. Parameter rename to max_concurrent_devices is consistent.
Also applies to: 92-93
35-36: run_devices is synchronous, no await or asyncio.run neededThe signature in
switchmap/poller/async_poll.py(lines 215–217) showsdef run_devices(...)with noasynckeyword, confirming it’s a sync entry point that manages its own loop. You can safely call it directly as-is.switchmap/poller/snmp/async_snmp_info.py (1)
164-171: LGTM: concurrent aggregation with robust exception handlingUse of asyncio.gather with return_exceptions and targeted per-item warning logs for layer1/layer2/layer3 is sound and resilient.
Also applies to: 172-179, 296-303, 233-240
switchmap/poller/snmp/async_snmp_manager.py (2)
155-157: Good: per-device concurrency capped at 10 via semaphoreThis aligns with the team’s validated setting for device stability and throughput.
902-913: Good: bulk walk advances from last row only, avoiding fan-out and duplicatesThis fixes the prior growth/duplication issue and improves performance and memory use.
Also applies to: 922-922
switchmap/poller/snmp/mib/generic/mib_lldp.py (1)
79-93: LGTM: lazy bridge data loading with support checksDefers BRIDGE-MIB work until needed and respects support; good for performance and correctness.
switchmap/poller/snmp/mib/generic/mib_if.py (2)
72-87: Async system() migration looks correct and minimal
system()now awaitsifstackstatus()and returns it under theIF-MIBkey. The shape aligns with the rest of the async aggregation flow. No issues spotted.
102-114: Good: per-device concurrency is rate-limited to 10Using
asyncio.Semaphore(10)is consistent with our prior decision to cap concurrent SNMP requests at 10. The localized note/TODO is helpful for future policy changes.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
switchmap/poller/snmp/async_snmp_info.py (1)
56-61: Fix incorrect logger call: missing code argument (will raise TypeError).log.log2warning requires (code, message). Currently called with a single string.
- if isinstance(result, Exception): - log.log2warning(f"{key} failed: {result}") + if isinstance(result, Exception): + log.log2warning(1004, f"{key} failed: {result}")switchmap/poller/async_poll.py (1)
186-186: Deterministic zone sorting in CLI to avoid TypeError on custom objects.- zones = sorted(config.zones()) + zones = sorted(config.zones(), key=lambda z: z.name)
🧹 Nitpick comments (3)
switchmap/poller/snmp/async_snmp_info.py (2)
350-355: Use asyncio.to_thread instead of get_event_loop/run_in_executor.Cleaner, future-proof, and avoids deprecated API usage inside coroutines.
- else: - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.system) + else: + result = await asyncio.to_thread(query.system) @@ - else: - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer1) + else: + result = await asyncio.to_thread(query.layer1) @@ - else: - - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer2) + else: + result = await asyncio.to_thread(query.layer2) @@ - else: - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer3) + else: + result = await asyncio.to_thread(query.layer3)Also applies to: 394-396, 426-427, 458-459
328-334: Micro: iterate dict items directly in _add_data.Slightly simpler and avoids extra lookups.
- for primary in source.keys(): - for secondary, value in source[primary].items(): - target[primary][secondary] = value + for primary, secondary_map in source.items(): + for secondary, value in secondary_map.items(): + target[primary][secondary] = valueswitchmap/poller/async_poll.py (1)
62-70: Formatting: fix indentation and let Black pass.There’s inconsistent indent around tasks/start/end timing. Run black.
Also applies to: 71-79
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
requirements.txt(1 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/poll.py(0 hunks)switchmap/poller/snmp/async_snmp_info.py(1 hunks)switchmap/poller/snmp/poller.py(0 hunks)switchmap/poller/snmp/snmp_info.py(0 hunks)switchmap/poller/snmp/snmp_manager.py(0 hunks)
💤 Files with no reviewable changes (4)
- switchmap/poller/snmp/snmp_manager.py
- switchmap/poller/snmp/snmp_info.py
- switchmap/poller/poll.py
- switchmap/poller/snmp/poller.py
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/snmp/async_snmp_info.py
🧬 Code graph analysis (2)
switchmap/poller/async_poll.py (4)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (5)
log2info(213-225)log2warning(183-195)log2debug(198-210)log2exception(273-307)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (2)
Poll(11-101)initialize_snmp(41-70)
switchmap/poller/snmp/async_snmp_info.py (8)
switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)switchmap/poller/snmp/base_query.py (2)
Query(4-65)supported(47-65)switchmap/poller/snmp/mib/generic/mib_if.py (2)
system(72-87)layer1(89-150)switchmap/poller/snmp/mib/generic/mib_snmpv2.py (1)
system(79-115)switchmap/core/log.py (2)
log2warning(183-195)log2debug(198-210)switchmap/poller/snmp/async_snmp_manager.py (3)
hostname(184-193)sysobjectid(222-250)query(471-589)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)switchmap/poller/snmp/async_poller.py (1)
query(72-101)
🪛 Ruff (0.12.2)
switchmap/poller/async_poll.py
164-164: Do not catch blind exception: Exception
(BLE001)
switchmap/poller/snmp/async_snmp_info.py
371-371: Consider moving this statement to an else block
(TRY300)
372-372: Do not catch blind exception: Exception
(BLE001)
402-402: Consider moving this statement to an else block
(TRY300)
404-404: Do not catch blind exception: Exception
(BLE001)
434-434: Consider moving this statement to an else block
(TRY300)
436-436: Do not catch blind exception: Exception
(BLE001)
466-466: Consider moving this statement to an else block
(TRY300)
468-468: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: PR Workflow
switchmap/poller/async_poll.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to format.
switchmap/poller/snmp/async_snmp_info.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to format.
🔇 Additional comments (1)
switchmap/poller/snmp/async_snmp_info.py (1)
1-471: Black formatting failing in CI.Run black to satisfy the pipeline; no behavior change expected.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
switchmap/poller/async_poll.py (1)
260-263: Fix docstring per pipeline and ensure proper closure.Add required sections.
-def run_cli_device(hostname): - """Run CLI device polling - main entry point.""" +def run_cli_device(hostname): + """Run CLI device polling - main entry point. + + Args: + hostname: str. Host to poll across all zones. + + Returns: + None + """
♻️ Duplicate comments (4)
switchmap/poller/async_poll.py (4)
64-69: Set aiohttp ClientSession timeout to avoid hangs.Session-wide timeout prevents indefinite waits on POSTs.
- async with aiohttp.ClientSession() as session: + timeout = aiohttp.ClientTimeout(total=30) + async with aiohttp.ClientSession(timeout=timeout) as session: tasks = [ device(argument, device_semaphore, session, post=True) for argument in arguments ]
167-173: Fix incorrect log2exception usage; switch to log2warning per project guidance.log2exception signature requires sys.exc_info; also warnings are preferred.
- except aiohttp.ClientError as e: - log_message = ( - f"HTTP error posting data for {hostname}: {e}" - ) - log.log2exception(1415, log_message) + except aiohttp.ClientError as e: + log_message = f"HTTP error posting data for {hostname}: {e}" + log.log2warning(1415, log_message) return False @@ - except Exception as e: - log_message = f"Unexpected error polling device {hostname}: {e}" - log.log2exception(1409, log_message) + except Exception as e: + log_message = f"Unexpected error polling device {hostname}: {e}" + log.log2warning(1409, log_message) return FalseAlso applies to: 186-189
208-209: Sort zones deterministically by name to avoid TypeError.sorted(config.zones()) may fail if Zone objects aren’t comparable.
- zones = sorted(config.zones()) + zones = sorted(config.zones(), key=lambda z: z.name)
44-48: Guard zones with missing hostnames (prevents TypeError).zone.hostnames can be None per ConfigPoller.zones(); iterating will crash.
Apply:
- for zone in zones: - arguments.extend( - _META(zone=zone.name, hostname=_, config=config) - for _ in zone.hostnames - ) + for zone in zones: + if not zone.hostnames: + continue + arguments.extend( + _META(zone=zone.name, hostname=_, config=config) + for _ in zone.hostnames + )
🧹 Nitpick comments (4)
switchmap/poller/async_poll.py (2)
70-73: Use monotonic clock for durations.time.monotonic() is unaffected by system clock changes.
- start_time = time.time() + start_time = time.monotonic() results = await asyncio.gather(*tasks, return_exceptions=True) - end_time = time.time() + end_time = time.monotonic() @@ - f"Polling completed in {end_time - start_time:.2f}s: " + f"Polling completed in {end_time - start_time:.2f}s: "Also applies to: 80-81
15-15: Remove unused import 'rest'.Not referenced after switching to aiohttp.
-from switchmap.core import log, rest, files +from switchmap.core import log, filesswitchmap/poller/snmp/async_snmp_info.py (2)
350-355: Use get_running_loop() instead of deprecated get_event_loop().Modern asyncio best practice; avoids warnings in 3.11+.
- loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() @@ - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() @@ - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() @@ - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop()Also applies to: 392-396, 426-427, 458-460
318-327: Docstring formatting: fix Args indentation for _add_data.Second parameter isn’t properly documented per CI rule.
async def _add_data(source, target): """Add data from source to target dict. Both dicts must have two keys. Args: - source: Source dict - target: Target dict + source: Source dict + target: Target dict Returns: target: Aggregated data """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
switchmap/poller/async_poll.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
🧬 Code graph analysis (2)
switchmap/poller/async_poll.py (4)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (5)
log2info(213-225)log2warning(183-195)log2debug(198-210)log2exception(273-307)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (2)
Poll(11-101)initialize_snmp(41-70)
switchmap/poller/snmp/async_snmp_info.py (6)
switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)switchmap/poller/snmp/base_query.py (2)
Query(4-65)supported(47-65)switchmap/core/log.py (2)
log2warning(183-195)log2debug(198-210)switchmap/poller/snmp/async_snmp_manager.py (3)
hostname(184-193)sysobjectid(222-250)query(471-589)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)switchmap/poller/snmp/async_poller.py (1)
query(72-101)
🪛 Ruff (0.12.2)
switchmap/poller/async_poll.py
186-186: Do not catch blind exception: Exception
(BLE001)
switchmap/poller/snmp/async_snmp_info.py
371-371: Consider moving this statement to an else block
(TRY300)
372-372: Do not catch blind exception: Exception
(BLE001)
402-402: Consider moving this statement to an else block
(TRY300)
404-404: Do not catch blind exception: Exception
(BLE001)
434-434: Consider moving this statement to an else block
(TRY300)
436-436: Do not catch blind exception: Exception
(BLE001)
466-466: Consider moving this statement to an else block
(TRY300)
468-468: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: PR Workflow
switchmap/poller/async_poll.py
[error] 250-250: Docstring violation: The docstring's short description on the first line doesn't have a blank line after it. Corrective Action: Add the trailing blank line. (Command: python .github/workflows/scripts/check_docstrings.py --directories switchmap setup bin tests .github)
[error] 250-250: Docstring violation: Missing 'Args' section. Corrective Action: Add an 'Args:' section listing the arguments this function accepts. (Command: python .github/workflows/scripts/check_docstrings.py --directories switchmap setup bin tests .github)
[error] 250-250: Docstring violation: The arguments defined in the docstring don't match those of the function. Corrective Action: Update your docstring to match the listed function arguments. (Command: python .github/workflows/scripts/check_docstrings.py --directories switchmap setup bin tests .github)
[error] 260-260: Docstring violation: Single line docstring without 'Args:' or 'Results:' sections defined. Corrective Action: Define the 'Args:' or 'Results:' sections. (Command: python .github/workflows/scripts/check_docstrings.py --directories switchmap setup bin tests .github)
[error] 260-260: Docstring violation: Unclosed docstring. Corrective Action: Ensure the docstring is properly closed with triple quotes. (Command: python .github/workflows/scripts/check_docstrings.py --directories switchmap setup bin tests .github)
🔇 Additional comments (3)
switchmap/poller/async_poll.py (2)
21-60: Overall async design looks good.Good use of gather + semaphore; summary logging is clear.
21-63: SNMP semaphore limit confirmed at 10
No changes needed.switchmap/poller/snmp/async_snmp_info.py (1)
12-315: Async aggregation flow is solid.Good partitioning by layers, support checks, and tolerant merging with per-item warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
switchmap/poller/async_poll.py (7)
64-69: Set aiohttp ClientTimeout (and optionally connector limit) to avoid hangsWithout a timeout, posts can hang indefinitely; optionally cap connector concurrency to match device limit.
- async with aiohttp.ClientSession() as session: + timeout = aiohttp.ClientTimeout(total=30) + # Optional: align TCP connection pool with device concurrency + connector = aiohttp.TCPConnector(limit=max_concurrent_devices) + async with aiohttp.ClientSession(timeout=timeout, connector=connector) as session: tasks = [ device(argument, device_semaphore, session, post=True) for argument in arguments ]
44-48: Guard zones with missing/empty hostnames to avoid TypeError and None entrieszone.hostnames can be None per ConfigPoller; current code will raise at iteration.
- for zone in zones: - arguments.extend( - _META(zone=zone.name, hostname=_, config=config) - for _ in zone.hostnames - ) + for zone in zones: + if not zone.hostnames: + continue + arguments.extend( + _META(zone=zone.name, hostname=hn, config=config) + for hn in zone.hostnames + if hn + )
38-40: Clamp concurrency to a sane minimum (>=1) when sourced from configPrevents creating Semaphore(0) or with non-int values causing deadlock/runtime errors.
if max_concurrent_devices is None: max_concurrent_devices = config.agent_subprocesses() + if not isinstance(max_concurrent_devices, int) or max_concurrent_devices < 1: + log.log2warning(1401, f"Invalid concurrency={max_concurrent_devices}; defaulting to 1") + max_concurrent_devices = 1
146-149: Avoid KeyError when 'misc' is absent in processed dataudevice.process() may not populate "misc".
- data = _device.process() - data["misc"]["zone"] = zone + data = _device.process() + data.setdefault("misc", {}) + data["misc"]["zone"] = zone
211-217: Guard None hostnames in CLI pathPrevents iterating None; also skip falsy entries.
- for zone in zones: - for next_hostname in zone.hostnames: + for zone in zones: + if not zone.hostnames: + continue + for next_hostname in zone.hostnames: if next_hostname == hostname: arguments.append( _META(zone=zone.name, hostname=hostname, config=config) )
208-209: Bug: sorting Zone objects without key can raise TypeErrorUse the same deterministic key as above.
- zones = sorted(config.zones()) + zones = sorted(config.zones(), key=lambda z: z.name)
167-173: Fix logging: wrong log2exception signature; use log2warning per project guidancelog.log2exception requires sys.exc_info; current call will raise. The project prefers warnings for async SNMP ops.
- except aiohttp.ClientError as e: - log_message = ( - f"HTTP error posting data for {hostname}: {e}" - ) - log.log2exception(1415, log_message) - return False + except aiohttp.ClientError as e: + log_message = f"HTTP error posting data for {hostname}: {e}" + log.log2warning(1415, log_message) + return False
🧹 Nitpick comments (3)
switchmap/poller/async_poll.py (3)
141-142: Consider a per-device timeout to prevent stuck SNMP queriesProtects against devices that never respond.
- snmp_data = await poll.query() + snmp_data = await asyncio.wait_for(poll.query(), timeout=60)
152-166: Treat non-2xx responses as failure (not just !=200)APIs may return 201/204 on success.
- ) as res: - if res.status == 200: + ) as res: + if 200 <= res.status < 300: log_message = ( f"Successfully polled and posted data " f"for {hostname}" ) log.log2debug(1407, log_message) else:
224-231: Add timeout to CLI session as well for consistencyMirror main path behavior.
- semaphore = asyncio.Semaphore(1) - async with aiohttp.ClientSession() as session: + semaphore = asyncio.Semaphore(1) + timeout = aiohttp.ClientTimeout(total=30) + async with aiohttp.ClientSession(timeout=timeout) as session: tasks = [ device(argument, semaphore, session, post=False) for argument in arguments ] results = await asyncio.gather(*tasks, return_exceptions=True)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
switchmap/poller/async_poll.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/async_poll.py
🧬 Code graph analysis (1)
switchmap/poller/async_poll.py (4)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (5)
log2info(213-225)log2warning(183-195)log2debug(198-210)log2exception(273-307)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (2)
Poll(11-101)initialize_snmp(41-70)
🪛 Ruff (0.12.2)
switchmap/poller/async_poll.py
186-186: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
switchmap/poller/async_poll.py (1)
152-154: Confirm API auth/base URL parity with legacy rest.postThe prior rest.post likely handled base URL, auth headers, and TLS config. Ensure aiohttp POST reproduces that behavior (headers, tokens, cookies, verify).
Would you confirm whether API_POLLER_POST_URI is an absolute URL and whether auth headers/certs from ConfigAPIClient are required here?
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
switchmap/poller/snmp/async_snmp_info.py (6)
105-116: Do not let supported() errors kill the whole section; gather with return_exceptions and log.One failing MIB support probe will currently bubble an exception and abort the entire system pass.
Apply:
- # Check if supported - support_results = await asyncio.gather( - *[item.supported() for item, _ in query_items] - ) - - supported_items = [ - (item, name) - for (item, name), supported in zip(query_items, support_results) - if supported - ] + # Check if supported (don't fail the whole section on one probe) + support_results = await asyncio.gather( + *[item.supported() for item, _ in query_items], + return_exceptions=True, + ) + supported_items = [] + for (item, name), res in zip(query_items, support_results): + if res is True: + supported_items.append((item, name)) + elif isinstance(res, Exception): + log.log2warning(1300, f"system.supported() error in {name}: {res}")
155-165: Same hardening for layer1 supported() check.- support_results = await asyncio.gather( - *[item.supported() for item, _ in query_items] - ) - - supported_items = [ - (item, name) - for (item, name), supported in zip(query_items, support_results) - if supported - ] + support_results = await asyncio.gather( + *[item.supported() for item, _ in query_items], + return_exceptions=True, + ) + supported_items = [] + for (item, name), res in zip(query_items, support_results): + if res is True: + supported_items.append((item, name)) + elif isinstance(res, Exception): + log.log2warning(1300, f"layer1.supported() error in {name}: {res}")
215-225: Same hardening for layer2 supported() check.- support_results = await asyncio.gather( - *[item.supported() for item, _ in query_items] - ) - - # Filter supported MIBs - supported_items = [ - (item, name) - for (item, name), supported in zip(query_items, support_results) - if supported - ] + support_results = await asyncio.gather( + *[item.supported() for item, _ in query_items], + return_exceptions=True, + ) + supported_items = [] + for (item, name), res in zip(query_items, support_results): + if res is True: + supported_items.append((item, name)) + elif isinstance(res, Exception): + log.log2warning(1300, f"layer2.supported() error in {name}: {res}")
278-288: Same hardening for layer3 supported() check.- support_results = await asyncio.gather( - *[item.supported() for item, _ in query_items] - ) - - # Filter supported MIBs - supported_items = [ - (item, name) - for (item, name), supported in zip(query_items, support_results) - if supported - ] + support_results = await asyncio.gather( + *[item.supported() for item, _ in query_items], + return_exceptions=True, + ) + supported_items = [] + for (item, name), res in zip(query_items, support_results): + if res is True: + supported_items.append((item, name)) + elif isinstance(res, Exception): + log.log2warning(1300, f"layer3.supported() error in {name}: {res}")
353-355: Prefer asyncio.to_thread over get_event_loop().run_in_executor.Modern, clearer, and avoids deprecated loop access.
- loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.system) + result = await asyncio.to_thread(query.system) @@ - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer1) + result = await asyncio.to_thread(query.layer1) @@ - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer2) + result = await asyncio.to_thread(query.layer2) @@ - loop = asyncio.get_event_loop() - result = await loop.run_in_executor(None, query.layer3) + result = await asyncio.to_thread(query.layer3)Also applies to: 394-396, 426-428, 458-460
328-334: Tighten _add_data loop (readability).- for primary in source.keys(): - for secondary, value in source[primary].items(): - target[primary][secondary] = value + for primary, secondary_map in source.items(): + for secondary, value in secondary_map.items(): + target[primary][secondary] = valueswitchmap/poller/async_poll.py (5)
37-41: Honor config fallback when concurrency is None (avoid spurious warning).Docstring says we use config when None; current code forces 1 and warns.
- # Use config value if not provided - if not isinstance(max_concurrent_devices,int) or max_concurrent_devices < 1: - log.log2warning(1401, f"Invalid concurrency={max_concurrent_devices}; defaulting to 1") - max_concurrent_devices = 1 + # Use config value if not provided + if max_concurrent_devices is None: + max_concurrent_devices = config.agent_subprocesses() + # Clamp and warn if invalid + if not isinstance(max_concurrent_devices, int) or max_concurrent_devices < 1: + log.log2warning(1401, f"Invalid concurrency={max_concurrent_devices}; defaulting to 1") + max_concurrent_devices = 1
164-175: Treat any 2xx post as success.Some APIs return 201/202 on success.
- if res.status == 200: + if 200 <= res.status < 300: log_message = ( f"Successfully polled and posted data " f"for {hostname}" ) log.log2debug(1407, log_message) else:
97-105: Add timeout retry with exponential backoff (team preference).Implements resilient re-polling on asyncio.TimeoutError without retrying non-timeout failures.
-async def device(poll_meta, device_semaphore, session, post=True): +async def device(poll_meta, device_semaphore, session, post=True, max_retries=3): @@ - try: - poll = async_poller.Poll(hostname) - - # Initialize SNMP connection - if not await poll.initialize_snmp(): - log_message = f"Failed to initialize SNMP for {hostname}" - log.log2debug(1406, log_message) - return False - - # Query device data asynchronously - snmp_data = await poll.query() - - # Process if we get valid data - if bool(snmp_data) and isinstance(snmp_data, dict): - # Process device data - _device = udevice.Device(snmp_data) - data = _device.process() - data["misc"]["zone"] = zone - - if post: - try: - # Construct full URL for posting - url = f"{config.server_url_root()}{API_PREFIX}{API_POLLER_POST_URI}" - log_message = f"Posting data for {hostname} to {url}" - log.log2debug(1416, log_message) - - async with session.post( - url, json=data - ) as res: - if res.status == 200: - log_message = ( - f"Successfully polled and posted data " - f"for {hostname}" - ) - log.log2debug(1407, log_message) - else: - log_message = ( - f"Failed to post data for {hostname}, " - f"status={res.status}" - ) - log.log2warning(1414, log_message) - except aiohttp.ClientError as e: - log_message = ( - f"HTTP error posting data for {hostname}: {e}" - ) - log.log2warning(1415, log_message) - return False - - else: - pprint(data) - - return True - else: - log_message = ( - f"Device {hostname} returns no data. Check " - f"connectivity/SNMP configuration" - ) - log.log2debug(1408, log_message) - return False - - except (asyncio.TimeoutError, KeyError, ValueError) as e: - log_message = f"Recoverable error polling device {hostname}: {e}" - log.log2warning(1409, log_message) - return False - except Exception as e: - log_message = f"Unexpected error polling device {hostname}: {e}" - log.log2warning(1409, log_message) - return False + for attempt in range(max_retries + 1): + try: + poll = async_poller.Poll(hostname) + + # Initialize SNMP connection + if not await poll.initialize_snmp(): + log_message = f"Failed to initialize SNMP for {hostname}" + log.log2debug(1406, log_message) + return False + + # Query device data asynchronously + snmp_data = await poll.query() + + # Process if we get valid data + if bool(snmp_data) and isinstance(snmp_data, dict): + _device = udevice.Device(snmp_data) + data = _device.process() + data["misc"]["zone"] = zone + + if post: + try: + url = f"{config.server_url_root()}{API_PREFIX}{API_POLLER_POST_URI}" + log.log2debug(1416, f"Posting data for {hostname} to {url}") + async with session.post(url, json=data) as res: + if 200 <= res.status < 300: + log.log2debug(1407, f"Successfully polled and posted data for {hostname}") + else: + log.log2warning(1414, f"Failed to post data for {hostname}, status={res.status}") + except aiohttp.ClientError as e: + log.log2warning(1415, f"HTTP error posting data for {hostname}: {e}") + return False + else: + pprint(data) + + return True + + log.log2debug(1408, f"Device {hostname} returns no data. Check connectivity/SNMP configuration") + return False + + except asyncio.TimeoutError as e: + if attempt < max_retries: + delay = 2 ** attempt + log.log2warning(1409, f"Timeout polling {hostname} (attempt {attempt+1}/{max_retries+1}); retrying in {delay}s: {e}") + await asyncio.sleep(delay) + continue + log.log2warning(1410, f"Max retries exceeded for {hostname} due to timeout: {e}") + return False + except (KeyError, ValueError) as e: + log.log2warning(1411, f"Non-retryable error polling {hostname}: {e}") + return False + except Exception as e: + log.log2warning(1412, f"Unexpected error polling device {hostname}: {e}") + return FalseAlso applies to: 135-203
221-221: Deterministic zone ordering; avoid TypeError if Zone isn’t comparable.- zones = sorted(config.zones()) + zones = sorted(config.zones(), key=lambda z: z.name)
126-133: Harden 'none' check: ignore surrounding whitespace.- not hostname - or not isinstance(hostname, str) - or hostname.lower() == "none" + not hostname + or not isinstance(hostname, str) + or hostname.strip().lower() == "none"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
switchmap/core/configuration.py(0 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(1 hunks)
💤 Files with no reviewable changes (1)
- switchmap/core/configuration.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-31T13:36:01.640Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng async polling, the data structure passed to udevice.Device() comes from async_snmp_info.Query.everything() which initializes it as defaultdict(lambda: defaultdict(dict)) and explicitly sets the "misc" key. This means accessing data["misc"]["zone"] is safe without defensive programming like setdefault().
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap/poller/update/device.py, the Device.process() method returns data structured as defaultdict(lambda: defaultdict(dict)) which automatically creates missing keys when accessed. The "misc" key is guaranteed to exist and be a dict, so accessing data["misc"]["zone"] is safe without using setdefault().
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng's async_snmp_info.py, all data collection methods (misc, system, layer1, layer2, layer3) use defaultdict(lambda: defaultdict(dict)) which creates nested defaultdicts. This means accessing data["misc"]["zone"] or any other nested key path will automatically create missing keys without raising KeyError, making defensive programming with setdefault() unnecessary.
Applied to files:
switchmap/poller/async_poll.pyswitchmap/poller/snmp/async_snmp_info.py
📚 Learning: 2025-08-10T14:53:39.577Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:270-273
Timestamp: 2025-08-10T14:53:39.577Z
Learning: In the switchmap-ng SNMP poller migration from easysnmp to pysnmp, the pysnmp library returns bytes for OctetString values in SNMP responses for OIDs like ifDescr, ifAlias, and ifName in the IF-MIB module. The team prefers to complete the async migration first before addressing potential edge cases in string decoding.
Applied to files:
switchmap/poller/snmp/async_snmp_info.py
🧬 Code graph analysis (2)
switchmap/poller/async_poll.py (6)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (4)
log2warning(183-195)log2info(213-225)log2debug(198-210)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (2)
Poll(11-101)initialize_snmp(41-70)switchmap/core/configuration.py (1)
server_url_root(325-344)tests/switchmap_/server/db/ingest/test_ingest.py (1)
agent_subprocesses(235-244)
switchmap/poller/snmp/async_snmp_info.py (7)
switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)switchmap/poller/snmp/base_query.py (2)
Query(4-65)supported(47-65)switchmap/poller/snmp/mib/generic/mib_if.py (2)
system(72-87)layer1(89-150)switchmap/poller/snmp/mib/generic/mib_snmpv2.py (1)
system(79-115)switchmap/core/log.py (2)
log2warning(183-195)log2debug(198-210)switchmap/poller/snmp/async_snmp_manager.py (3)
hostname(184-193)sysobjectid(222-250)query(471-589)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)
🪛 Ruff (0.12.2)
switchmap/poller/async_poll.py
199-199: Do not catch blind exception: Exception
(BLE001)
switchmap/poller/snmp/async_snmp_info.py
371-371: Consider moving this statement to an else block
(TRY300)
372-372: Do not catch blind exception: Exception
(BLE001)
402-402: Consider moving this statement to an else block
(TRY300)
404-404: Do not catch blind exception: Exception
(BLE001)
434-434: Consider moving this statement to an else block
(TRY300)
436-436: Do not catch blind exception: Exception
(BLE001)
466-466: Consider moving this statement to an else block
(TRY300)
468-468: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
switchmap/poller/async_poll.py (1)
64-76: LGTM: bounded concurrency + shared session with timeout.Good use of a device-level semaphore and a session-wide ClientTimeout; gather with return_exceptions and post‑processing summary is clean.
…tem queries MIBs, converted sys MIBs to async
…e polling issue added a max polling timeout
abbi4code
force-pushed
the
async_device_poll
branch
from
f927ecd to
0d31de7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
snmp_test.py (1)
24-24: Don’t hardcode target IP; accept via CLIUse argparse to pass the hostname. Safer and reusable. This also avoids leaking targets.
@@ -async def test_everything(): +async def test_everything(hostname: str): @@ - hostname = "162.249.37.218" + # hostname provided by caller @@ - result = await test_everything() + parser = argparse.ArgumentParser(description="Test async_snmp_info.everything()") + parser.add_argument("-H", "--host", required=True, help="Target hostname/IP") + args = parser.parse_args() + result = await test_everything(args.host)Also applies to: 109-121
test_all_devices.py (2)
4-4: Remove absolute sys.path injectionHard-coded user path breaks portability and leaks local FS details.
-sys.path.insert(0, "/Users/imexyyyyy/files/gsoc/switchmap-ng")
1-18: Don’t ship as pytest-discoverable “test_*.py”This can trigger live fleet polling in CI. Move under scripts/ or rename.
Options:
- Rename to scripts/dev/all_devices.py
- Or add pytest.ini to ignore this path
🧹 Nitpick comments (7)
snmp_test.py (1)
11-11: Avoid sys.path hacksRely on proper packaging/imports; path injection reduces portability.
-sys.path.insert(0, ".")switchmap/poller/snmp/async_snmp_manager.py (1)
1061-1066: Narrow broad exception in conversion fallback (ruff BLE001)Use specific exceptions for safer handling and to satisfy lint.
- try: - return bytes(str(value), "utf-8") - except Exception: - return None + try: + return bytes(str(value), "utf-8") + except (TypeError, ValueError, UnicodeEncodeError): + return NoneApply the same change to any remaining fallback in
_convert.Also applies to: 1195-1199
switchmap/poller/async_poll.py (5)
35-41: Honor docstring: use config default when concurrency is Nonedevices() claims to default from config but doesn’t. Implement the fallback then clamp.
- # Use config value if not provided - if not isinstance(max_concurrent_devices,int) or max_concurrent_devices < 1: + # Default from config if None, then clamp + if max_concurrent_devices is None: + max_concurrent_devices = config.agent_subprocesses() + if not isinstance(max_concurrent_devices, int) or max_concurrent_devices < 1: log.log2warning(1401, f"Invalid concurrency={max_concurrent_devices}; defaulting to 1") max_concurrent_devices = 1
221-221: Sort zones by name in CLI tooMatch devices() to avoid TypeError on non-comparable objects.
-zones = sorted(config.zones()) +zones = sorted(config.zones(), key=lambda z: z.name)
241-247: Set aiohttp timeout in CLI pathAvoid indefinite hangs; mirror the 30s timeout used above.
- async with aiohttp.ClientSession() as session: + timeout = aiohttp.ClientTimeout(total=30) + async with aiohttp.ClientSession(timeout=timeout) as session:
195-203: Broad except trips ruff (BLE001) — keep as last‑resort with noqaYou already catch specific recoverables; annotate catch‑all.
- except Exception as e: + except Exception as e: # noqa: BLE001 - last-resort guard to prevent task crash storms log_message = f"Unexpected error polling device {hostname}: {e}" log.log2warning(1409, log_message) return False
135-143: Add retry with exponential backoff on timeouts (preferred behavior)Improves resilience to transient network issues per PR objectives/learners.
- try: - poll = async_poller.Poll(hostname) - # Initialize SNMP connection - if not await poll.initialize_snmp(): - log_message = f"Failed to initialize SNMP for {hostname}" - log.log2debug(1406, log_message) - return False - # Query device data asynchronously - snmp_data = await poll.query() + max_retries = 3 + for attempt in range(max_retries + 1): + try: + poll = async_poller.Poll(hostname) + # Initialize SNMP connection + if not await poll.initialize_snmp(): + log_message = f"Failed to initialize SNMP for {hostname}" + log.log2debug(1406, log_message) + return False + # Query device data asynchronously + snmp_data = await poll.query() + break + except asyncio.TimeoutError as e: + if attempt < max_retries: + wait_s = 2 ** attempt + log.log2warning(1417, f"Timeout polling {hostname} (attempt {attempt+1}/{max_retries+1}); retrying in {wait_s}s: {e}") + await asyncio.sleep(wait_s) + continue + else: + log.log2warning(1410, f"Max retries exceeded for {hostname} due to timeout: {e}") + return False @@ - except (asyncio.TimeoutError, KeyError, ValueError) as e: + except (KeyError, ValueError) as e: log_message = f"Recoverable error polling device {hostname}: {e}" log.log2warning(1409, log_message) return False - except Exception as e: + except Exception as e: # noqa: BLE001 log_message = f"Unexpected error polling device {hostname}: {e}" log.log2warning(1409, log_message) return FalseAlso applies to: 145-186, 195-203
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bin/systemd/switchmap_poller(3 hunks)requirements.txt(1 hunks)snmp_test.py(2 hunks)switchmap/core/configuration.py(0 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/poll.py(0 hunks)switchmap/poller/snmp/async_poller.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(3 hunks)switchmap/poller/snmp/async_snmp_manager.py(1 hunks)switchmap/poller/snmp/poller.py(0 hunks)switchmap/poller/snmp/snmp_info.py(0 hunks)switchmap/poller/snmp/snmp_manager.py(0 hunks)test_all_devices.py(1 hunks)
💤 Files with no reviewable changes (5)
- switchmap/poller/snmp/snmp_manager.py
- switchmap/poller/snmp/poller.py
- switchmap/core/configuration.py
- switchmap/poller/poll.py
- switchmap/poller/snmp/snmp_info.py
✅ Files skipped from review due to trivial changes (1)
- switchmap/poller/snmp/async_poller.py
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/systemd/switchmap_poller
- requirements.txt
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-10T14:53:39.577Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:270-273
Timestamp: 2025-08-10T14:53:39.577Z
Learning: In the switchmap-ng SNMP poller migration from easysnmp to pysnmp, the pysnmp library returns bytes for OctetString values in SNMP responses for OIDs like ifDescr, ifAlias, and ifName in the IF-MIB module. The team prefers to complete the async migration first before addressing potential edge cases in string decoding.
Applied to files:
switchmap/poller/snmp/async_snmp_manager.py
📚 Learning: 2025-08-14T11:10:31.364Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_ipv6.py:64-64
Timestamp: 2025-08-14T11:10:31.364Z
Learning: In the switchmap-ng codebase, maintain the existing convention of using `self.snmp_object` instead of `self._snmp_object` when referencing the SNMP object in MIB query classes.
Applied to files:
switchmap/poller/snmp/async_snmp_manager.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng's async_snmp_info.py, all data collection methods (misc, system, layer1, layer2, layer3) use defaultdict(lambda: defaultdict(dict)) which creates nested defaultdicts. This means accessing data["misc"]["zone"] or any other nested key path will automatically create missing keys without raising KeyError, making defensive programming with setdefault() unnecessary.
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng async polling, the data structure passed to udevice.Device() comes from async_snmp_info.Query.everything() which initializes it as defaultdict(lambda: defaultdict(dict)) and explicitly sets the "misc" key. This means accessing data["misc"]["zone"] is safe without defensive programming like setdefault().
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap/poller/update/device.py, the Device.process() method returns data structured as defaultdict(lambda: defaultdict(dict)) which automatically creates missing keys when accessed. The "misc" key is guaranteed to exist and be a dict, so accessing data["misc"]["zone"] is safe without using setdefault().
Applied to files:
switchmap/poller/snmp/async_snmp_info.pyswitchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:36:01.640Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
🧬 Code graph analysis (4)
test_all_devices.py (1)
switchmap/poller/async_poll.py (1)
run_devices(265-280)
switchmap/poller/snmp/async_snmp_manager.py (1)
switchmap/core/log.py (1)
log2warning(183-195)
switchmap/poller/snmp/async_snmp_info.py (4)
switchmap/core/log.py (2)
log2warning(183-195)log2exception(273-307)switchmap/poller/snmp/async_snmp_manager.py (2)
sysobjectid(222-250)hostname(184-193)switchmap/poller/snmp/iana_enterprise.py (1)
enterprise(41-51)switchmap/poller/snmp/__init__.py (1)
get_queries(50-64)
switchmap/poller/async_poll.py (5)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (4)
log2warning(183-195)log2info(213-225)log2debug(198-210)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (2)
Poll(12-102)initialize_snmp(42-71)switchmap/core/configuration.py (1)
server_url_root(325-344)
🪛 Ruff (0.12.2)
switchmap/poller/snmp/async_snmp_manager.py
1197-1197: Do not catch blind exception: Exception
(BLE001)
snmp_test.py
9-9: SyntaxError: Expected a statement
9-9: SyntaxError: Expected a statement
9-9: SyntaxError: Expected a statement
9-9: SyntaxError: Expected a statement
10-10: SyntaxError: Expected a statement
10-10: SyntaxError: Expected a statement
10-10: SyntaxError: Expected a statement
10-10: SyntaxError: Expected a statement
10-11: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Expected ',', found ':'
13-13: SyntaxError: Expected ',', found name
13-13: SyntaxError: Expected ',', found name
13-13: SyntaxError: Expected ',', found name
13-13: SyntaxError: Unparenthesized generator expression cannot be used here
13-13: SyntaxError: Expected 'in', found ')'
56-56: SyntaxError: Expected except or finally after try block
56-56: SyntaxError: Expected a statement
56-56: SyntaxError: Expected a statement
56-56: SyntaxError: Expected a statement
57-57: SyntaxError: Expected a statement
57-57: SyntaxError: Expected a statement
57-57: SyntaxError: Expected a statement
57-57: SyntaxError: Expected a statement
57-58: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Simple statements must be separated by newlines or semicolons
59-59: SyntaxError: Expected ',', found ':'
59-59: SyntaxError: Expected ',', found name
59-59: SyntaxError: Expected ',', found name
59-59: SyntaxError: Expected ',', found name
59-59: SyntaxError: Unparenthesized generator expression cannot be used here
59-59: SyntaxError: Expected 'in', found ')'
switchmap/poller/snmp/async_snmp_info.py
257-257: Redefinition of unused layer1 from line 135
(F811)
314-314: Redefinition of unused layer2 from line 194
(F811)
393-393: f-string without any placeholders
Remove extraneous f prefix
(F541)
395-395: Shebang should contain python, pytest, or uv run
(EXE003)
395-395: Shebang is present but file is not executable
(EXE001)
395-395: Shebang should be at the beginning of the file
(EXE005)
401-401: Undefined name hostname
(F821)
406-406: Undefined name hostname
(F821)
417-417: Undefined name hostname
(F821)
421-421: Undefined name hostname
(F821)
423-423: Undefined name hostname
(F821)
switchmap/poller/async_poll.py
199-199: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: PR Workflow
switchmap/poller/snmp/async_snmp_manager.py
[warning] 1-1: Black would reformat this file.
snmp_test.py
[error] 9-9: Black formatting error: Cannot parse: 9:0: <<<<<<< HEAD
switchmap/poller/snmp/async_snmp_info.py
[warning] 1-1: Black would reformat this file.
switchmap/poller/async_poll.py
[warning] 1-1: Black would reformat this file.
🔇 Additional comments (3)
switchmap/poller/snmp/async_snmp_manager.py (1)
155-157: Semaphore cap aligned with team guidancePer learnings, hardcoded 10 is correct. LGTM.
switchmap/poller/snmp/async_snmp_info.py (1)
66-81: Good: vendor lookup guarded by sysObjectIDNull-safe IANA enterprise handling. LGTM.
switchmap/poller/async_poll.py (1)
156-176: Good: async HTTP post, proper URL, and status handlingNon-blocking post via aiohttp with debug/warning logs. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
switchmap/poller/snmp/async_snmp_manager.py (1)
517-543: Consolidate duplicate top-level_convertdefinitions (fix override).Found two
def _convert(value):in switchmap/poller/snmp/async_snmp_manager.py at lines 983 and 1068; the later definition overrides the earlier — keep a single implementation or rename one and update callers.
♻️ Duplicate comments (3)
switchmap/poller/async_poll.py (2)
225-226: Sort zones deterministically by name.Avoids TypeError if Zone objects aren’t comparable and ensures stable order.
- zones = sorted(config.zones()) + zones = sorted(config.zones(), key=lambda z: z.name)
37-47: Honor config default when concurrency is None; current code forces 1.Docstring says None should use config.agent_subprocesses(), but implementation defaults to 1. Fix to avoid unintended throttling.
- # Use config value if not provided - if ( - not isinstance(max_concurrent_devices, int) - or max_concurrent_devices < 1 - ): - log.log2warning( - 1401, - f"Invalid concurrency={max_concurrent_devices}; defaulting to 1", - ) - max_concurrent_devices = 1 + # Resolve concurrency + if max_concurrent_devices is None: + max_concurrent_devices = config.agent_subprocesses() + elif ( + not isinstance(max_concurrent_devices, int) + or max_concurrent_devices < 1 + ): + log.log2warning( + 1401, + f"Invalid concurrency={max_concurrent_devices}; defaulting to 1", + ) + max_concurrent_devices = 1switchmap/poller/snmp/async_snmp_manager.py (1)
1068-1152: Remove duplicate _convert; breaks CI (F811) and risks divergence.There’s a second definition of _convert that shadows the canonical one above and fails flake8 (F811). Drop the duplicate block entirely.
-def _convert(value): - """Convert SNMP value from pysnmp object to Python type. - ... - """ - # Handle pysnmp exception values - if isinstance(value, NoSuchObject): - return None - if isinstance(value, NoSuchInstance): - return None - if isinstance(value, EndOfMibView): - return None - ... - try: - return bytes(str(value), "utf-8") - except Exception: - return None
🧹 Nitpick comments (4)
switchmap/poller/snmp/async_snmp_manager.py (1)
1061-1066: Narrow blind exception in fallback conversion (BLE001).Catching Exception broadly is flagged. Limit to specific conversion errors.
- try: - return bytes(str(value), "utf-8") - except Exception: - return None + try: + return bytes(str(value), "utf-8") + except (ValueError, TypeError, UnicodeError): + return Noneswitchmap/poller/async_poll.py (3)
160-166: Fix long line (E501) and improve URL construction readability.Split into components to satisfy 80-char limit.
- url = f"{config.server_url_root()}{API_PREFIX}{API_POLLER_POST_URI}" + base_url = config.server_url_root() + path = f"{API_PREFIX}{API_POLLER_POST_URI}" + url = f"{base_url}{path}"
203-206: Remove blind catch; let gather(return_exceptions=True) handle unknown errors.This satisfies BLE001 and surfaces exceptions to the caller where they’re already logged per-device.
- except Exception as e: - log_message = f"Unexpected error polling device {hostname}: {e}" - log.log2warning(1409, log_message) - return False + # Let unexpected exceptions propagate to asyncio.gather(...)
150-153: Add timeout retry with exponential backoff (per team preference).Retries only on asyncio.TimeoutError; avoids retrying on validation errors.
- # Query device data asynchronously - snmp_data = await poll.query() + # Query device data asynchronously with timeout retries + max_retries = 3 + backoff = 1 + for attempt in range(max_retries + 1): + try: + snmp_data = await poll.query() + break + except asyncio.TimeoutError as e: + if attempt < max_retries: + log.log2warning( + 1409, + f"Timeout polling {hostname} (attempt {attempt+1}/{max_retries+1}); " + f"retrying in {backoff}s: {e}", + ) + await asyncio.sleep(backoff) + backoff *= 2 + continue + else: + log.log2warning( + 1410, + f"Max retries exceeded for {hostname} due to timeout: {e}", + ) + return False @@ - except (asyncio.TimeoutError, KeyError, ValueError) as e: + except (KeyError, ValueError) as e: log_message = f"Recoverable error polling device {hostname}: {e}" log.log2warning(1409, log_message) return FalseI can push a full patch if you prefer.
Also applies to: 199-203
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
snmp_test.py(0 hunks)switchmap/poller/async_poll.py(1 hunks)switchmap/poller/snmp/async_snmp_info.py(2 hunks)switchmap/poller/snmp/async_snmp_manager.py(1 hunks)
💤 Files with no reviewable changes (1)
- snmp_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- switchmap/poller/snmp/async_snmp_info.py
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
📚 Learning: 2025-08-09T22:25:34.114Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/async_snmp_manager.py:160-160
Timestamp: 2025-08-09T22:25:34.114Z
Learning: In the switchmap-ng SNMP poller implementation (async_snmp_manager.py), the semaphore concurrency limit for async SNMP requests should be hardcoded to 10. Testing has shown that most SNMP devices can handle 10 concurrent requests without getting overwhelmed, providing better performance than a lower limit while maintaining stability.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:36:01.640Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:186-189
Timestamp: 2025-08-31T13:36:01.640Z
Learning: In the switchmap-ng async polling implementation (async_poll.py), timeout errors during device polling should be handled with retry logic rather than immediate failure. The user prefers implementing re-polling with exponential backoff for timeout errors to make the system more resilient to temporary network issues.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:07:19.958Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:102-105
Timestamp: 2025-08-14T15:07:19.958Z
Learning: In the switchmap-ng IF-MIB async migration, the developer initially chose a semaphore limit of 8 concurrent queries (for 15 total OIDs) by dividing roughly in half to avoid overwhelming SNMP devices, showing consideration for device capacity when determining concurrency limits.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-24T14:56:45.197Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/snmp/async_snmp_info.py:0-0
Timestamp: 2025-08-24T14:56:45.197Z
Learning: In the switchmap-ng project, when handling errors in async SNMP operations, the preference is to use warning logs (log.log2warning) instead of exception logs (log.log2exception) for better error handling and cleaner logging output.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-14T15:17:56.012Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/cisco/mib_ciscovtp.py:125-133
Timestamp: 2025-08-14T15:17:56.012Z
Learning: The switchmap-ng project uses a custom logging module with log.log2warning for warning-level logging instead of the standard Python logging module.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng async polling, the data structure passed to udevice.Device() comes from async_snmp_info.Query.everything() which initializes it as defaultdict(lambda: defaultdict(dict)) and explicitly sets the "misc" key. This means accessing data["misc"]["zone"] is safe without defensive programming like setdefault().
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap/poller/update/device.py, the Device.process() method returns data structured as defaultdict(lambda: defaultdict(dict)) which automatically creates missing keys when accessed. The "misc" key is guaranteed to exist and be a dict, so accessing data["misc"]["zone"] is safe without using setdefault().
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-31T13:33:52.738Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#334
File: switchmap/poller/async_poll.py:146-149
Timestamp: 2025-08-31T13:33:52.738Z
Learning: In switchmap-ng's async_snmp_info.py, all data collection methods (misc, system, layer1, layer2, layer3) use defaultdict(lambda: defaultdict(dict)) which creates nested defaultdicts. This means accessing data["misc"]["zone"] or any other nested key path will automatically create missing keys without raising KeyError, making defensive programming with setdefault() unnecessary.
Applied to files:
switchmap/poller/async_poll.py
📚 Learning: 2025-08-10T14:53:39.577Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_if.py:270-273
Timestamp: 2025-08-10T14:53:39.577Z
Learning: In the switchmap-ng SNMP poller migration from easysnmp to pysnmp, the pysnmp library returns bytes for OctetString values in SNMP responses for OIDs like ifDescr, ifAlias, and ifName in the IF-MIB module. The team prefers to complete the async migration first before addressing potential edge cases in string decoding.
Applied to files:
switchmap/poller/snmp/async_snmp_manager.py
📚 Learning: 2025-08-14T11:10:31.364Z
Learnt from: abbi4code
PR: PalisadoesFoundation/switchmap-ng#328
File: switchmap/poller/snmp/mib/generic/mib_ipv6.py:64-64
Timestamp: 2025-08-14T11:10:31.364Z
Learning: In the switchmap-ng codebase, maintain the existing convention of using `self.snmp_object` instead of `self._snmp_object` when referencing the SNMP object in MIB query classes.
Applied to files:
switchmap/poller/snmp/async_snmp_manager.py
🧬 Code graph analysis (2)
switchmap/poller/async_poll.py (5)
switchmap/poller/configuration.py (2)
ConfigPoller(8-175)hostnames(39-59)switchmap/core/log.py (4)
log2warning(183-195)log2info(213-225)log2debug(198-210)log2see(228-240)switchmap/core/files.py (1)
skip_file(359-375)switchmap/poller/snmp/async_poller.py (3)
Poll(11-101)initialize_snmp(41-70)query(72-101)switchmap/core/configuration.py (2)
server_url_root(348-367)agent_subprocesses(58-79)
switchmap/poller/snmp/async_snmp_manager.py (1)
switchmap/core/log.py (1)
log2warning(183-195)
🪛 Ruff (0.12.2)
switchmap/poller/async_poll.py
203-203: Do not catch blind exception: Exception
(BLE001)
switchmap/poller/snmp/async_snmp_manager.py
1149-1149: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: PR Workflow
switchmap/poller/async_poll.py
[error] 163-163: flake8: E501 line too long (92 > 80 characters).
switchmap/poller/snmp/async_snmp_manager.py
[error] 1068-1068: flake8: F811 redefinition of unused '_convert' from line 983.
🔇 Additional comments (2)
switchmap/poller/async_poll.py (2)
64-69: Nice: clear summary logging and per-device error reporting.The aggregated success/failed/error counters plus per-device warnings give good operational insight.
Also applies to: 84-101
243-251: Good: share one semaphore and session for CLI multi-zone polls.Prevents unconstrained concurrency and redundant TCP handshakes.
|
@coderabbitai full review |
|
Please fix the failing tests |
What kind of change does this PR introduce?
Replace the current multiprocessing-based device polling with async/await patterns for better performance and resource efficiency.
Current State
Proposed PR
Issue Number:
Fixes #335
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit