Skip to content

Trigger dependency sync#100

Open
benjofalanga wants to merge 4 commits into
OpensourceICTSolutions:mainfrom
benjofalanga:trigger-dependency-sync
Open

Trigger dependency sync#100
benjofalanga wants to merge 4 commits into
OpensourceICTSolutions:mainfrom
benjofalanga:trigger-dependency-sync

Conversation

@benjofalanga

Copy link
Copy Markdown

Summary

This adds optional Zabbix trigger dependency synchronization based on NetBox cabling.

When enabled, nbxSync can update trigger dependencies after a successful device host sync. The feature uses:

  • NetBox device roles to classify devices into dependency levels
  • NetBox interface cabling to find directly connected upstream/downstream devices
  • Configured Zabbix trigger descriptions to update the correct triggers

The default hierarchy is:

access point -> switch -> gateway/firewall

@bvbaekel

bvbaekel commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Hey @benjofalanga,

Thanks for putting this together; trigger dependency sync based on NetBox cabling is a genuinely useful feature and it's clear you've thought through the design (level ordering, managed vs. unmanaged dependency preservation, cross-server guard, idempotency check before calling trigger.update).

Appreciate the effort!

That said, I'd like to ask you to split this PR up before I can merge it. Right now it mixes the actual feature with a bunch of unrelated changes, which makes it hard for me to review confidently.

Please remove from this PR:

  1. The .gitignore file: this is actually dangerous: the pattern nbxsync/* would tell git to ignore the entire plugin source directory. This must not be merged.

  2. The nbxsync.egg-info/ directory : build artifacts should never be committed. If you want, open a separate small PR that adds nbxsync.egg-info/ (and *.egg-info/) to a proper .gitignore.

  3. All the prose/style rewrites across the docs and README: you've gone through README.md, docs/configuration.md, docs/index.md, docs/installation.md, docs/permissions.md, docs/setup.md, and docs/traffic_flows.md and changed things like "Netbox" -> "NetBox" casing, removed emoji from headings, rewrote sentences for brevity, changed the README introduction from bullet points with checkmarks to a plain list, etc. Some of these are fine on their own (the NetBox casing fix is correct), but they don't belong in a feature PR. They make the diff much larger than it needs to be and force me to carefully verify each doc hunk isn't sneaking in a semantic change alongside a copy-edit. Please put these in a separate "docs cleanup" PR. Furthermore, not every change is one I'd accept, so this might require some coordination before I will merge that PR.

What should stay:

What I consider the actual feature scope, so:

  • nbxsync/utils/trigger_dependency_sync.py: the new module
  • nbxsync/settings.py: the TriggerDependencyLevelConfig, TriggerDependencyConfig additions and the trigger_dependencies field on PluginSettingsModel
  • nbxsync/__init__.py: the default config addition for trigger_dependencies
  • nbxsync/jobs/synchost.py: the import and the call to sync_device_trigger_dependencies
  • nbxsync/tests/: the new test files and the synchost test additions
  • Doc changes that only document the trigger dependency feature (the new trigger_dependencies section in configuration.md, the feature bullet in index.md, the trigger deps note in setup.md)

A few code-level notes for the feature itself once it's isolated:

  • The default role list is very Meraki-centric (meraki gw, meraki-gw, meraki mx, meraki-mx, mx), I bet you're a Meraki-shop :). Since the levels are fully configurable, I'd prefer keeping the defaults generic and letting users add vendor-specific slugs themselves. The defaults should work as a starting point for any deployment, not assume a specific vendor.

  • On a broader design note: the current approach stores the trigger dependency configuration statically in PLUGINS_CONFIG, which means any change requires editing configuration.py and restarting NetBox (and the workers). I'd much prefer this to be backed by Django models so the configuration can be managed dynamically through the UI/API without a restart; the same way Zabbix Servers, Templates, and Host Groups already work in nbxSync. This is a more natural fit for something that operators will want to tune per-environment (adding roles, adjusting trigger descriptions) without a maintenance window. Not necessarily a blocker for a first iteration, but worth designing towards.

  • _sync_child_dependency opens a ZabbixConnection per child device. When a switch has 48 connected APs, that's 48 separate API sessions. Consider passing the API connection in, or grouping children by Zabbix server to reuse a single connection.

  • The get_connected_devices function calls device.interfaces.all() which fires a query per device. Combined with connected_endpoints on each interface, this could get chatty on larger deployments. I've seen issues related to scale and the workers, so this should be solid or people will run into issues using this.

  • In synchost.py line 41: pluginsettings.trigger_dependencies.enabled is checked here and again inside sync_device_trigger_dependencies. The outer check is sufficient: the inner one is redundant.

So in summary: please split the PR into

  • (1) the trigger dependency feature only, and
  • (2) a separate docs/style cleanup PR. Once the feature PR is clean I'm happy to do a proper line-by-line and get it merged.

Let me know if you're willing to do this; if not I would like to ask if I may reuse your code and work on this?

Thanks again for the contribution!

@benjofalanga

Copy link
Copy Markdown
Author

Hey @benjofalanga,

Thanks for putting this together; trigger dependency sync based on NetBox cabling is a genuinely useful feature and it's clear you've thought through the design (level ordering, managed vs. unmanaged dependency preservation, cross-server guard, idempotency check before calling trigger.update).

Appreciate the effort!

That said, I'd like to ask you to split this PR up before I can merge it. Right now it mixes the actual feature with a bunch of unrelated changes, which makes it hard for me to review confidently.

Please remove from this PR:

1. **The `.gitignore` file**: this is actually dangerous: the pattern `nbxsync/*` would tell git to ignore the entire plugin source directory. This must not be merged.

2. **The `nbxsync.egg-info/` directory** : build artifacts should never be committed. If you want, open a separate small PR that adds `nbxsync.egg-info/` (and `*.egg-info/`) to a proper `.gitignore`.

3. **All the prose/style rewrites across the docs and README**: you've gone through `README.md`, `docs/configuration.md`, `docs/index.md`, `docs/installation.md`, `docs/permissions.md`, `docs/setup.md`, and `docs/traffic_flows.md` and changed things like "Netbox" -> "NetBox" casing, removed emoji from headings, rewrote sentences for brevity, changed the README introduction from bullet points with checkmarks to a plain list, etc. Some of these are fine on their own (the NetBox casing fix is correct), but they don't belong in a feature PR. They make the diff much larger than it needs to be and force me to carefully verify each doc hunk isn't sneaking in a semantic change alongside a copy-edit. Please put these in a separate "docs cleanup" PR. Furthermore, not every change is one I'd accept, so this might require some coordination before I will merge that PR.

What should stay:

What I consider the actual feature scope, so:

* `nbxsync/utils/trigger_dependency_sync.py`: the new module

* `nbxsync/settings.py`: the `TriggerDependencyLevelConfig`, `TriggerDependencyConfig` additions and the `trigger_dependencies` field on `PluginSettingsModel`

* `nbxsync/__init__.py`: the default config addition for `trigger_dependencies`

* `nbxsync/jobs/synchost.py`: the import and the call to `sync_device_trigger_dependencies`

* `nbxsync/tests/`: the new test files and the synchost test additions

* Doc changes that _only_ document the trigger dependency feature (the new `trigger_dependencies` section in `configuration.md`, the feature bullet in `index.md`, the trigger deps note in `setup.md`)

A few code-level notes for the feature itself once it's isolated:

* The default role list is very Meraki-centric (`meraki gw`, `meraki-gw`, `meraki mx`, `meraki-mx`, `mx`), I bet you're a Meraki-shop :). Since the levels are fully configurable, I'd prefer keeping the defaults generic and letting users add vendor-specific slugs themselves. The defaults should work as a starting point for _any_ deployment, not assume a specific vendor.

* On a broader design note: the current approach stores the trigger dependency configuration statically in `PLUGINS_CONFIG`, which means any change requires editing `configuration.py` and restarting NetBox (and the workers). I'd much prefer this to be backed by Django models so the configuration can be managed dynamically through the UI/API without a restart; the same way Zabbix Servers, Templates, and Host Groups already work in nbxSync. This is a more natural fit for something that operators will want to tune per-environment (adding roles, adjusting trigger descriptions) without a maintenance window. Not necessarily a blocker for a first iteration, but worth designing towards.

* `_sync_child_dependency` opens a `ZabbixConnection` per child device. When a switch has 48 connected APs, that's 48 separate API sessions. Consider passing the API connection in, or grouping children by Zabbix server to reuse a single connection.

* The `get_connected_devices` function calls `device.interfaces.all()` which fires a query per device. Combined with `connected_endpoints` on each interface, this could get chatty on larger deployments. I've seen issues related to scale and the workers, so this should be solid or people will run into issues using this.

* In `synchost.py` line 41: `pluginsettings.trigger_dependencies.enabled` is checked here _and_ again inside `sync_device_trigger_dependencies`. The outer check is sufficient: the inner one is redundant.

So in summary: please split the PR into

* (1) the trigger dependency feature only, and

* (2) a separate docs/style cleanup PR. Once the feature PR is clean I'm happy to do a proper line-by-line and get it merged.

Let me know if you're willing to do this; if not I would like to ask if I may reuse your code and work on this?

Thanks again for the contribution!

Hey @bvbaekel ,

Thanks for the detailed review. Yes, I’m happy to split this up.

I’ll clean this PR so it only contains the trigger dependency feature, remove the .gitignore and egg-info artifacts, and revert the unrelated docs/style changes. I’ll keep only the docs that directly describe the new trigger_dependencies feature.

I’ll also adjust the default role list to be generic, remove the redundant enabled check, and look into reusing Zabbix connections plus optimizing the connected-device lookup.

After that I’ll push an updated branch for review. The docs/style cleanup can be handled separately if needed.

@benjofalanga

benjofalanga commented Jul 10, 2026

Copy link
Copy Markdown
Author

Hey @bvbaekel ,
I cleaned this PR up and kept it scoped to the trigger dependency feature only.

Scope cleanup

Removed from this PR:

  • .gitignore
  • nbxsync.egg-info/
  • unrelated README/docs prose and style cleanup

The PR now only includes the trigger dependency implementation, related tests, and the relevant trigger_dependencies documentation.

Code review feedback addressed

  • Replaced Meraki-specific default role names with generic defaults:
    • access point, access-point, ap
    • switch, sw
    • gateway, gw, firewall, router
  • Kept vendor-specific role names configurable through PLUGINS_CONFIG.
  • Added a TODO noting that this config may move to Django models later for UI/API management.
  • Refactored Zabbix connection handling so child dependency sync reuses one API connection per Zabbix server instead of opening one session per child device.
  • Optimized connected device lookup to avoid the previous per-interface connected_endpoints behavior.
  • Removed the redundant inner enabled check from sync_device_trigger_dependencies.

Validation

I tested the cleaned implementation with focused Django tests:

manage.py test nbxsync.tests.utils.test_trigger_dependency_sync nbxsync.tests.jobs.test_synchost nbxsync.tests.test_plugin_settings

Result:

Ran 30 tests
OK
System check identified no issues

I also ran the repo’s Ruff format check:

ruff format --check ./nbxsync

Result:

365 files already formatted

Manual behavior test

I restored a NetBox/Zabbix test dataset and enabled trigger dependencies locally.

For switch AMSMK-SW05-F05-IDF05A-STACK01:

  • Found 48 connected APs.
  • All 48 APs were on the same Zabbix server and had host IDs.
  • After aligning the local trigger description config with the test environment, syncing the switch updated all 48 AP trigger dependencies.
  • A second run reported all 48 as unchanged, confirming the idempotency check works.

Follow-up

I left the configuration in PLUGINS_CONFIG for this iteration to keep the PR focused. A future improvement would be moving trigger dependency configuration into Django models so operators can manage it through the NetBox UI/API without restarting NetBox/workers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants