Skip to content

20878 use database routing when running script #20879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

20878 use database routing when running script #20879

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
701561d
20878 use database routing when running script
arthanson Nov 25, 2025
6094aa8
update comments
arthanson Nov 25, 2025
5208e54
remove event_tracking processing if not commit
arthanson Dec 1, 2025
aec8e29
atomic lock on both default and potentially branch database
arthanson Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions netbox/extras/jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
import traceback
from contextlib import ExitStack

from django.db import transaction
from django.db import router, transaction
from django.utils.translation import gettext as _

from core.signals import clear_events
from dcim.models import Device
from extras.models import Script as ScriptModel
from netbox.context_managers import event_tracking
from netbox.jobs import JobRunner
from netbox.registry import registry
from utilities.exceptions import AbortScript, AbortTransaction
Expand Down Expand Up @@ -42,10 +44,18 @@ def run_script(self, script, request, data, commit):
# A script can modify multiple models so need to do an atomic lock on
# both the default database (for non ChangeLogged models) and potentially
# any other database (for ChangeLogged models)
with transaction.atomic():
script.output = script.run(data, commit)
if not commit:
raise AbortTransaction()
branch_db = router.db_for_write(Device)
with transaction.atomic(using='default'):
# If branch database is different from default, wrap in a second atomic transaction
if branch_db != 'default':
Copy link
Member

@jnovinger jnovinger Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this more generic, rather than talking specifically about a branch_db?

with transaction.atomic(using=branch_db):
script.output = script.run(data, commit)
if not commit:
raise AbortTransaction()
else:
script.output = script.run(data, commit)
if not commit:
raise AbortTransaction()
Copy link
Member

@jnovinger jnovinger Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, I don't think this guarantees true cross-connection atomicity, even though they are talking to the same DB, I'm pretty sure they are doing so via two separate connections.

As it stands, outside of exceptional circumstances, I think that this is pretty safe and handles the the failure cases (where something failure in either the outer atomic() block or the inner atomic() block). But, I think that potentially changes if anything is ever added after the script.run(...) invocation but before the end of the outer atomic() block. I think in that case, it's possible that the inner block can succeed and commit while the outer block can still fail.

Do you think it would be worthwhile to add a comment here making it clear that (outside of the not commit cases) no other logic should ever exist here (or should be very carefully thought out), after the script.run(...) invocation and before exiting the outer block?

except AbortTransaction:
script.log_info(message=_("Database changes have been reverted automatically."))
if script.failed:
Expand Down Expand Up @@ -108,14 +118,14 @@ def run(self, data, request=None, commit=True, **kwargs):
script.request = request
self.logger.debug(f"Request ID: {request.id if request else None}")

# Execute the script. If commit is True, wrap it with the event_tracking context manager to ensure we process
# change logging, event rules, etc.
if commit:
self.logger.info("Executing script (commit enabled)")
with ExitStack() as stack:
for request_processor in registry['request_processors']:
stack.enter_context(request_processor(request))
self.run_script(script, request, data, commit)
else:
self.logger.warning("Executing script (commit disabled)")

with ExitStack() as stack:
for request_processor in registry['request_processors']:
if not commit and request_processor is event_tracking:
continue
stack.enter_context(request_processor(request))
self.run_script(script, request, data, commit)
Loading