✨ Delay reconciliation until handlers sync #3406
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GonzaloLuminary The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @GonzaloLuminary! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @GonzaloLuminary. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
size/L
|
I thought this already works out of the box as every Get/List call of the cached reader check if the corresponding informer has synced |
We thought the same, but we were not able to get the initial reconciles to happen after all the elements coming from the first List passed through the event handlers. The test added in this PR does not pass in master which indicates that reconciles are happening before all initial elements go through the event handlers. We think that controller-runtime is only waiting for the first List to end before starting the reconcile workers. We'd prefer in some cases for the reconciles to happen after all initial elements have passed through the event handlers. |
There was a problem hiding this comment.
/ok-to-test
@GonzaloLuminary please sign the CLA and use an e-mail address connected to your github account so we can accept your contribution
pkg/internal/source/kind.go
Outdated
|
|
||
| // WaitForHandlerSync when set to true, waits for the handler registration's HasSynced | ||
| // before starting reconciliation. | ||
| WaitForHandlerSync bool |
There was a problem hiding this comment.
Is there any reason to make this optional? IOW, are there known use-cases where one would not want this?
There was a problem hiding this comment.
This option slows down reconciliations after reboots in non-HA mode. Given that business logic is usually only implemented in Reconcile, I think it can be dangerous to always leave this option as true due to its performance implications.
There was a problem hiding this comment.
It may result in it taking longer until the controller is started but unless the handlers are slow, it shouldn't make a big difference. Waiting for the handlers ensures that all requests got in and properly get de-duplicated so it avoids work, so I don't think it would overall be a slowdown.
@sbueringer do you have a take on if this should be configurable?
There was a problem hiding this comment.
Waiting for the handlers ensures that all requests got in and properly get de-duplicated so it avoids work, so I don't think it would overall be a slowdown.
Agree.
I think it doesn't have to be configurable, but if we make it configurable I would prefer if it's enabled per default.
k8s-ci-robot
added
ok-to-test
| reconcileCalledChan := make(chan struct{}) | ||
|
|
||
| // Create the controller. | ||
| testQueue := &controllertest.Queue{ |
There was a problem hiding this comment.
Please use the actual constructor rather than copying it partly
There was a problem hiding this comment.
This is not resolved at all. Please use the controller constructor instead of rebuilding it for your testcase. Please also do not resolve my comments as it hides them, it is up to the person making the comment to decide if they got addressed or not
There was a problem hiding this comment.
I'm not sure what which constructor to use here. Maybe it's because the comment is outdated and the code has changed a bit. Can you please let me know which constructor to use? Happy to apply the changes
I am guessing that unblocks alrready once the store got synced, but that doesn't necesitate that he handlers did finish processing, especially if they are slow and potentially themselves do something like listing against the cache? And ofc we only do this for the primary object of the controller, not secondary for which it may have handlers. |
GonzaloLuminary
force-pushed
the
gonzalo/wait_for_handler_sync
branch
from
6ad796a to
5509dd7
Compare
k8s-ci-robot
added
cncf-cla: yes
alvaroaleman
added
the
tide/merge-method-squash
I think I'm missing something. I could be entirely wrong I thought this works like this: (xref: https://github.com/kubernetes/sample-controller/blob/master/docs/images/client-go-controller-interaction.jpeg)
Would this alone ensure that the controller always sees the global picture of all resources? (under the assumption that the "global picture" is build up based on Get/List calls on the cached client, not by whatever might be done in eventhandlers) Or are you building up an additional data store via event handlers in your case? So yes the Reconcile would be started before all event handlers are executed, but I'm not following why executing the event handlers is necessary to get a complete global picture if we ensure on every Get/List that the thread safe store has all initial objects. Independent of that point, I think Alvaro is right with #3406 (comment) and it would be beneficial to wait for all event handlers to complete to deduplicate all initial events and avoid redundant reconciles of the same objects on startup. |
This is important for controllers that handle quota, for example a controller that has to remove scheduling gates based on some quota decision. For performance reasons, the quota computation requires a starting point after which decisions can be made. Quota is then modified incrementally instead of having to reconstruct the quota state from expensive List calls. I think other controllers in the k8s repo wait on the handlers to sync. I think that kube-scheduler does this in https://github.com/kubernetes/kubernetes/blob/627ce4946ccd6b67bc18f5a4dda92cf6c583e9b7/cmd/kube-scheduler/app/server.go#L263-L274 and https://github.com/kubernetes/kubernetes/blob/c6be0527684a967c1b0dd14486ae2241832723f0/pkg/scheduler/eventhandlers.go#L486 |
|
For a bit more context, we had similar discussions about the sync issue in controller-runtime within the kueue repo kubernetes-sigs/kueue#5221 |
|
I don't entirely follow, but I"m fine with the change as I see the point about avoiding redundant reconciles on startup Let's continue with this discussion #3406 (comment) |
If I understand that issue correctly, the problem is that a custom cache (not the controller-runtime cache) is being filled from the handlers, so if reconcile runs before the event handlers finished processing, that custom cache will not be in sync. |
|
@GonzaloLuminary we want to release the next c-r minor version soon, are you able to address the feedback? |
GonzaloLuminary
force-pushed
the
gonzalo/wait_for_handler_sync
branch
from
5509dd7 to
e2eb199
Compare
GonzaloLuminary
changed the title
k8s-ci-robot
added
cncf-cla: no
GonzaloLuminary
force-pushed
the
gonzalo/wait_for_handler_sync
branch
from
e2eb199 to
51236f5
Compare
k8s-ci-robot
added
cncf-cla: yes
I have removed the option to wait for the handlers. Instead we will always wait for the handlers to sync before starting the reconciliation. |
GonzaloLuminary
force-pushed
the
gonzalo/wait_for_handler_sync
branch
from
51236f5 to
975e3aa
Compare
pkg/internal/source/kind.go
Outdated
| return | ||
| } | ||
| if !ks.Cache.WaitForCacheSync(ctx) { | ||
| if !toolscache.WaitForCacheSync(ctx.Done(), handlerRegistration.HasSynced) { |
There was a problem hiding this comment.
Why do we check both? Isn't the Cache.WaitForCacheSync implicit in the registration having synced?
There was a problem hiding this comment.
Not sure, I removed it and some unit tests fail sine they are moking the informers WaitForCacheSync return value and I cannot make it work. I'm not sure if the registration sync is equivalent to the cache sync though. I'm not sure if the cache can have more sources
There was a problem hiding this comment.
I see, I guess there is little harm in having both so lets re-add it
| <-sourceSynced | ||
| }) | ||
|
|
||
| It("should not call Reconcile until all event handlers have processed initial objects", func(specCtx SpecContext) { |
There was a problem hiding this comment.
A simler version of the test below, main changes:
- Start cache right at the beginning so it syncs in the background
- The firstPod handling doesn't seem neccessary
- Its enough to test inside reconcile that the handler processed everything to validate the ordering, we don't need to also test on the outside that reconcile was called after the handler finished
It("should not call Reconcile until all event handlers have processed initial objects", func(specCtx SpecContext) {
nPods := 20
pods := make([]*corev1.Pod, nPods)
for i := range nPods {
pods[i] = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: strconv.Itoa(i),
Namespace: "default",
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{{Name: "test", Image: "test"}},
},
}
_, err := clientset.CoreV1().Pods("default").Create(specCtx, pods[i], metav1.CreateOptions{})
Expect(err).NotTo(HaveOccurred())
}
defer func() {
for _, pod := range pods {
_ = clientset.CoreV1().Pods("default").Delete(specCtx, pod.Name, metav1.DeleteOptions{})
}
}()
testCache, err := cache.New(cfg, cache.Options{})
Expect(err).NotTo(HaveOccurred())
ctx, cancel := context.WithCancel(specCtx)
defer cancel()
go func() {
defer GinkgoRecover()
_ = testCache.Start(ctx)
}()
// Tracks how many objects have been processed by the event handler.
var handlerProcessedCount atomic.Int32
// Channel to block one of the event handlers to simulate slow event handler processing.
blockHandler := make(chan struct{})
// Tracks whether Reconcile was called.
var reconcileCalled atomic.Bool
// Create the controller.
testCtrl := New(Options[reconcile.Request]{
MaxConcurrentReconciles: 1,
CacheSyncTimeout: 10 * time.Second,
NewQueue: func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
return &controllertest.Queue{
TypedInterface: workqueue.NewTyped[reconcile.Request](),
}
},
Name: "test-reconcile-order",
LogConstructor: func(_ *reconcile.Request) logr.Logger {
return log.RuntimeLog.WithName("test-reconcile-order")
},
Do: reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
// handlerProcessedCount should be equal to the number of pods created since we are waiting
// for the handlers to finish processing before reconciling.
Expect(handlerProcessedCount.Load()).To(Equal(int32(nPods)))
reconcileCalled.Store(true)
return reconcile.Result{}, nil
})},
)
// Watch pods with an event handler that blocks all pods but the first one in the list.
// Kind sources wait for handler sync to ensure that Reconcile is not called until all
// initial objects have been processed by the event handlers.
err = testCtrl.Watch(source.Kind(testCache, &corev1.Pod{}, handler.TypedFuncs[*corev1.Pod, reconcile.Request]{
CreateFunc: func(ctx context.Context, evt event.TypedCreateEvent[*corev1.Pod], q workqueue.TypedRateLimitingInterface[reconcile.Request]) {
<-blockHandler
handlerProcessedCount.Add(1)
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{Name: evt.Object.GetName(), Namespace: evt.Object.GetNamespace()}})
},
}))
Expect(err).NotTo(HaveOccurred())
controllerDone := make(chan error)
go func() {
defer GinkgoRecover()
controllerDone <- testCtrl.Start(ctx)
}()
// Give the controller time to start the reconciler. We asserts
// in there that all events have been processed, so if we start it
// prematurely, that assertion will fail. We can not get rid of the
// sleep unless we stop using envtest for this test.
time.Sleep(1 * time.Second)
close(blockHandler)
Eventually(reconcileCalled.Load).Should(BeTrue())
cancel()
Eventually(controllerDone, 5*time.Second).Should(Receive())
})
|
lgtm modulo the two comments I left, @GonzaloLuminary |
4 participants
Ensure reconciliation does not start until all handlers have synced. This is important for controllers that need a global picture of existing resources before making reconciliation decisions.
Examples include the k8s ResourceQuota controller and cluster autoscaler, which do this internally with client-go. This PR brings the same capability to controller-runtime.
Related: Projects like Kueue could also benefit from this feature, though they would need additional API to coordinate handler sync across multiple managers before allowing reconciliation.