Replacing endpointSlice Reconciler with a direct Pod Reconciler #300
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/hold #301 should submit first, and there is still a cache dumping event that needs to be implemented (when the InferencePool changes selector) |
603f683 to
8cc1b74
Compare
|
|
||
| func (c *PodReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&corev1.Pod{}). |
There was a problem hiding this comment.
This is less efficient, iiuc, each pod event can trigger reconcile.
There was a problem hiding this comment.
Maybe should migrate to raw k8s informer
There was a problem hiding this comment.
Yeah, we are aware of that, in all cases we will need to start the pod controller only after we know what the inferencePool is to know the selector and setup a sever side filter. I will check if controller-runtime allows us to set that up, but agree switching to raw informer is an option.
Another approach I am thinking about is to have the inferencePool controller create a service to trigger creating endpointslice objects, and we continue to work with what we have. wdyt about this? I know Rob doesn't like it :)
There was a problem hiding this comment.
So I checked, in controller-runtime we can setup a server-side filter, but again the complexity lies in the fact that we don't know the selector beforehand (i.e., at the time we start the operator), but also the challenge is that we need to reset it every time the selector is updated. Which is a complexity with both controller-runtime and informers. Ideally we need the inferencePool controller start the pod controller.
In any case, I think it is ok continue with controller-runtime for now, do the minimal things needed to have a functionally correct setup, and we can revisit in the near future how we want to make this more scalable. I would rather focus our efforts in the short term on optimizing the scheduling algorithm and prefix cache aware routing. @kfswain wdyt?
There was a problem hiding this comment.
I agree.
I also agree that this is inefficient. But yes, I think we can live with controller-runtime for now, and focus on feature breadth, and then come back to things like this when we are more focused on efficiency.
Happy to make an issue about this so it's not dropped.
There was a problem hiding this comment.
Another approach I am thinking about is to have the inferencePool controller create a service to trigger creating endpointslice objects, and we continue to work with what we have. wdyt about this? I know Rob doesn't like it :)
Persoanlly i donot care much about whether a service is needed to select backend pods.
If without service, we can start multiple informer to use filter, each for a InferencePool?
|
/lgtm Thanks! |
|
/unhold |
…rnetes-sigs#300) * reversion to pod reconciliation * adding ready check and unit tests * updating test * ablating unnecessary func * embedding ready status into update so non-ready pods are deleted * scrubbing serviceName & zone as they are obsolete * implementing pod cache flushing logic * Renaming file so merge confilcts can find the diffs easier * cleaning up messy merge conflict * nil checking short circuit * Listing fixes * feedback cleanup * log formatting and removing pods if not found * removing err to provent perma-reconciliation * removing dev image ref * cleaning up err logic
…rnetes-sigs#300) * reversion to pod reconciliation * adding ready check and unit tests * updating test * ablating unnecessary func * embedding ready status into update so non-ready pods are deleted * scrubbing serviceName & zone as they are obsolete * implementing pod cache flushing logic * Renaming file so merge confilcts can find the diffs easier * cleaning up messy merge conflict * nil checking short circuit * Listing fixes * feedback cleanup * log formatting and removing pods if not found * removing err to provent perma-reconciliation * removing dev image ref * cleaning up err logic
This works with the reversion commit: #300
This is a stylistic decision to maintain readability of our K8s interface layer.
The PR that was reverted utilizes in-line informers to list the relevant pods from the
InferencePoolselector.In the event that we hit scalability concerns wrt pod reconciliation. We should look to move away from the standard controller-runtime Reconciliation and look to either a simple list backed by informers. Such as the referenced PR, or move to self-implemented work-queue, informer based controller implementation (Example here). Which would allow us to only allow reconcile on pods that match the selector.
This PR also removes the
serviceName&zoneflags, as they are no longer in use with the removal of the EndpointSlice