-
Notifications
You must be signed in to change notification settings - Fork 127
Add DD_DASHBOARD_FORCE_SYNC_PERIOD environment variable #2184
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.
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
base: main
Are you sure you want to change the base?
Add DD_DASHBOARD_FORCE_SYNC_PERIOD environment variable #2184
Conversation
…#2179 Signed-off-by: puretension <[email protected]>
Hi! Could someone please add the 'enhancement' label and either a milestone or the 'qa/skip-qa' label to this PR? |
Signed-off-by: puretension <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should any documentation be updated?
logger.Info("DatadogDashboard manifest has changed") | ||
shouldUpdate = true | ||
} else if instance.Status.LastForceSyncTime == nil || ((defaultForceSyncPeriod - now.Sub(instance.Status.LastForceSyncTime.Time)) <= 0) { | ||
} else if instance.Status.LastForceSyncTime == nil || ((forceSyncPeriod - now.Sub(instance.Status.LastForceSyncTime.Time)) <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puretension Thanks for putting this together! It looks good I think but it seems like this doesn't follow the same pattern as the monitors do exactly. I haven't dived too deeply into the patterns used in other crd controllers here but the Monitor controller seems to check the Monitor specific MonitorLastForceSyncTime
rather than the generic LastForceSyncTime:
datadog-operator/internal/controller/datadogmonitor/controller.go
Lines 154 to 178 in 900987e
} else if instance.Status.MonitorLastForceSyncTime == nil || (forceSyncPeriod-now.Sub(instance.Status.MonitorLastForceSyncTime.Time)) <= 0 { | |
// Periodically force a sync with the API monitor to ensure parity | |
// Get monitor to make sure it exists before trying any updates. If it doesn't, set shouldCreate | |
m, err = r.get(instance, newStatus) | |
if err != nil { | |
logger.Error(err, "error getting monitor", "Monitor ID", instance.Status.ID) | |
if strings.Contains(err.Error(), ctrutils.NotFoundString) { | |
shouldCreate = true | |
} | |
} else { | |
shouldUpdate = true | |
} | |
} else if instance.Status.MonitorStateLastUpdateTime == nil || (defaultRequeuePeriod-now.Sub(instance.Status.MonitorStateLastUpdateTime.Time)) <= 0 { | |
// If other conditions aren't met, and we have passed the defaultRequeuePeriod, then update monitor state | |
// Get monitor to make sure it exists before trying any updates. If it doesn't, set shouldCreate | |
m, err = r.get(instance, newStatus) | |
if err != nil { | |
logger.Error(err, "error getting monitor", "Monitor ID", instance.Status.ID) | |
if strings.Contains(err.Error(), ctrutils.NotFoundString) { | |
shouldCreate = true | |
} | |
} | |
updateMonitorState(m, now, newStatus) | |
} | |
} |
The MonitorLastForceSyncTime
seems to be set whenever a monitor is synced. I'm not sure if having a Monitor specific variable there was intentional or not or if the maintainers care about consistency in this case but thought it'd be worthwhile to bring to your attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orkhanM Great catch! You're absolutely right about the consistency issue.
Looking at the Monitor controller pattern you referenced,
I can see it uses MonitorLastForceSyncTime for the force sync check.
I'll update the Dashboard implementation to use DashboardLastForceSyncTime
to match this pattern for consistency across controllers.
Thanks for pointing this out!
I don't think any documentation updates are needed for this change |
- Rename LastForceSyncTime to DashboardLastForceSyncTime in DatadogDashboardStatus - Update controller logic to match Monitor controller pattern - Update generated deepcopy code accordingly This ensures consistency across CRD controllers as suggested in PR feedback. Signed-off-by: puretension <[email protected]>
What does this PR do?
Adds DD_DASHBOARD_FORCE_SYNC_PERIOD environment variable to configure DatadogDashboard controller sync frequency, following the same pattern as DD_MONITOR_FORCE_SYNC_PERIOD.
Motivation
Fixes #2179 - DatadogDashboard controller has a hardcoded 60-minute force sync period, unlike DatadogMonitor which supports DD_MONITOR_FORCE_SYNC_PERIOD. This means dashboard drift from manual UI changes can persist for up to 60 minutes.
Additional Notes
Minimum Agent Versions
No minimum version requirements - this is an operator-only change.
Describe your test plan
Checklist
enhancement
qa/skip-qa
label