filter irrelevant pods in pod controller

[nayihz]

Pod controller only need to care about pods that related to inferencepool. So we can prevent these irrelevant pods enter in queue.

[netlify]

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	125dd5e
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/6808592ffce4e3000819b8d0
😎 Deploy Preview	https://deploy-preview-696--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[danehans]

This will prevent Pods from being removed from the datastore. For example, an Inference Platform Owner migrates model server pods from one InferencePool to another. Thoughts @ahg-g @kfswain @nirrozenbaum

[nayihz]

predicate.Funcs{
    CreateFunc: func(ce event.CreateEvent) bool {
	    pod := ce.Object.(*corev1.Pod)
	    return c.Datastore.PoolLabelsMatch(pod.GetLabels())
    },
    UpdateFunc: func(ue event.UpdateEvent) bool {
	    oldPod := ue.ObjectOld.(*corev1.Pod)
	    newPod := ue.ObjectNew.(*corev1.Pod)
	    return c.Datastore.PoolLabelsMatch(oldPod.GetLabels()) || c.Datastore.PoolLabelsMatch(newPod.GetLabels())
    },
    DeleteFunc: func(de event.DeleteEvent) bool {
	    pod := de.Object.(*corev1.Pod)
	    return c.Datastore.PoolLabelsMatch(pod.GetLabels())
    },
    GenericFunc: func(ge event.GenericEvent) bool {
	    pod := ge.Object.(*corev1.Pod)
	    return c.Datastore.PoolLabelsMatch(pod.GetLabels())
    },
}

This will prevent Pods from being removed from the datastore.

Add UpdateFunc will not lose event. WDYT? @danehans

[kfswain]

Add UpdateFunc will not lose event. WDYT? @danehans

IIRC the predicate is sent to the API server. And, unfortunately, since the Selector can be changed, this doesn't work. Filtering client side wouldn't reduce API server load, nor client load really (reconciliation isn't much more expensive).

We could have a discussion around: Should the InferencePool selector be mutable? I lean towards yes, I don't want to force a user to change an entire inferencePool over label selectors, but that's a different discussion.

Another solution would be to regenerate the controllers on a pool Selector update, buuuut that's a not-awesome solution either.

Happy to hear other thoughts! Thank you for the contributions regardless, always happy to see new "faces"(github Handles). And even if we translate this PR to documenting this discussion as comments on the SetupWithManager func, that would be a win to me also.

[nayihz]

Thanks for your reply. But I'm very sorry that I can't get what you mean clearly. This PR aims to reduce the frequency of reconciliation of pod reconciler. The changes related to the IInferencePool.Spec.Selector you mentioned should be reconciled by the pool reconciler, but not pod reconciler, right? @kfswain

[nayihz]

After test this PR in my local env, controller will panic sometimes when start. I think it's because the pool reconciler may lag behind(ds.pool == nil) when both pod and pool reconciler start concurrently. So poolSelector := selectorFromInferencePoolSelector(ds.pool.Spec.Selector) will raise to panic.

	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x108 pc=0x1655557]

goroutine 305 [running]:
k8s.io/apimachinery/pkg/util/runtime.handleCrash({0x2548860, 0x3865660}, {0x1e85320, 0x37ec1c0}, {0x3865660, 0x0, 0x100000000440b58?})
	/go/pkg/mod/k8s.io/[email protected]/pkg/util/runtime/runtime.go:89 +0xe7
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc000b121c0?})
	/go/pkg/mod/k8s.io/[email protected]/pkg/util/runtime/runtime.go:59 +0x105
panic({0x1e85320?, 0x37ec1c0?})
	/usr/local/go/src/runtime/panic.go:792 +0x132
sigs.k8s.io/gateway-api-inference-extension/pkg/epp/datastore.(*datastore).PoolLabelsMatch(0x417691?, 0xc00002d860)
	/src/pkg/epp/datastore/datastore.go:129 +0x77
sigs.k8s.io/gateway-api-inference-extension/pkg/epp/controller.(*PodReconciler).SetupWithManager.func1({{0x2570768?, 0xc000805b08?}})
	/src/pkg/epp/controller/pod_reconciler.go:73 +0x42

[hzxuzhonghu]

This is not right, pool label is not known before this calling

[nayihz]

It'll be reconciled by c.Datastore.PodResyncAll when pool added/updated labels. So I think it is acceptable to ignore these Pods at this time.
But as #696 (comment) mentioned, this change will prevent Pods from being removed from the datastore. So I proposed a new way to solve this promblem. #696 (comment)

[nirrozenbaum]

that's good to have without any relation to the predicate.
nit: you can use the internal ds function (same logic, just to be consistent with GetPool func)

if !ds.PoolHasSynced() {
	return false
}

[nayihz]

It will cause deadlock if we use ds.PoolHasSynced because it also need to acquire poolAndModelsMu.

[nirrozenbaum]

are you sure? :)
Go mutex behavior around reentrant locks can be a bit nuanced.
see this example here https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/epp/datastore/datastore.go#L112-L117.
you can give it a try in your fork.

[nayihz]

I think it's safe not to acquire RWMutex twice in one function.
https://pkg.go.dev/sync#RWMutex

If any goroutine calls RWMutex.Lock while the lock is already held by one or more readers, concurrent calls to RWMutex.RLock will block until the writer has acquired (and released) the lock, to ensure that the lock eventually becomes available to the writer. Note that this prohibits recursive read-locking.

https://stackoverflow.com/questions/30547916/goroutine-blocks-when-calling-rwmutex-rlock-twice-after-an-rwmutex-unlock

[nirrozenbaum]

it's definitely safe to keep current check.
was just a nice to have comment :)

[nirrozenbaum]

IIRC the predicate is sent to the API server. And, unfortunately, since the Selector can be changed, this doesn't work. Filtering client side wouldn't reduce API server load, nor client load really (reconciliation isn't much more expensive).

predicate is a client side filter.
it doesn't reduce API server load, it does reduce the addition of the pod to workqueue and the reconciliation cycle.
overall, after @nayihz latest change this is LGTM from my side.
not reducing a lot, but still a small improvement, mainly in the logging of epp when trying to debug (reducing noise in log when pod is not relevant).

[nirrozenbaum]

overall, after @nayihz latest change this is LGTM from my side.
not reducing a lot, but still a small improvement, mainly in the logging of epp when trying to debug (reducing noise in log when pod is not relevant).

since I'm not sure it's acceptable by other reviewers of this PR, I'm leaving the final LGTM stamp for the others.

[nayihz]

kindly ping @kfswain @danehans for further review.

[kfswain]

overall, after @nayihz latest change this is LGTM from my side.
not reducing a lot, but still a small improvement, mainly in the logging of epp when trying to debug (reducing noise in log when pod is not relevant).

++ this PR lgtm, and thank you @nayihz for the contribution!!

[kfswain]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfswain, nayihz

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:

• OWNERS [kfswain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kfswain]

(not directly related to this PR) I am leaving this comment for posterity, and will reference it in the future if needed:
I find the RoI on PRs like this to be low, and often descend into opinionated discussion, which can pull a high amount of reviewer attention/energy, which is somewhat of a limited resource.

Following controller/reconciler optimizations should have very clear reasons why the PR is needed.