feat: parent child interfaces - #87
Conversation
📝 WalkthroughWalkthroughThe PR adds configurable LibreNMS port-stack LAG patterns, vendor-aware relationship resolution, stable port-ID synchronization, relationship controls, permission-scoped operations, row locking, and extensive regression coverage. ChangesPort-stack pattern management
Relationship resolution and synchronization
Permission and concurrency controls
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🔵 Low · up to Malformed cached VLAN override IDs could prevent the interfaces table from rendering; the PR is otherwise mergeable with owner awareness and follow-up to validate IDs before lookup. Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@netbox_librenms_plugin/librenms_api.py`:
- Around line 413-414: The call returning (True, data.get("mappings", []))
should validate that "mappings" is a list of dicts before reporting success; in
librenms_api.py replace the raw return in the function that parses the response
with logic that: ensure data.get("mappings") is a list, filter out any non-dict
entries (e.g., [m for m in mappings if isinstance(m, dict)]), and if the result
is missing/empty use an empty list and still return success=False or True as
appropriate for your function (prefer True with an empty list to match
get_device_inventory()/get_inventory_filtered() behavior); update the code paths
that call resolve_port_relationships() to rely on the validated list so callers
that call .get() on each item won't hit AttributeError/TypeError.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1918-1938: The fetch response is parsed unconditionally; update
the fetch chain inside the existing fetch(url, { ... }) call so you first check
the Response.ok (the returned r object) before calling r.json() — if !r.ok
attempt to parse JSON or text to pull an error message and treat it as an error
branch (set btn.disabled = false, btn.innerHTML to the alert icon and btn.title
to the backend error or status text); if r.ok continue to parse r.json() and
handle data.status === 'success' as now. Also enhance the .catch handler to
include the caught error.message in btn.title instead of the generic 'Request
failed'. Reference the existing variables btn, url, csrf, body and the current
then/catch chain when making the change.
- Around line 346-365: The delegated change handler that auto-selects LAG
members sets memberCheckbox.checked but doesn't trigger those checkboxes' own
change listeners, so bulk-action state isn't recomputed; inside the loop in the
change handler (the document.addEventListener('change', ...) block that checks
'input[name="select"]' and the 'autoSelectLagMembers' toggle and iterates
`document.querySelectorAll('tr[data-member-of-lag="' + portId + '"]')`), after
setting `memberCheckbox.checked = checkbox.checked`, dispatch a change event on
the memberCheckbox (e.g. memberCheckbox.dispatchEvent(new Event('change', {
bubbles: true }))) so those checkboxes' listeners run and the global bulk-action
state is updated.
In `@netbox_librenms_plugin/tests/test_coverage_actions.py`:
- Around line 4243-4319: Add thin view-level regression tests that exercise
AddAsOOBView and PromoteToHostView end-to-end rather than only calling
set_librenms_oob: craft a request/POST that goes through the view code paths
which would pass oob_type="oob" (simulate the same MagicMock device/CF data used
in current tests), call AddAsOOBView and PromoteToHostView (or use the test
client to POST to their endpoints) and assert the HTTP response is not 400 and
that the device custom_field_data was updated (via get_librenms_oob or
inspecting obj.custom_field_data) to contain type=="oob" or the saved oob id;
keep the existing util-level tests but add one view-level test per path
referencing AddAsOOBView, PromoteToHostView and set_librenms_oob so the
user-visible behavior is covered.
In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 1731-1781: The tests in TestGetPortStack rely on the pytest
fixture mock_librenms_api but that fixture isn't available at class scope; move
or register mock_librenms_api at module/collection scope so pytest can find it:
either place the mock_librenms_api fixture into your project's conftest.py
(preferred) or expose it via module-level pytest_plugins in the test module so
TestGetPortStack and its methods can use it; ensure the fixture name remains
mock_librenms_api and that tests like
TestGetPortStack::test_returns_mappings_list_on_success,
test_returns_false_on_404, and test_returns_empty_list_when_no_mappings_key
import/use the same fixture.
In `@netbox_librenms_plugin/urls.py`:
- Around line 936-940: The changelog route for PortStackLagPattern is missing
the model context: update the path that registers
PortStackLagPatternChangeLogView (name "portstacklagpattern_changelog") to pass
kwargs={"model": PortStackLagPattern} so the view receives the model like the
other *ChangeLogView routes; locate the path call for
PortStackLagPatternChangeLogView and add the kwargs parameter referencing the
PortStackLagPattern model.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 116-134: OOB links are appended using link.get("local_port")
directly which skips the configured interface_name_field resolution; update the
OOB branch (where get_librenms_oob(...) and
self.librenms_api.get_device_links(oob["id"]) are used) to fetch OOB ports via
self.librenms_api.get_ports(oob["id"]) and build an oob_local_ports_map keyed by
port id that resolves the display name based on the same interface_name_field
logic used for the main device (mirror how local_ports_map is built), then use
that map to populate "local_port" and "local_port_id" when appending to
links_data so OOB interface names respect the user's naming preference.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 436-462: The _has_lag_signals function currently re-queries
PortStackLagPattern.objects.all() and recompiles regexes on every call; change
it to reuse cached compiled patterns by retrieving a precompiled attribute if
present (e.g. PortStackLagPattern._compiled_pattern) or by compiling once and
storing it on the model instance (check for getattr(pat_obj,
"_compiled_pattern", None) and use that) so you avoid repeated DB reads and
regex.compile calls; alternatively move the pattern-compile logic into a small
helper (cached via functools.lru_cache or a module-level variable) that returns
compiled patterns from PortStackLagPattern.lag_name_pattern, and then have
_has_lag_signals iterate those compiled regexes instead of recompiling each
time.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 172-217: The update_fields branch in _save_device currently skips
model validation and only relies on DB errors; before calling
device.save(update_fields=update_fields) call device.full_clean(exclude=...) to
validate the updated fields only (build exclude as the set of model field names
not present in update_fields) and handle ValidationError the same way as the
other branch (use _err with the same message formatting). Keep existing
IntegrityError handling and HTMX-aware _err behavior; use the same
ValidationError/message_dict logic used when update_fields is None.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 512-546: _resolve_interface_by_port_id currently only searches
interfaces directly on the passed obj, so interfaces that were created on VC
member devices (via sync_interface using device_selection_*) are never found
when running sync from the VC master; update _resolve_interface_by_port_id to,
when obj is a VC master (or when a device selection is present), iterate the
actual selected member devices' Interface/VMInterface querysets in addition to
obj itself and attempt the same librenms_id/name_hint matching against each
member device's interfaces; ensure you reference the same get_librenms_device_id
matching logic and return the matching iface and None as before so the post-sync
LAG/parent update and single-item relationship endpoints succeed.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e79507fd-bbd2-4c3f-8fb9-b2ce8e5f66f4
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (65)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/migrations/0012_portstacklagpattern.pynetbox_librenms_plugin/migrations/0013_portstacklagpattern_data.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.py
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
eeb6bc2 to
4986913
Compare
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
f0f9eef to
8ccd827
Compare
7a2a5af to
432b7f8
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
432b7f8 to
7033069
Compare
The guard scanner required the stack element to equal the guard string
exactly, so a compound '{% if not migrated_to_marker and x %}' guard
was falsely reported unguarded. Match the guard as one conjunct of a
pure and-chain instead; any or-compound still fails closed (with and
binding tighter than or, an or operand can render the branch with the
marker set).
The LAG/parent endpoints gate on change_interface / change_vminterface, but NetBoxObjectPermissionMixin asks has_perm without an instance, so a CONSTRAINED grant clears the gate. The owner was then resolved with a raw get_object_or_404 and the interface index was built off the plain manager, so a user constrained to one site could post another site's device id and set lag/parent on interfaces it cannot see (both ends are written: the source FK, and the aggregate's type bump). Resolve the owner through restrict_object_or_404 and build the index through Interface/VMInterface .restrict(user, "change"), so an out-of-grant id 404s like a nonexistent one. The POST gate now also declares the view permission the owner lookup performs, so a missing read grant is a stated 403 rather than a puzzling 404; reaching these endpoints already requires the device sync tab, which needs view_device anyway. The direct post() drives in the tests now bind the request the way View.setup() does under dispatch, instead of leaving self.request unset.
The collision-banner test stubbed views.sync.interfaces.get_object_or_404, which the object-scoped resolution replaced with restrict_object_or_404.
_lock_selected_device_targets and _lock_relationship_scope re-locked the object's
own virtual chassis with a raw manager filter, which the raw-pk scan reported.
The id comes from obj, which the request already resolved through a scoped
queryset, so route both through relock_scoped_row: that states the provenance
rather than relying on how the expression is written.
Dropping of=("self",) changes no lock: neither queryset joins, so the row locked
is the same one.
…l contracts Three suites still described how the code used to behave. SingleInterfaceVerifyView resolves the row by the stable LibreNMS port_id and fails closed when the caller posts none, so two tests that posted only a name got a 404. Post the port_id the view asks for, and give the shared port fixture one. test_mixed_structural_and_name_signals_warn_when_os_is_unknown created the "ios" PortStackLagPattern that migration 0013 already seeds, so it hit the case insensitive uniqueness constraint. Take the seeded row instead. test_verify_response_does_not_expose_inaccessible_vc_members asserted that str(hidden_device.pk) was absent from the whole response body. That matches any rendered number, so the assertion tracked pk allocation rather than a leak: it failed when the hidden device happened to get pk 1500 and the snapshot rendered ifMtu 1500. Assert the pk cannot appear as a device reference instead. The test passed on the branches above only because pks landed differently there.
Two test-infrastructure gaps left this branch red at its own tip while every test passed in isolation. Redis is not rolled back between tests and primary keys are reused, so a value cached under one pk was read by the next test that drew the same pk. Every test now runs in a unique key prefix on a sibling Redis database, which also keeps the dev server's DEBUG-time cache.clear() out of the run. A test marked django_db(transaction=True) truncates every table, including the rows migration 0013 seeds. pytest-django sorts every ordinary database test ahead of every transactional one, so no test observes that flush inside the same run. The damage is carried ACROSS runs: the flush lands in the session teardown and leaves a reused database empty, so the next run starts with LAG pattern detection already disabled. _reseed_after_transactional_flush restores the rows after the database fixture finalizes, and _restore_migration_seeded_rows covers a run that begins against an already-empty reused database by seeding inside each test's own transaction, which is rolled back afterwards. The per-test restore gates on the django_db marker rather than on request.fixturenames. pytest-django's _django_db_marker requests _django_db_helper, never the public db or transactional_db fixture, so those names are absent from the closure and a fixturenames-only check is false for every marker-only test. The gate keeps the fixturenames branch as well, for tests that request db directly without the marker.
Both suites post server_key "stub" and both passed alone but failed in a full
run. test_interface_vlan_sync declares pytest_plugins at module level, which
registers the API-test helpers session-wide, and that plugin carries an autouse
fixture pinning get_plugin_config to a default-only server map. Every module
collected after it inherits the mock, so "stub" became an unknown explicit key,
LibreNMSAPI raised KeyError, build_librenms_api returned None and the view
failed closed with 400 before it reached the behaviour under test. The
concurrency thread returned for the same reason and never reached target
locking.
Each module now overrides the fixture by name, which is the pattern
test_device_fields_server_scoping already uses. Load the helper plugin
explicitly to reproduce the old failures:
pytest -p netbox_librenms_plugin.tests.test_librenms_api_helpers \
netbox_librenms_plugin/tests/test_verify_views.py \
netbox_librenms_plugin/tests/test_sync_interface_concurrency.py
Relationship resolution read port_id_high and port_id_low. A live LibreNMS
26.6.1 returns {id, device_id, high_ifIndex, high_port_id, low_ifIndex,
low_port_id, ifStackStatus}, so both lookups returned None, every entry was
skipped, and LAG membership and sub-interface parenting both resolved to an
empty map. Nothing was mis-paired: the loop never reached the pairing.
LibreNMS 24.07 renamed the old port_id_high and port_id_low columns to
high_ifIndex and low_ifIndex, because they held ifIndex values, and added
high_port_id and low_port_id carrying the resolved port ids. The endpoint
serves the rows verbatim, so the response uses the column names. The API docs
still describe the pre-2024 shape, which is where the wrong spelling came from.
high_port_id is the field to read here: this resolver keys by port_id from the
ports payload, so high_ifIndex would match only where an ifIndex happens to
equal some port's id.
Every fixture in the suite used the documented spelling, so production and its
tests agreed on a shape no server sends and the suite stayed green. The fixtures
now carry the real shape: with the wrong keys restored, 29 of 39 resolver tests
fail, against none before. One test pins a verbatim live entry, and an entry
carrying neither key is now logged rather than dropped in silence.
… 4.4 Two CI-only failures on every leg, plus two more on the 4.4.0 leg. The first two named `server_key = "stub"`. Only `default` is configured in `media/configuration.testing.py`, which is what CI runs, so the view refused the unconfigured server and returned before the behaviour under test. Both now use `default`. The 4.4.0 pair hit an upstream bug: `Interface.clean()` there compares `self.parent.device.virtual_chassis` with `self.parent.virtual_chassis`, and `Interface` has no `virtual_chassis` attribute (4.6 compares `self.device.virtual_chassis`). So the validation NetBox means to run raises AttributeError, and a parent on another chassis member 500s instead of being accepted. `min_version` is 4.4.0 and that leg gates, so `_validate_relationship` tolerates exactly that failure: the parent edge, `exc.name == "virtual_chassis"`, and both interfaces on members of one chassis, which is the case the comparison exists to allow. Anything else propagates.
Reuse the concurrent platform winner directly instead of re-reading it through a
view-restricted queryset. The branch runs only when no platform existed at
preflight, so the gate asked for ("add", Platform) and never ("view", Platform);
restrict() then returns none() for an add-only user and aborts an assign they
were authorized to perform.
Handle IntegrityError at the outer transaction in the interface sync. The
relationship pass catches it around an inner savepoint, but Postgres validates
Django's DEFERRABLE INITIALLY DEFERRED foreign keys at the outermost COMMIT, so
a concurrently deleted related row escaped that handler and returned HTTP 500.
The inner comment claimed to cover this case and no longer does.
Coerce the cached VLAN id once and drop the entry when it is not numeric. The
value comes from the LibreNMS payload, which is only checked for being a dict, so
a non-numeric VID raised ValueError and aborted a sync that had already applied
other rows.
Degrade the relationship cell when no owner resolves: reverse() with an empty
object_id raises NoReverseMatch and takes down the whole table render.
Bound the port-id regex at 19 digits, the width of a PostgreSQL bigint. An
oversized string was previously rejected only by CPython's int_max_str_digits
limit, which a host may raise or disable.
Extract _lock_mapping_in_scope() so the device-type and platform mapping views
share one scope-then-lock implementation; both copies had to stay identical for
the permission guarantee to hold.
Tests: each production change has a test that fails without it. Also cover the VM
routed sync pages (they inherit the same scoped get_object), add in-grant control
cases for the mapping-scope refusals, name the locked table in the OOB IP lock
assertion, seed a LAG pattern no migration row can match, pin the verify refusal
status, rename the fallback test to match what it asserts, and drop the worker
lock_timeout below the caller's future deadline in two "must not lock" tests.
…e class The interface table wrote the selection and name accessors into base_columns and the row-attribute map into _meta before calling super().__init__(). Both are class attributes: django-tables2 deep-copies type(self).base_columns only after __init__ runs, and reads _meta.row_attrs from the class. A table built with a non-default interface_name_field therefore retargeted the columns for every later table in the same worker process, across requests and users. The two columns are now copied, given their accessor and passed through extra_columns, which Table.__init__ applies to its own copy. The accessor has to be set before that call because BoundColumn.accessor is cached while the columns are bound, so a later assignment is read back as the column name and renders an empty cell. The row-attribute map is passed as row_attrs, which binds to the instance. Two tests fail against the previous code: the class keeps the leaked "ifName" accessor, and _meta.row_attrs is populated on the class.
|
Superseded by #157. GitHub closed this PR automatically when its base branch #157 has the identical head commit and content, based directly on |
Summary
Stacked on #115 — review only the delta over
feat/bulk-import.Parent / LAG / child interface relationships. Resolves LAG aggregate↔member and sub-interface parent↔child pairs from LibreNMS
port_stack(with a per-OSPortStackLagPatternfallback for name-based aggregates likePo1), renders a combined Parent/LAG column with a verify control, and bulk-syncs the relationships onto NetBox interfaces. Adds thePortStackLagPatternmodel + migration.Motivation / Problem
Feature. Reflect LibreNMS LAG/sub-interface topology on the NetBox side.
Scope of Change
How Was This Tested?
.Nsub-units, Nokia SAP skip), row-owner pinning, verify rollback; real Device/Interface rows.Risk Assessment
Writes
lag/parentFKs only for user-selected rows; links validated withfull_cleanbefore save.Backwards Compatibility
Summary by CodeRabbit
New Features
Bug Fixes