refactor(capacity): convert timer-based controller to controller-runtime reconciler#1025
Open
juliusclausnitzer wants to merge 3 commits into
Open
refactor(capacity): convert timer-based controller to controller-runtime reconciler#1025juliusclausnitzer wants to merge 3 commits into
juliusclausnitzer wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe capacity controller now runs as a controller-runtime Reconciler with watch-based setup and a minimum reconcile interval. Its configuration, manager wiring, and reconcile tests were updated to match the new lifecycle and timing behavior. ChangesCapacity Reconciler Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
juliusclausnitzer
changed the title
juliusclausnitzer
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/scheduling/reservations/capacity/controller_test.go (1)
857-965: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated reconcile-test setup into a same-file helper.
TestReconcile_ReactsToKnowledgeChangeandTestReconcile_MinIntervalEarlyReturnshare almost identical setup (scheme, hypervisor, knowledge, fake client, mock scheduler,NewController). Factoring it into one same-file helper keeps the tests short and focused on their distinct assertions.As per coding guidelines: "Test files should be short and contain only necessary test cases" and "keep helper functions in the same file as the tests that use them".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/reservations/capacity/controller_test.go` around lines 857 - 965, Both TestReconcile_ReactsToKnowledgeChange and TestReconcile_MinIntervalEarlyReturn duplicate the same controller test setup. Extract the repeated scheme, hypervisor, knowledge, fake client, mock scheduler, and NewController initialization into a same-file helper in controller_test.go, then have both tests call that helper and keep only their unique assertions. Use the existing identifiers newTestScheme, newHypervisor, newFlavorGroupKnowledge, newMockSchedulerServer, and NewController to make the helper easy to locate and reuse.Source: Coding guidelines
internal/scheduling/reservations/capacity/controller.go (1)
105-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the repeated watch registrations into a loop.
The four
WatchesMulticlustercalls differ only by object type and the name in the error message. A small slice-driven loop removes the copy/paste and makes adding future CRDs a one-line change.♻️ Sketch
watched := []struct { name string obj client.Object }{ {"Knowledge", &v1alpha1.Knowledge{}}, {"Hypervisor", &hv1.Hypervisor{}}, {"CommittedResource", &v1alpha1.CommittedResource{}}, {"Pipeline", &v1alpha1.Pipeline{}}, } for _, w := range watched { if bldr, err = bldr.WatchesMulticluster(w.obj, handler.EnqueueRequestsFromMapFunc(coalesce)); err != nil { return fmt.Errorf("failed to watch %s: %w", w.name, err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/reservations/capacity/controller.go` around lines 105 - 127, The controller setup repeats four nearly identical WatchesMulticluster registrations in the capacity controller’s builder chain, so collapse them into a small slice-driven loop. Use a local list of watched resources with a display name and client.Object, iterate in the same setup function, and keep the existing coalesce handler plus the per-resource error wrapping (for example, in BuildController and its WatchesMulticluster calls) so adding future CRDs only requires one entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/scheduling/reservations/capacity/controller_test.go`:
- Around line 857-965: Both TestReconcile_ReactsToKnowledgeChange and
TestReconcile_MinIntervalEarlyReturn duplicate the same controller test setup.
Extract the repeated scheme, hypervisor, knowledge, fake client, mock scheduler,
and NewController initialization into a same-file helper in controller_test.go,
then have both tests call that helper and keep only their unique assertions. Use
the existing identifiers newTestScheme, newHypervisor, newFlavorGroupKnowledge,
newMockSchedulerServer, and NewController to make the helper easy to locate and
reuse.
In `@internal/scheduling/reservations/capacity/controller.go`:
- Around line 105-127: The controller setup repeats four nearly identical
WatchesMulticluster registrations in the capacity controller’s builder chain, so
collapse them into a small slice-driven loop. Use a local list of watched
resources with a display name and client.Object, iterate in the same setup
function, and keep the existing coalesce handler plus the per-resource error
wrapping (for example, in BuildController and its WatchesMulticluster calls) so
adding future CRDs only requires one entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8c19406-0b28-49f1-8690-e21b7012b6ee
📒 Files selected for processing (4)
cmd/manager/main.gointernal/scheduling/reservations/capacity/config.gointernal/scheduling/reservations/capacity/controller.gointernal/scheduling/reservations/capacity/controller_test.go
PhilippMatthes
left a comment
PhilippMatthes
left a comment
Member
There was a problem hiding this comment.
Amazing job! I would appreciate if we could exchange on my comments a little -- maybe I'm also not grasping the full context. Let me know, we can also sync offline.
Comment on lines
+68
to
+71
| c.mu.Lock() | ||
| elapsed := time.Since(c.lastReconcileAt) | ||
| minInterval := c.config.MinReconcileInterval.Duration | ||
| c.mu.Unlock() |
Member
There was a problem hiding this comment.
Can you explain why this mutex is needed? You are setting max concurrent reconciles to 1, so there won't be any race conditions here I assume.
Collaborator
Author
There was a problem hiding this comment.
True, more of a defensive programming measure, but not really needed unless someone changes the concurrent reconciles. Since that is not an easily changeable chart value probably overkill
Comment on lines
+73
to
+79
| if c.lastReconcileAt != (time.Time{}) && elapsed < minInterval { | ||
| remaining := minInterval - elapsed | ||
| LoggerFromContext(ctx).V(1).Info("skipping reconcile: min interval not elapsed", | ||
| "elapsed", elapsed.Round(time.Second), | ||
| "remaining", remaining.Round(time.Second)) | ||
| return ctrl.Result{RequeueAfter: remaining}, nil | ||
| } |
Member
There was a problem hiding this comment.
Do I understand this code correctly? You populate the controller-runtime informer cache by watching the corresponding resources (knowledge, hypervisor, committedresource, pipeline) below in SetupWithManager. Then, you enqueue every resource change in the workqueue to trigger a reconcile. This means, the reconcile method gets basically spammed with generic requests without any concrete reference resource to reconcile on. To avoid that you run the expensive logic all the time, you check for the elapsed time and don't trigger when it's not time yet. Have you thought about different ways of solving this issue, like a periodically firing goroutine and populating the controller-runtime informer-cache some other way? The way you implemented it probably works fine, I'm just curious if there isn't a better way.
Member
There was a problem hiding this comment.
Doesn't this approach mean that, when the last resource change didn't trigger a reconcileAll because it was just shy of the deadline, but then no further resource change happens, you're getting stale data?
Collaborator
Author
There was a problem hiding this comment.
The work queue should deduplicate natively, so if all resources change simultaneously, it should still only be one call to the reconcile method, which would reduce spam significantly, but of course I understand your point that this seems a bit aggressive.
Also, the controller-runtime will re-queue after remaining minimum time, so we should not get stale-ness of more than 30 seconds.
Lets say worst case, 5 resources change in intervals of 5 seconds, and the work queue is emptying so fast that is does not deduplicate here already. In this case the first reconcile goes through. The other 4 would requeue after remaining minimal interval, so all re-queue at the same point in time (after 30s) and would then be deduplicated in the work queue, which would then reconcile and still ensure maximum staleness of 30 sec.
Comment on lines
-52
to
-66
| func (c *Controller) Start(ctx context.Context) error { | ||
| timer := time.NewTimer(0) // fire immediately on start | ||
| defer timer.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case <-timer.C: | ||
| cycleCtx := WithNewGlobalRequestID(ctx) | ||
| if err := c.reconcileAll(cycleCtx); err != nil { | ||
| LoggerFromContext(cycleCtx).Error(err, "reconcile cycle failed") | ||
| } | ||
| timer.Reset(c.config.ReconcileInterval.Duration) | ||
| } |
Member
There was a problem hiding this comment.
What was the idea behind deleting this code and replacing it with a reconcile method? See also my comment https://github.com/cobaltcore-dev/cortex/pull/1025/changes#r3544600386
Collaborator
Author
There was a problem hiding this comment.
I think Marcel got annoyed by stale data :) Here is the ticket for a bit more context https://github.wdf.sap.corp/orgs/sap-cloud-infrastructure/projects/36/views/2?sliceBy%5Bvalue%5D=I764822&pane=issue&itemId=236292&issue=sap-cloud-infrastructure%7Cworkload-management-issues%7C490
Comment on lines
491
to
506
| var existing v1alpha1.FlavorGroupCapacity | ||
| err := c.client.Get(ctx, types.NamespacedName{Name: crdName}, &existing) | ||
| if apierrors.IsNotFound(err) { | ||
| existing = v1alpha1.FlavorGroupCapacity{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: crdName}, | ||
| Spec: v1alpha1.FlavorGroupCapacitySpec{ | ||
| FlavorGroup: groupName, | ||
| AvailabilityZone: az, | ||
| }, | ||
| } | ||
| if createErr := c.client.Create(ctx, &existing); createErr != nil { | ||
| return fmt.Errorf("failed to create FlavorGroupCapacity %s: %w", crdName, createErr) | ||
| } | ||
| } else if err != nil { | ||
| return fmt.Errorf("failed to get FlavorGroupCapacity %s: %w", crdName, err) | ||
| } |
Member
There was a problem hiding this comment.
I noticed the current implementation works a bit differently from a typical reconciler pattern. Traditionally, a reconciler converges a Kubernetes resource's spec (desired state) into its status (actual state), whereas here we have a single routine creating and patching all resources together. Is there a specific reason the CRDs need to be updated in sync? Just trying to understand the context.
I was wondering if it might be worth exploring an approach similar to how we handle cortex datasources? Something like a Start(ctx) routine that discovers all the possible flavor groups and ensures the resource specs exist, paired with a reconciler on each individual flavor group resource that collects the data and writes it to the status. As a nice bonus, we could track a "last updated" timestamp inside the resource and take advantage of the controller-runtime workqueue the way it's designed to be used.
Of course, this is not a show-stopper from my side, but definitely something to be aware of. I can also share more details if you like on how I exactly would implement this, if you want to spend more time on refactoring.
Collaborator
Author
There was a problem hiding this comment.
I think that might not be optimal here, as we have interdependencies between different flavor groups. For example, to calculate freeResources we use a round robin approach that includes all groups. But happy to be proven wrong.
Contributor
Test Coverage ReportTest Coverage 📊: 70.1% |
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
2 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.