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 14 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
5 changes: 1 addition & 4 deletions netbox/core/models/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,7 @@ def backend_class(self):

@property
def ready_for_sync(self):
return self.enabled and self.status not in (
DataSourceStatusChoices.QUEUED,
DataSourceStatusChoices.SYNCING
)
return self.enabled and self.status != DataSourceStatusChoices.SYNCING
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.

Question: If we are adding the READY state and other changes here do we still need to skip QUEUED check here? Just wondering if your other changes will allow it to be kept as I think if it is actually queued and hasn't finished we probably don't want to allow sync again?

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.

that's why I had initially added the scheduled state initially, so that when it's queued, I would disable the syncing button, but allow it only when the state is scheduled. The issue was that scheduled was completely redundant with Queued. As they could be set only during the save action of the data-source object. They would also get overwritten when the job finishes. so queued, just as before reflects when the job is queued when the user hits sync, but also when the sync_interval is set.

  • The proper way to fix it would be to add a new field, for example last-status and use that for the latest job status.
  • keep the current status field, and use it only for the workers Q state (new, ready, queued) and maybe re-add the scheduled state as well.

what do you think?

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.

While removing QUEUED from the ready_for_sync check allows users to manually trigger syncs even when a recurring job is queued, this change could allow multiple simultaneous sync operations for the same DataSource. The SyncDataSourceJob.enqueue() method sets status to QUEUED before the job runs (in jobs.py line 34). If a user manually triggers a sync while a job is queued but not yet running, both could execute. Consider whether QUEUED should be excluded or if additional logic is needed to prevent race conditions.

Suggested change
return self.enabled and self.status != DataSourceStatusChoices.SYNCING
return self.enabled and self.status not in (
DataSourceStatusChoices.SYNCING,
DataSourceStatusChoices.QUEUED,
)

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 modified ready_for_sync logic lacks test coverage. Given that this property is critical for determining when a DataSource can be synced and is a key part of the bug fix, tests should be added to verify the behavior with different status values (NEW, QUEUED, READY, COMPLETED, FAILED) and the enabled flag. Consider adding tests to netbox/core/tests/test_models.py to cover this property.

Copilot uses AI. Check for mistakes.

def clean(self):
super().clean()
Expand Down
Loading