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 all 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
17 changes: 16 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,28 @@ 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):

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

# Determine initial status based on new/existing instance
if not self.instance.pk:
# New instance
object_status = DataSourceStatusChoices.NEW
else:
# Existing instance
if not self.cleaned_data.get("sync_interval"):
object_status = DataSourceStatusChoices.READY
else:
object_status = self.instance.status

# # Final override only if the user explicitly provided a status
self.instance.status = object_status

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


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