Enhance logging in OpenStackDatasourceReconciler#654
Merged
SoWieMarkus merged 1 commit intomainfrom Mar 27, 2026
Merged
Conversation
…ity during reconciliation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
📝 WalkthroughWalkthroughThe OpenStack datasource controller's Reconcile method now includes explicit "NotFound" error handling as a no-op case, adds extensive trace logging throughout the reconciliation lifecycle, elevates unsupported syncer lookups to error-level logging, and changes the requeue interval to use the configured SyncInterval directly instead of calculating time remaining until the next sync. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Test Coverage ReportTest Coverage 📊: 68.1% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/knowledge/datasources/plugins/openstack/controller.go (1)
246-257:⚠️ Potential issue | 🟠 MajorRequeue against
nextTime, not “interval from now”.Line 246 already computes and stores the planned next sync time, but Line 257 now waits a full
SyncIntervalfrom the end of the reconcile. That guarantees the controller wakes up afterstatus.nextSyncTime, so the schedule drifts by the time spent patching/logging each cycle and the status field stops matching actual behavior.Suggested fix
// Calculate the next sync time based on the configured sync interval. log.Info("Finished reconcile", "next", nextTime) -return ctrl.Result{RequeueAfter: datasource.Spec.OpenStack.SyncInterval.Duration}, nil +return ctrl.Result{RequeueAfter: time.Until(nextTime)}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/datasources/plugins/openstack/controller.go` around lines 246 - 257, The controller currently sets datasource.Status.NextSyncTime = metav1.NewTime(nextTime) but returns ctrl.Result{RequeueAfter: datasource.Spec.OpenStack.SyncInterval.Duration}, which causes drift; change the requeue to use the actual remaining time until nextTime by computing dur := nextTime.Sub(time.Now()) (clamp to >=0) and return ctrl.Result{RequeueAfter: dur}; update the return in the reconcile function that currently references RequeueAfter to use this computed dur so the status.NextSyncTime matches controller wake-up time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/knowledge/datasources/plugins/openstack/controller.go`:
- Around line 198-200: The log shows "Finished syncing datasource" before
checking err from syncer.Sync; change the flow in the controller so you call
nResults, err := syncer.Sync(ctx), then immediately inspect err: if err != nil
handle/return it (and log a non-success message including the error and
datasource.Name), treating ErrWaitingForDependencyDatasource specially if
needed, and only call log.Info("Finished syncing datasource", "name",
datasource.Name, "numberOfResults", nResults) when err == nil so failures are
not logged as successful.
---
Outside diff comments:
In `@internal/knowledge/datasources/plugins/openstack/controller.go`:
- Around line 246-257: The controller currently sets
datasource.Status.NextSyncTime = metav1.NewTime(nextTime) but returns
ctrl.Result{RequeueAfter: datasource.Spec.OpenStack.SyncInterval.Duration},
which causes drift; change the requeue to use the actual remaining time until
nextTime by computing dur := nextTime.Sub(time.Now()) (clamp to >=0) and return
ctrl.Result{RequeueAfter: dur}; update the return in the reconcile function that
currently references RequeueAfter to use this computed dur so the
status.NextSyncTime matches controller wake-up time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18ff011d-ec5c-4550-bee3-75141a648b7d
📒 Files selected for processing (1)
internal/knowledge/datasources/plugins/openstack/controller.go
PhilippMatthes
approved these changes
Mar 27, 2026
umswmayj
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.