Skip to content

Closes 20817 fix datasource sync broken when cron is set #20837

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
ifoughal wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Closes 20817 fix datasource sync broken when cron is set #20837

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b4160ad
Feat: set status as editable field
ifoughal Nov 19, 2025
111aca1
Feat: added clean method to set data-source state to Ready or scheduled
ifoughal Nov 19, 2025
bfeba36
Feat: added status update during save method of DataSourceForm
ifoughal Nov 19, 2025
2e0ff04
Feat: added 2 states for DataSourceStatusChoices
ifoughal Nov 19, 2025
a49869a
Feat: removed QUEUED from ready for sync condition
ifoughal Nov 19, 2025
5b5b5c8
Revert "Feat: set status as editable field"
ifoughal Nov 19, 2025
e11508d
Fix: removed status update from the enqueue method
ifoughal Nov 20, 2025
71f707b
Feat: removed SCHEDULED choice due to redundency with sync interval
ifoughal Nov 20, 2025
da4c669
Feat: reworked status update logic
ifoughal Nov 20, 2025
57b47dc
style: use != instead of not in for single SYNCING check
ifoughal Nov 26, 2025
3016b1d
Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…
ifoughal Nov 26, 2025
e4b6140
revert: re-added queued status set for datasource object
ifoughal Nov 26, 2025
929d024
Merge branch 'closes-20817-Fix-datasource-sync-broken-when-cron-is-se…
ifoughal Nov 26, 2025
93e5f91
Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…
ifoughal Dec 1, 2025
09d1049
Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…
ifoughal Dec 5, 2025
77ee6ba
refactor: moved status update logic from clean() to save() method
ifoughal Dec 5, 2025
544c97d
XMerge branch 'closes-20817-Fix-datasource-sync-broken-when-cron-is-s…
ifoughal Dec 5, 2025
cf16a29
Style: removed comment
ifoughal Dec 5, 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
2 changes: 2 additions & 0 deletions netbox/core/choices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ class DataSourceStatusChoices(ChoiceSet):
SYNCING = 'syncing'
COMPLETED = 'completed'
FAILED = 'failed'
READY = 'ready'
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The PR description mentions introducing two new states: "Ready" and "Scheduled". However, only the READY state is added here. The SCHEDULED state is missing from the DataSourceStatusChoices. This means the logic described in the PR description for setting status to "Scheduled" when a user sets a sync interval cannot be implemented.

Copilot uses AI. Check for mistakes.

CHOICES = (
(NEW, _('New'), 'blue'),
(QUEUED, _('Queued'), 'orange'),
(SYNCING, _('Syncing'), 'cyan'),
(COMPLETED, _('Completed'), 'green'),
(FAILED, _('Failed'), 'red'),
(READY, _('Ready'), 'green'),
Copy link
Collaborator

@arthanson arthanson Dec 4, 2025

Choose a reason for hiding this comment

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

You say SCHEDULED status is added but there is no scheduled in the status or code anywhere? Still confused as you state "The two new states are READY & SCHEDULED."

Copy link
Contributor Author

@ifoughal ifoughal Dec 5, 2025

Choose a reason for hiding this comment

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

I ended up removing it, so there is only one state that has been added in the end, which is the READY state.

The reasoning behind was that scheduled would interfere with the post sync action trigger status (completed/failed/syncing). Therefore, I decided to keep the ready state only, which has the same effect as new, the only difference is that new is only applied when the object gets created, whereas ready is applied when there there is no effective sync interval.

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The READY status uses the same color ('green') as COMPLETED. This could be confusing for users since these statuses represent different states: COMPLETED means a sync operation finished successfully, while READY means the DataSource is ready to be synced but hasn't completed a sync yet. Consider using a different color like 'blue' or 'gray' for READY to better differentiate it visually from COMPLETED.

Suggested change
(READY, _('Ready'), 'green'),
(READY, _('Ready'), 'gray'),

Copilot uses AI. Check for mistakes.
)


Expand Down
13 changes: 12 additions & 1 deletion netbox/core/forms/model_forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from utilities.forms.fields import CommentField, JSONField
from utilities.forms.rendering import FieldSet
from utilities.forms.widgets import HTMXSelect
from core.choices import DataSourceStatusChoices

__all__ = (
'ConfigRevisionForm',
Expand Down Expand Up @@ -79,14 +80,24 @@ def __init__(self, *args, **kwargs):
if self.instance and self.instance.parameters:
self.fields[field_name].initial = self.instance.parameters.get(name)

def save(self, *args, **kwargs):
def clean(self):
super().clean()
if not self.instance.pk:
self.cleaned_data['status'] = DataSourceStatusChoices.NEW
else:
if not self.data.get('sync_interval'):
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using self.data.get('sync_interval') directly accesses raw form data instead of the cleaned and validated data. This could lead to inconsistencies since self.data contains the raw POST/GET data (strings), while self.cleaned_data['sync_interval'] contains the properly validated and converted value. Use self.cleaned_data.get('sync_interval') instead for consistency and reliability.

Suggested change
if not self.data.get('sync_interval'):
if not self.cleaned_data.get('sync_interval'):

Copilot uses AI. Check for mistakes.
self.cleaned_data['status'] = DataSourceStatusChoices.READY
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This condition will always be False for updates because self.instance.pk will exist for existing instances. The logic appears to be checking if this is a new object, but the code only sets status to NEW when the instance doesn't have a pk. However, for creates, Django forms with ModelForm already have self.instance set with a pk=None before save. After clean() is called but before save(), the instance won't have a pk yet, so this condition should use self.instance.pk is None or check the _state.adding attribute. But more critically, this logic shouldn't override the status for updates when the DataSource already has a valid status from a sync operation (COMPLETED, FAILED, etc.).

Suggested change
if not self.instance.pk:
self.cleaned_data['status'] = DataSourceStatusChoices.NEW
else:
if not self.data.get('sync_interval'):
self.cleaned_data['status'] = DataSourceStatusChoices.READY
# Only set status to NEW if this is a new instance
if self.instance.pk is None:
self.cleaned_data['status'] = DataSourceStatusChoices.NEW

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The logic for setting status based on sync_interval is incomplete. According to the PR description, when a user sets a sync interval, the status should be set to "Scheduled". However, this code only handles the case when sync_interval is removed (setting to READY), but doesn't set the status to SCHEDULED when sync_interval is present. There's also no SCHEDULED state defined, so you should add an else branch to handle when sync_interval is set (e.g., set status to QUEUED or implement SCHEDULED).

Suggested change
else:
self.cleaned_data['status'] = DataSourceStatusChoices.QUEUED

Copilot uses AI. Check for mistakes.
def save(self, *args, **kwargs):
parameters = {}
for name in self.fields:
if name.startswith('backend_'):
parameters[name[8:]] = self.cleaned_data[name]
self.instance.parameters = parameters

# update status
self.instance.status = self.cleaned_data.get('status', self.instance.status)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This logic unconditionally overwrites the DataSource status from the form's cleaned_data. This will break the intended behavior where sync operations set the status to SYNCING, COMPLETED, or FAILED (as seen in DataSource.sync() method and SyncDataSourceJob). After a sync completes and sets status to COMPLETED or FAILED, if a user edits the DataSource, this form will reset the status to READY or NEW, losing the sync result information. The form should only update the status when explicitly changing sync_interval settings, not on every save.

Copilot uses AI. Check for mistakes.

return super().save(*args, **kwargs)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The status modification in clean() will interfere with the enqueue_sync_job signal handler in signals.py (line 259-275). When saving a DataSource with a sync_interval, the sequence will be: 1) Form sets status to READY (or NEW), 2) Form saves the instance, 3) The post_save signal fires and enqueues a job, 4) The SyncDataSourceJob.enqueue() sets status to QUEUED. This creates an inconsistent state where the form's status is immediately overwritten. The form should not manage status at all, or should coordinate with the signal/job system.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The DataSource.status field is marked as editable=False in the model (data.py line 56), indicating it's intended to be managed programmatically, not by user input. Having the form manipulate this field directly violates this design principle. The status should be managed exclusively by the sync process (DataSource.sync(), SyncDataSourceJob) and the signal handlers, not by the form. This creates a conflict between the intended API design and the form implementation.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The new status transition logic in the form's clean() and save() methods lacks test coverage. Tests should be added to verify that: 1) New DataSources get status=NEW, 2) Updating an existing DataSource with sync_interval removed sets status=READY, 3) Updating an existing DataSource with sync_interval set behaves correctly, and 4) Status from completed sync operations (COMPLETED/FAILED) is preserved appropriately. Consider adding tests to verify this form behavior.

Copilot uses AI. Check for mistakes.


Expand Down
1 change: 0 additions & 1 deletion netbox/core/jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ def enqueue(cls, *args, **kwargs):

# Update the DataSource's synchronization status to queued
if datasource := job.object:
datasource.status = DataSourceStatusChoices.QUEUED
DataSource.objects.filter(pk=datasource.pk).update(status=datasource.status)

return job
Expand Down
1 change: 0 additions & 1 deletion netbox/core/models/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ def backend_class(self):
@property
def ready_for_sync(self):
return self.enabled and self.status not in (
DataSourceStatusChoices.QUEUED,
DataSourceStatusChoices.SYNCING
)

Expand Down
Loading