Filter InferenceModels from Reconciliation

[danehans]

Currently, the InferenceModel reconciler reconciles all InferenceModels in the configured namespace. Since the endpoint picker extension is configured for a single InferencePool name, the InferenceModel reconciler should only reconcile InferenceModels that reference, e.g. poolRef, the configured InferencePool.

InferenceModels that do not reference the configured InferencePool are deleted from the data store. Why is this approach being taken instead of filtering the watch, e.g Predicate, so the reconciler only reconciles matching InferenceModels?

[danehans]

/assign

[liu-cong]

@kfswain may have some insights. It looks to me either approach should work?

(This is being discussed here #113 (comment))
I have another point about filtering InferenceModels in the same namespace as the InferencePool. In my opinion users should be allowed to create InferenceModels in different namespaces (think a team owns its own model and namespace). cc @ahg-g and @robscott

[ahg-g]

/kind bug

We need to do this. This is actually a bug.

[kfswain]

/assign

[danehans]

In my opinion users should be allowed to create InferenceModels in different namespaces (think a team owns its own model and namespace)

xref #113

IMHO this should be a post-v0.2 task but feel free to assign yourself the issue if you want to add this feature.

[ahg-g]

I think this is a bug, the epp currently is 1:1 with inference pool, and so we shouldn't be serving traffic for models that are not defined on the pool that epp was created for, right?

[kfswain]

We don't. We already filter on pool name and namespace. I was gonna ask you how this was a bug. It's definitely unoptimized, but I wasnt clear on the bug portion.

I think @danehans is responding to the ask to let inference models be written in different namespaces, but reference the same pool. I personally want to hold off on that feature till there is a pretty resounding ask for it, as that seems atypical behavior

[liu-cong]

Pls ignore my comment on cross namespace reference, it's being discussed here:
#113 (comment)

IMO the current implementation works as intended. The proposed solution here (using Predicate to filter) is an improvement.

[danehans]

We already filter on pool name and namespace.

This bug is related to InferenceModel reconciliation and not InferencePools. Only InferenceModels with spec.poolRef of the configured --poolName and --poolNamespace should be reconciled. Currently, all InferneceModels in the --poolNamespace will be reconciled and stored in the data store c.updateDatastore(logger, infModel).

[kfswain]

This line filters based on pool name:

gateway-api-inference-extension/pkg/ext-proc/controller/inferencemodel_reconciler.go

Line 74 in 9f34673

if infModel.Spec.PoolRef.Name == c.PoolNamespacedName.Name {

[nirrozenbaum]

there is an easy way to setup a predicate that will filter reconcilation events unless this condition is satisfied:
infModel.Spec.PoolRef.Name == c.PoolNamespacedName.Name

@danehans would it be ok if I push that fix in a PR? or do you prefer doing it yourself?

[kfswain]

I have an open PR working on that rn, happy to push it. Just wanted some clarification here

[nirrozenbaum]

as far as I understood from this issue, I think @danehans meant adding a predicate to filter the events during the controller registration within the controller manager using WithEventFilter func rather than getting the events and check the PoolRef at the beginning of the reconciliation handler func.

[kfswain]

Actually, introducing this change would introduce a bug.

Imagine the case where an inferenceModel is created with the poolRef pointing to your pool. The predicate returns true, and the model is added to the datastore. This same model resource is updated to have a different poolRef. The predicate returns false and the model is not removed from the datastore erroneously. The line I linked above force deletes the model if the pool names dont match. This is WAI.

[ahg-g]

Ah, we do have the check at https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/ext-proc/controller/inferencemodel_reconciler.go#L74

[nirrozenbaum]

Actually, introducing this change would introduce a bug.

Imagine the case where an inferenceModel is created with the poolRef pointing to your pool. The predicate returns true, and the model is added to the datastore. This same model resource is updated to have a different poolRef. The predicate returns false and the model is not removed from the datastore erroneously. The line I linked above force deletes the model if the pool names dont match. This is WAI.

@kfswain the bug you described here is a matter of implementation.
it will happen if the code checks only for the updated object and doesn't check the previous object (in update func), while the correct behavior should be to check both objects.
if there are no objections I'll be happy to push a PR with the correct behavior to fix this and reduce the reconcile events noise.

[nirrozenbaum]

please see PR #397

[danehans]

Reopening the issue due to #149 (comment).

[nirrozenbaum]

@danehans since you originally opened this issue, I'd be happy to hear your thoughts on the last comments in PR #397