[change] NetJSON DeviceMonitoring compliance #2 - #62
Conversation
|
@pandafy @nemesisdesign In the last commit (d46b568), I've made the
Todo: We need to define new methods to collect as much monitoring info as possible. |
You can go ahead with this, since it will make it clear for us what methods are to be removed. We can always revert the commit if something went wrong.
instead of being exhaustive, let's start with minimal data. Check what data is being collected in openwrt-openwisp-monitoring and add support for it here. Otherwise we may spend time collecting every possible metric many of which might not be used in OpenWISP. We can always iterate to make it better. |
|
Thanks @pandafy , I have updated the OpenWRT backend for now (will do AirOS later). Below you can see the comparison between netengine and openwrt-openwisp-monitoring. I've added a few more metrics but it may not be possible to add all. Now I need to fix some tests. output from openwrt-openwisp-monitoring
{
"type": "DeviceMonitoring",
"general": {
"local_time": 1623319682,
"uptime": 178,
"hostname": "OpenWrt"
},
"dhcp_leases": [
{
"mac": "04:0e:3c:ca:55:5f",
"client_id": "01:04:0e:3c:ca:55:5f",
"client_name": "purhan",
"ip": "192.168.2.140",
"expiry": 1623362752
}
],
"interfaces": [
{
"type": "vlan",
"up": false,
"name": "eth0.2"
},
{
"mac": "1c:3b:f3:10:0a:42",
"type": "bridge",
"stp": false,
"up": true,
"txqueuelen": 1000,
"name": "br-lan",
"multicast": true,
"addresses": [
{
"proto": "static",
"address": "192.168.2.1",
"family": "ipv4",
"mask": 24,
"gateway": "192.168.0.1"
},
{
"family": "ipv6",
"mask": 64,
"proto": "static",
"address": "fe80::1e3b:f3ff:fe10:a42"
}
],
"bridge_members": [
"eth0.1"
],
"mtu": 1500
},
{
"mac": "1c:3b:f3:10:0a:42",
"type": "wireless",
"up": true,
"txqueuelen": 1000,
"name": "wlan0",
"wireless": {
"country": "00",
"noise": 0,
"ssid": "TP-Link",
"channel": 2,
"tx_power": 20,
"mode": "station",
"signal": -46,
"frequency": 2417
},
"multicast": true,
"addresses": [
{
"proto": "dhcp",
"address": "192.168.0.100",
"family": "ipv4",
"mask": 24,
"gateway": "192.168.0.1"
},
{
"family": "ipv6",
"mask": 64,
"proto": "static",
"address": "fe80::1e3b:f3ff:fe10:a42"
}
],
"mtu": 1500
},
{
"mac": "1c:3b:f3:10:0a:42",
"type": "other",
"up": true,
"txqueuelen": 1000,
"name": "eth0",
"multicast": true,
"addresses": [
{
"family": "ipv6",
"mask": 64,
"proto": "static",
"address": "fe80::1e3b:f3ff:fe10:a42"
}
],
"mtu": 1500
},
{
"mac": "1c:3b:f3:10:0a:42",
"type": "vlan",
"up": true,
"txqueuelen": 1000,
"name": "eth0.1",
"multicast": true,
"mtu": 1500
}
],
"resources": {
"memory": {
"total": 61452288,
"shared": 278528,
"free": 33243136,
"cached": 7983104,
"available": 26767360,
"buffered": 2465792
},
"cpus": 1,
"disk": [
{
"filesystem": "/dev/root",
"available_bytes": 0,
"mount_point": "/rom",
"used_percent": 100,
"size_bytes": 2621440,
"used_bytes": 2621440
},
{
"filesystem": "/dev/mtdblock5",
"available_bytes": 192512,
"mount_point": "/overlay",
"used_percent": 95,
"size_bytes": 3735552,
"used_bytes": 3543040
}
],
"load": [
0.33,
0.27,
0.11
],
"swap": {
"free": 0,
"total": 0
}
},
"dns_servers": [
"8.8.8.8",
"8.8.4.4"
],
"neighbors": [
{
"mac": "04:0e:3c:ca:55:5f",
"state": "REACHABLE",
"interface": "br-lan",
"ip": "192.168.2.140"
},
{
"mac": "84:d8:1b:62:a3:55",
"state": "REACHABLE",
"interface": "wlan0",
"ip": "192.168.0.1"
},
{
"mac": "04:0e:3c:ca:55:5f",
"state": "STALE",
"interface": "br-lan",
"ip": "fe80::2ed4:5d74:941f:ce1e"
}
]
}output from netengine
{
"type": "DeviceMonitoring",
"general": {
"uptime": 30,
"local_time": 1623319570
},
"resources": {
"cpus": 0,
"memory": {
"total": 60012,
"shared": 88,
"free": 27340,
"cached": 7160
},
"swap": {
"total": 0,
"free": 0
}
},
"interfaces": [
{
"name": "lo",
"statistics": {
"mac": "",
"type": "softwareLoopback",
"up": true,
"rx_bytes": 2437,
"tx_bytes": 2437,
"mtu": 65536
}
},
{
"name": "eth0",
"statistics": {
"mac": "1c:3b:c3:b3:10:0a",
"type": "ethernetCsmacd",
"up": true,
"rx_bytes": 160673,
"tx_bytes": 615795,
"mtu": 1500
}
},
{
"name": "Device 14c3:7662",
"statistics": {
"mac": "",
"type": "ethernetCsmacd",
"up": false,
"rx_bytes": 0,
"tx_bytes": 0,
"mtu": 1500
}
},
{
"name": "br-lan",
"statistics": {
"mac": "1c:3b:c3:b3:10:0a",
"type": "ethernetCsmacd",
"up": true,
"rx_bytes": 147924,
"tx_bytes": 601205,
"mtu": 1500
}
},
{
"name": "eth0.1",
"statistics": {
"mac": "1c:3b:c3:b3:10:0a",
"type": "ethernetCsmacd",
"up": true,
"rx_bytes": 147924,
"tx_bytes": 614999,
"mtu": 1500
}
},
{
"name": "wlan0",
"statistics": {
"mac": "1c:3b:c3:b3:10:0a",
"type": "ethernetCsmacd",
"up": true,
"rx_bytes": 604709,
"tx_bytes": 158937,
"mtu": 1500
}
}
],
"neighbors": [
{
"mac": "04:0e:3c:c3:8a:55",
"state": "REACHABLE",
"interface": "br-lan"
},
{
"mac": "04:0e:3c:c3:8a:55",
"state": "REACHABLE",
"interface": "br-lan"
},
{
"mac": "c2:84:c3:98:1b:62",
"state": "STALE",
"interface": "wlan0"
}
]
} |
701f1df to
b8c427f
Compare
b8c427f to
cfa6650
Compare
- Fix failing tests - Pass more realistic values in mocks - Remove redundant code
Pull Request Test Coverage Report for Build 938832440Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
01de297 to
8cd7089
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I got following errors while running tests again OpenWrt device. Is there anything that needs to be configured on the device? Is there any extra snmpd cofnigguration needed?
(venv-netengine) pandafy@FRIDAY:~/openwisp/netengine$ DISABLE_MOCKS=1 TEST_SETTINGS_FILE=./test-settings.json nose2 tests.test_snmp.test_openwrt
........EE....E.......E..
======================================================================
ERROR: test_interfaces_to_dict (tests.test_snmp.test_openwrt.TestSNMPOpenWRT)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pandafy/openwisp/netengine/tests/test_snmp/test_openwrt.py", line 80, in test_interfaces_to_dict
self.assertIsInstance(self.device.interfaces_to_dict, list)
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 335, in interfaces_to_dict
if_type = self.interfaces_type[i]['type']
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 281, in interfaces_type
'type': types[self.get_value(types_oid + str(i))],
KeyError: '1'
======================================================================
ERROR: test_interfaces_type (tests.test_snmp.test_openwrt.TestSNMPOpenWRT)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pandafy/openwisp/netengine/tests/test_snmp/test_openwrt.py", line 71, in test_interfaces_type
self.assertIsInstance(self.device.interfaces_type, list)
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 281, in interfaces_type
'type': types[self.get_value(types_oid + str(i))],
KeyError: '1'
======================================================================
ERROR: test_netjson_compliance (tests.test_snmp.test_openwrt.TestSNMPOpenWRT)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pandafy/openwisp/netengine/tests/test_snmp/test_openwrt.py", line 120, in test_netjson_compliance
device_dict = self.device.to_dict()
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 532, in to_dict
'interfaces': self.interfaces_to_dict,
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 335, in interfaces_to_dict
if_type = self.interfaces_type[i]['type']
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 281, in interfaces_type
'type': types[self.get_value(types_oid + str(i))],
KeyError: '1'
======================================================================
ERROR: test_to_dict (tests.test_snmp.test_openwrt.TestSNMPOpenWRT)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pandafy/openwisp/netengine/tests/test_snmp/test_openwrt.py", line 113, in test_to_dict
device_dict = self.device.to_dict()
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 532, in to_dict
'interfaces': self.interfaces_to_dict,
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 335, in interfaces_to_dict
if_type = self.interfaces_type[i]['type']
File "/home/pandafy/openwisp/netengine/netengine/backends/snmp/openwrt.py", line 281, in interfaces_type
'type': types[self.get_value(types_oid + str(i))],
KeyError: '1'
----------------------------------------------------------------------
Ran 25 tests in 23.801s
FAILED (errors=4)
- Swap time with datetime in local_time - Remove unused warning - Use a single call for collecting load information
0b12396 to
da39965
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Great work Purhan! Please see my comments below.
b5940b2 to
cda0295
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
While testing it was found that to_dict methods are taking too long to fetch the data since it is making multiple queries. To make this method quicker, we can do a single query to device for a complete SNMP Walk and then process the data as needed locally by netengine.
I have been thinking about how we can bring this change and here is one of my idea.
Instead of changing complete implementation we can update the signature of current methods to accept another optional parameter snmpdump which will be the output from snmpwalk. The methods will locally fetch the required OIDs from snmpdump and then the existing code for further processing can be used.
Here's a pseudocode for example:
def local_time(self, snmpdump=None):
if snmpdump:
epoch = get_local_time_from_snmp_dump
else:
epoch = str(self.get('1.3.6.1.4.1.41112.1.4.8.1.0')[3][0][1])
timestamp = int(datetime.strptime(epoch, '%Y-%m-%d %H:%M:%S').timestamp())
return timestamp
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @purhan, with that fix I can now extract the info without errors.
For sure we'll have find a way to get all the info at once to resolve the performance issue when generating the JSON output, in the current form it would not be usable.
In the output of type I see ethernet Csmacd, but to be compliant with the NetJSON spec it should be just ethernet.
One thing I noticed is that wifi interfaces are also detected as ethernet and I don't see any wireless info is that the max we can do?
Looking at the snmpd package for OpenWRT, I do see a patch that seems to add support for WiFi information: https://github.com/openwrt/packages/blob/38f01ad2c9738c6c5a5dbda85019bb9cf5bc10f8/net/net-snmp/patches/750-ieee802dot11.patch.
I just asked on the OpenWRT forum if the know anything about it.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netengine/backends/snmp/airos.py`:
- Around line 473-477: Update to_dict in netengine/backends/snmp/airos.py at
lines 473-477 to default autowalk to None and perform walks for 1.3.6 and
1.2.840.10036 only when snmpdump is None. Apply the same change to to_dict in
netengine/backends/snmp/openwrt.py at lines 565-583, using walks for 1.3.6.1 and
1.2.840.10036; preserve caller-supplied dumps without issuing live SNMP
requests.
- Around line 297-324: Update interfaces_to_dict to skip entries whose interface
name is empty, immediately after retrieving name from interfaces_MAC and before
collecting or appending the remaining interface data. Preserve the existing
processing for named interfaces so the returned results contain no records with
an empty "name".
In `@netengine/backends/snmp/openwrt.py`:
- Around line 529-552: Update the neighbor processing around the neighbors and
neighbor_states collections to key state entries by their shared
ipNetToPhysicalTable OID suffix rather than list position. Build a lookup from
each neighbor-state row’s suffix, then retrieve the matching state while
processing each neighbor; preserve the existing UNKNOWN fallback and avoid
dropping valid trailing neighbors when a state row is missing.
In `@tests/static/test-openwrt-snmp-oid.json`:
- Around line 69-72: Update the DateAndTime fixture value in the JSON entry for
OID 1.3.6.1.2.1.25.1.2.0 so the seconds byte uses the doubled-backslash escaped
form expected by codecs.escape_decode, matching the representation used by
test_local_time_offset and the rest of the fixture without embedding a literal
carriage-return character.
In `@tests/test_snmp/test_airos.py`:
- Around line 43-46: Update test_get_value_error to avoid stopping
getcmd_patcher and issuing a live SNMP request; verify whether get_value(".")
can deterministically produce NoSuchObjectError without network I/O, and if not,
mock get_cmd with the same deterministic return-value pattern used by
test_validate_negative_result while preserving the expected exception assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f9b0c6b-05da-40bb-ba7f-c37463205af0
📒 Files selected for processing (19)
.github/workflows/pypi.ymldocs/source/index.rstdocs/source/topics/backends/snmp.rstdocs/source/topics/snmp.rstnetengine/backends/base.pynetengine/backends/dummy.pynetengine/backends/schema.pynetengine/backends/snmp/airos.pynetengine/backends/snmp/base.pynetengine/backends/snmp/openwrt.pyrequirements-test.txttests/static/test-airos-snmp.jsontests/static/test-openwrt-snmp-oid.jsontests/test_base.pytests/test_dummy.pytests/test_snmp/test_airos.pytests/test_snmp/test_base.pytests/test_snmp/test_openwrt.pytests/utils.py
💤 Files with no reviewable changes (1)
- docs/source/topics/snmp.rst
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
netengine/backends/base.py
[info] 29-29: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dictionary, **kwargs)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netengine/backends/schema.py
[warning] 3-3: Do not make http calls without encryption
Context: "http://json-schema.org/draft-07/schema#"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 zizmor (1.28.0)
.github/workflows/pypi.yml
[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (23)
netengine/backends/base.py (1)
22-30: LGTM!netengine/backends/dummy.py (1)
61-61: LGTM!netengine/backends/snmp/base.py (2)
1-7: LGTM!Also applies to: 19-22, 33-56, 74-98, 113-119, 138-154
100-111: LGTM!Also applies to: 121-136
netengine/backends/schema.py (1)
1-154: LGTM!netengine/backends/snmp/airos.py (3)
19-29: LGTM!Also applies to: 31-49, 51-62, 64-99, 103-123, 127-155, 166-179, 190-207, 217-237, 240-269, 271-293
326-340: LGTM!Also applies to: 377-388, 390-438, 440-458, 460-471
478-497: LGTM!netengine/backends/snmp/openwrt.py (2)
6-64: LGTM!Also applies to: 69-132, 136-156, 160-204, 206-264, 268-293, 297-330, 332-375
377-508: LGTM!.github/workflows/pypi.yml (1)
20-32: Unpinned action references and missingpersist-credentials: falseremain unresolved.Lines 20, 22, and 32 still use mutable tags (
@v7,@v7,@v1.14.1) instead of immutable commit SHAs, while the job grantsid-token: write(lines 17-18). Theactions/checkoutstep at line 20 still does not setpersist-credentials: false. Both concerns were raised in a previous review round on this file and remain unaddressed in this reformatted version.Source: Linters/SAST tools
docs/source/topics/backends/snmp.rst (2)
40-42: 📐 Maintainability & Code Quality | ⚡ Quick winTerminology: "firmwares" should be "backends".
Line 40 still calls AirOS and OpenWRT "firmwares." A previous reviewer already flagged this exact wording ("it's not a firmware, it's a backend") elsewhere in this document's history, and the concern is still open here.
76-83: 📐 Maintainability & Code QualityNo change needed.
Indented lines directly after
::are recognized as an RST literal block without a preceding blank line; add blank lines for readability, not correctness.> Likely an incorrect or invalid review comment.docs/source/index.rst (1)
67-72: LGTM!requirements-test.txt (1)
3-3: LGTM!tests/test_dummy.py (1)
26-29: LGTM!tests/static/test-airos-snmp.json (1)
11-34: LGTM!Also applies to: 64-73
tests/test_base.py (1)
3-8: LGTM!Also applies to: 93-106
tests/test_snmp/test_airos.py (1)
48-259: LGTM!tests/test_snmp/test_base.py (1)
4-18: LGTM!Also applies to: 46-66
tests/test_snmp/test_openwrt.py (1)
46-334: LGTM!tests/utils.py (2)
11-97: LGTM!
98-101: 📐 Maintainability & Code QualityNo leftover references remain.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netengine/backends/snmp/airos.py`:
- Around line 475-477: Update the to_dict method signatures in
netengine/backends/snmp/airos.py lines 475-477 and
netengine/backends/snmp/openwrt.py lines 572-574 so autowalk remains the first
positional parameter and snmpdump follows it, preserving existing to_dict(False)
calls while retaining snmpdump support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b36d787-7881-4f30-bd64-da66cf3c5d8c
📒 Files selected for processing (4)
netengine/backends/snmp/airos.pynetengine/backends/snmp/openwrt.pytests/test_snmp/test_airos.pytests/test_snmp/test_openwrt.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Kilo Code Review
🔇 Additional comments (4)
netengine/backends/snmp/airos.py (1)
19-473: LGTM!netengine/backends/snmp/openwrt.py (1)
24-570: LGTM!tests/test_snmp/test_airos.py (1)
109-113: LGTM!Also applies to: 148-185, 187-266
tests/test_snmp/test_openwrt.py (1)
89-120: LGTM!Also applies to: 131-165, 191-265, 267-366
# Conflicts: # .github/workflows/pypi.yml
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netengine/backends/snmp/airos.py (1)
86-99: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftScope all memoized SNMP results by
snmpdump.Both backends accept snapshot data but cache results without recording the snapshot. A later call with a different dump can return stale data. Resetting caches inside
to_dictdoes not protect direct accessor calls.
netengine/backends/snmp/airos.py#L86-L99: scope_interfacesand the other AirOS memoized accessors bysnmpdump, or bypass caching for supplied dumps.netengine/backends/snmp/openwrt.py#L69-L83: apply the same rule to_interfaces,_interfaces_MAC,_interfaces_mtu,_interfaces_up,_interfaces_bytes,_interfaces_type,_wireless_interfaces, and_interface_addr_and_mask.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/airos.py` around lines 86 - 99, Scope all memoized SNMP accessor results by the supplied snmpdump, or bypass caching whenever a dump is provided, so direct calls cannot return data from another snapshot. In netengine/backends/snmp/airos.py lines 86-99, update get_interfaces and the other AirOS memoized accessors; in netengine/backends/snmp/openwrt.py lines 69-83, apply the same handling to _interfaces, _interfaces_MAC, _interfaces_mtu, _interfaces_up, _interfaces_bytes, _interfaces_type, _wireless_interfaces, and _interface_addr_and_mask.netengine/backends/snmp/openwrt.py (2)
306-326: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKey interface addresses by SNMP index and preserve all records.
Line [321] stores one address under the interface name. A second address for the same interface overwrites the first. The fixture already contains three interfaces named
"Device 8086:100e"intests/test_snmp/test_openwrt.pylines [68]-[77]. Line [354] then retrieves one value and wraps it in a one-element list, so duplicate names can receive the same final address.Build an ifIndex-keyed address map and retain a list of addresses per index. Aggregate each interface by that same ifIndex. Add a regression test for duplicate names and multiple addresses.
Also applies to: 332-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/openwrt.py` around lines 306 - 326, Update the OpenWrt SNMP address aggregation around the IPv4 loop and the retrieval logic near the interface aggregation to key addresses by ifIndex rather than interface name, preserving a list of every address and mask for each index. Aggregate interfaces using the same ifIndex so duplicate names remain distinct and do not overwrite or reuse the final address. Add a regression test covering duplicate interface names with multiple addresses.
572-576: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the OpenWRT autowalk of
1.2.840.10036best-effort.
to_dict()callsSNMP.walk("1.2.840.10036")beforeget_wireless_interfaces(), butSNMP.walkraisesNetEngineErrorwhennext()returns a non-zero SNMP error status. On OpenWRT devices that handle unsupported wireless MIB queries with an SNMP error, default serialization fails before the per-objectNetEngineErrorfallback can skip wireless OID records. Catch this autowalk with one log entry, or make the backend ignoreNetEngineErrorfrom this second walk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/openwrt.py` around lines 572 - 576, Update OpenWRTBackend.to_dict around the secondary walk for “1.2.840.10036” so NetEngineError is handled as a best-effort failure rather than aborting serialization. Keep the primary “1.3.6.1” walk behavior unchanged, and emit at most one log entry when the wireless walk fails before continuing with the existing snmpdump processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pypi.yml:
- Line 32: Update the uses reference for pypa/gh-action-pypi-publish to the full
immutable commit SHA corresponding to v1.14.2, retaining “# v1.14.2” as the
version comment.
---
Outside diff comments:
In `@netengine/backends/snmp/airos.py`:
- Around line 86-99: Scope all memoized SNMP accessor results by the supplied
snmpdump, or bypass caching whenever a dump is provided, so direct calls cannot
return data from another snapshot. In netengine/backends/snmp/airos.py lines
86-99, update get_interfaces and the other AirOS memoized accessors; in
netengine/backends/snmp/openwrt.py lines 69-83, apply the same handling to
_interfaces, _interfaces_MAC, _interfaces_mtu, _interfaces_up,
_interfaces_bytes, _interfaces_type, _wireless_interfaces, and
_interface_addr_and_mask.
In `@netengine/backends/snmp/openwrt.py`:
- Around line 306-326: Update the OpenWrt SNMP address aggregation around the
IPv4 loop and the retrieval logic near the interface aggregation to key
addresses by ifIndex rather than interface name, preserving a list of every
address and mask for each index. Aggregate interfaces using the same ifIndex so
duplicate names remain distinct and do not overwrite or reuse the final address.
Add a regression test covering duplicate interface names with multiple
addresses.
- Around line 572-576: Update OpenWRTBackend.to_dict around the secondary walk
for “1.2.840.10036” so NetEngineError is handled as a best-effort failure rather
than aborting serialization. Keep the primary “1.3.6.1” walk behavior unchanged,
and emit at most one log entry when the wireless walk fails before continuing
with the existing snmpdump processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c30208a7-37ef-4d7a-bd1a-172422cbe8dc
📒 Files selected for processing (5)
.github/workflows/pypi.ymlnetengine/backends/snmp/airos.pynetengine/backends/snmp/openwrt.pytests/test_snmp/test_airos.pytests/test_snmp/test_openwrt.py
📜 Review details
🧰 Additional context used
🪛 zizmor (1.28.0)
.github/workflows/pypi.yml
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (6)
netengine/backends/snmp/airos.py (2)
6-9: LGTM!Also applies to: 19-55, 64-82
379-499: LGTM!netengine/backends/snmp/openwrt.py (2)
24-64: LGTM!
377-570: LGTM!tests/test_snmp/test_airos.py (1)
1-14: LGTM!Also applies to: 31-31, 41-55, 73-147, 149-267
tests/test_snmp/test_openwrt.py (1)
1-13: LGTM!Also applies to: 34-34, 44-293, 295-326, 350-366
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netengine/backends/snmp/airos.py (2)
490-492: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTolerate a failing vendor walk, as the OpenWRT backend does.
walkraisesNetEngineErrorwhen the agent reports an error indication or error status. If a device does not expose the1.2.840.10036subtree,to_dictfails completely, and the general, resource, and interface data is lost.netengine/backends/snmp/openwrt.pylines 592-597 already log a warning and continue in this case. Align the two backends.♻️ Proposed change
if snmpdump is None and autowalk: snmpdump = self.walk("1.3.6") - snmpdump.update(self.walk("1.2.840.10036")) + try: + snmpdump.update(self.walk("1.2.840.10036")) + except NetEngineError as exc: + logger.warning("Unable to collect optional vendor SNMP data: %s", exc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/airos.py` around lines 490 - 492, Update the autowalk branch in AirosBackend.to_dict to catch NetEngineError from the vendor-specific self.walk("1.2.840.10036") call, log a warning consistent with OpenWRT’s handling, and continue using the general walk results so device data is still returned. Keep failures from the primary self.walk("1.3.6") path unchanged.
438-448: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFix
CPU_count’s CPU-entry formula.
loadNumber == 3and the fixture has only the three load-average OIDs (1.3.6.1.4.1.10002.1.1.1.4.2.1.3.*), soCPU_count()returns0. Load entries and core identifiers are separate OIDs; returning0fromresources_to_dict()makes this output assert incorrectly when cores exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/airos.py` around lines 438 - 448, Update CPU_count to derive the CPU core count from the load-entry data without dividing by loadNumber or assuming each entry contains three fields. Use the core/load index OIDs returned by self.next, ensuring fixtures with three load-average entries and loadNumber equal to 3 return the existing core count rather than zero.netengine/backends/snmp/openwrt.py (1)
312-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
_interface_dictstate.
_interface_dictis only written and reset innetengine/backends/snmp/openwrt.pyand asserted intests/test_snmp/test_openwrt.py; no code reads it. Afterinterfaces_to_dictkeys addresses by SNMP index, drop the attribute initialization, loop, reset, and matching test assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netengine/backends/snmp/openwrt.py` around lines 312 - 313, Remove the unused _interface_dict state from the OpenWrt SNMP implementation: delete its initialization, the loop assigning interface names by index, and any reset or cleanup references. Remove the corresponding assertion from the OpenWrt SNMP tests, while preserving interfaces_to_dict behavior and its SNMP-index addressing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_snmp/test_openwrt.py`:
- Around line 239-281: Flatten the nested patch.object blocks in the test around
interfaces_to_dict by importing and using contextlib.ExitStack; register the
same nine patches with the stack and keep the call to
self.device.interfaces_to_dict() at the reduced indentation while preserving all
mocked return values.
---
Outside diff comments:
In `@netengine/backends/snmp/airos.py`:
- Around line 490-492: Update the autowalk branch in AirosBackend.to_dict to
catch NetEngineError from the vendor-specific self.walk("1.2.840.10036") call,
log a warning consistent with OpenWRT’s handling, and continue using the general
walk results so device data is still returned. Keep failures from the primary
self.walk("1.3.6") path unchanged.
- Around line 438-448: Update CPU_count to derive the CPU core count from the
load-entry data without dividing by loadNumber or assuming each entry contains
three fields. Use the core/load index OIDs returned by self.next, ensuring
fixtures with three load-average entries and loadNumber equal to 3 return the
existing core count rather than zero.
In `@netengine/backends/snmp/openwrt.py`:
- Around line 312-313: Remove the unused _interface_dict state from the OpenWrt
SNMP implementation: delete its initialization, the loop assigning interface
names by index, and any reset or cleanup references. Remove the corresponding
assertion from the OpenWrt SNMP tests, while preserving interfaces_to_dict
behavior and its SNMP-index addressing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26952c8c-ff75-4073-98f6-97d5179c50e6
📒 Files selected for processing (5)
.github/workflows/pypi.ymlnetengine/backends/snmp/airos.pynetengine/backends/snmp/openwrt.pytests/test_snmp/test_airos.pytests/test_snmp/test_openwrt.py
📜 Review details
🔇 Additional comments (5)
netengine/backends/snmp/airos.py (1)
88-88: LGTM!Also applies to: 131-131, 171-171, 196-196, 224-224, 250-250, 280-280, 483-486
netengine/backends/snmp/openwrt.py (1)
71-71: LGTM!Also applies to: 115-115, 141-141, 166-166, 215-215, 248-248, 277-277, 523-527, 590-597
tests/test_snmp/test_airos.py (1)
88-101: LGTM!Also applies to: 156-156
tests/test_snmp/test_openwrt.py (1)
80-93: LGTM!Also applies to: 184-229, 413-413, 465-486
.github/workflows/pypi.yml (1)
32-32: 🔒 Security & PrivacyNo change needed.
The
v1.14.2dereferenced tag commit matches the pinned SHA.
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
Proposed change log entry: |
4 participants
Closes #2, closes #64, closes #65
Resolves part of #60