feat: Added a factory function for the DecisionTree filter #1053

shmuelk · 2025-06-24T08:31:13Z

This PR adds a factory function for the DecisionTree filter along with tests for the added factory function.

In addition there is a small refactoring in cmd/epp/runner/runner.go that was requested in PR #1039

This PR fixes issue #1011

netlify · 2025-06-24T08:31:18Z

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

Name	Link
🔨 Latest commit	`293a4f0`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/685d670f51e87700084ba5fc
😎 Deploy Preview	https://deploy-preview-1053--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 project configuration.

k8s-ci-robot · 2025-06-24T08:31:23Z

Hi @shmuelk. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

ahg-g · 2025-06-24T16:07:39Z

/ok-to-test

ahg-g · 2025-06-24T16:08:03Z

/assign @liu-cong

liu-cong

/lgtm

Thanks! Just a few nits.

liu-cong · 2025-06-24T20:53:32Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+	return loadDecisionTree(&parameters, handle)
+}
+
+func loadDecisionTree(parameters *decisionTreeFilterParameters, handle plugins.Handle) (framework.Filter, error) {


nit: Prefer returning the concrete type (*DecisionTreeFilter) than the interface

liu-cong · 2025-06-24T20:54:03Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+	return result, nil
+}
+
+func loadDecisionTreeEntry(entry *decisionTreeFilterEntry, handle plugins.Handle) (framework.Filter, error) {


same nit, return concrete type

This function doesn't always return a DecisionTreeFilter, it may return a framework.Filter.

liu-cong · 2025-06-24T20:55:16Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+	if err != nil {
+		return nil, err
+	}
+	if result.Current == nil {


loadDecisionTreeEntry should make sure if error is nil, the result is always valid, so no need to check again

liu-cong · 2025-06-24T20:57:29Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+
+func loadDecisionTreeEntry(entry *decisionTreeFilterEntry, handle plugins.Handle) (framework.Filter, error) {
+	if entry == nil {
+		return nil, nil


I prefer the entry is never nil, and the caller should make sure it only calls loadDecisionTreeEntry when the entry is not nil.

Ideally when error is nil, the result is alway valid and safe to use

Ideally when error is nil, the result is alway valid and safe to use

++

However, I do think it's reasonable to nil check and return an error in this case. Or you'll still need to do the nil check discussed here: https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1053/files#r2164883752

Done. The caller now checks the entries before calling loadDecisionTreeEntry

liu-cong · 2025-06-24T21:00:57Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+
+	if entry.PluginRef != nil {
+		instance := handle.Plugins().Plugin(*entry.PluginRef)
+		if instance == nil {


This is unrelated but I think the handle should provide a GetPlugin() method and return an error if the plugin ref doesn't exist. That way is more explicit and less error prone.

++, agree with one small difference.
I suggest GetPlugin to return (plugin, bool) same as map does (bool - if key was found or not)

map[key], doesn't have to return a bool. There were reviews in which w were told to remove the bool and check for nll instead.

In general getters in Go do not have the prefix Get unlike for example Java.

It's more idiomatic in go to have the Plugin method to either return a bool or error to indicate the failure scenario. Maybe I was missing some context but I am not sure why checking nil is preferred.

Just calling this out but not related to this PR thought

shmuelk · 2025-06-25T10:58:34Z

/retest

shmuelk · 2025-06-25T11:59:07Z

I had done a rebase but the automation had issues with the commits from the dependency bot.

I re-did the rebase and force pushed.

liu-cong

/lgtm

liu-cong · 2025-06-25T17:01:07Z

pkg/epp/scheduling/framework/plugins/filter/decision_tree_filter.go

+
+	if entry.PluginRef != nil {
+		instance := handle.Plugins().Plugin(*entry.PluginRef)
+		if instance == nil {


It's more idiomatic in go to have the Plugin method to either return a bool or error to indicate the failure scenario. Maybe I was missing some context but I am not sure why checking nil is preferred.

Just calling this out but not related to this PR thought

nirrozenbaum · 2025-06-25T21:21:54Z

@shmuelk one conflict to solve and I think this is ready for merge.

nirrozenbaum · 2025-06-26T14:32:13Z

@shmuelk can you squash commits so past PRs(and their authors) won’t appear on your PR?

Signed-off-by: Shmuel Kallner <[email protected]>

shmuelk · 2025-06-26T15:29:07Z

@liu-cong I was asked to redo the history. Please re-approve

shmuelk · 2025-06-26T15:37:17Z

/retest

nirrozenbaum · 2025-06-26T16:31:27Z

/lgtm
/approve

Thanks! 🙏🏼

k8s-ci-robot · 2025-06-26T16:31:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nirrozenbaum, shmuelk

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~~ [nirrozenbaum]

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

…s-sigs#1053) * Added a factory function for the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Added tests of the factory function of the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Registered the factory function of the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Refactored the configuration loading Signed-off-by: Shmuel Kallner <[email protected]> --------- Signed-off-by: Shmuel Kallner <[email protected]>

Add APIs for the instantiated plugins to the EPP Handle (kubernetes-sigs#1039) * Added plugin instance APIs to plugins.Handle Signed-off-by: Shmuel Kallner <[email protected]> * An implementation of the new plugins.Handle APIs Signed-off-by: Shmuel Kallner <[email protected]> * Moved all configuration loading code to new package Signed-off-by: Shmuel Kallner <[email protected]> * Updates due to new and moved APIs Signed-off-by: Shmuel Kallner <[email protected]> * Cleanup of old configuration loading code Signed-off-by: Shmuel Kallner <[email protected]> --------- Signed-off-by: Shmuel Kallner <[email protected]> chore(deps): bump the kubernetes group with 6 updates (kubernetes-sigs#1050) Bumps the kubernetes group with 6 updates: | Package | From | To | | --- | --- | --- | | [k8s.io/api](https://github.com/kubernetes/api) | `0.33.1` | `0.33.2` | | [k8s.io/apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver) | `0.33.1` | `0.33.2` | | [k8s.io/apimachinery](https://github.com/kubernetes/apimachinery) | `0.33.1` | `0.33.2` | | [k8s.io/client-go](https://github.com/kubernetes/client-go) | `0.33.1` | `0.33.2` | | [k8s.io/code-generator](https://github.com/kubernetes/code-generator) | `0.33.1` | `0.33.2` | | [k8s.io/component-base](https://github.com/kubernetes/component-base) | `0.33.1` | `0.33.2` | Updates `k8s.io/api` from 0.33.1 to 0.33.2 - [Commits](kubernetes/api@v0.33.1...v0.33.2) Updates `k8s.io/apiextensions-apiserver` from 0.33.1 to 0.33.2 - [Release notes](https://github.com/kubernetes/apiextensions-apiserver/releases) - [Commits](kubernetes/apiextensions-apiserver@v0.33.1...v0.33.2) Updates `k8s.io/apimachinery` from 0.33.1 to 0.33.2 - [Commits](kubernetes/apimachinery@v0.33.1...v0.33.2) Updates `k8s.io/client-go` from 0.33.1 to 0.33.2 - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.33.1...v0.33.2) Updates `k8s.io/code-generator` from 0.33.1 to 0.33.2 - [Commits](kubernetes/code-generator@v0.33.1...v0.33.2) Updates `k8s.io/component-base` from 0.33.1 to 0.33.2 - [Commits](kubernetes/component-base@v0.33.1...v0.33.2) --- updated-dependencies: - dependency-name: k8s.io/api dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes - dependency-name: k8s.io/apiextensions-apiserver dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes - dependency-name: k8s.io/apimachinery dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes - dependency-name: k8s.io/client-go dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes - dependency-name: k8s.io/code-generator dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes - dependency-name: k8s.io/component-base dependency-version: 0.33.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: kubernetes ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> remove datastore dependency from the scheduler (kubernetes-sigs#1049) * remove datastore dependency from the scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * added back comments on snapshotting pods from datastore before calling schedule Signed-off-by: Nir Rozenbaum <[email protected]> * removed fake datastore from conformance scheduler test Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> Add subsetting logic for epp (kubernetes-sigs#981) feat: Added a factory function for the DecisionTree filter (kubernetes-sigs#1053) * Added a factory function for the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Added tests of the factory function of the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Registered the factory function of the DecisionTreeFilter Signed-off-by: Shmuel Kallner <[email protected]> * Refactored the configuration loading Signed-off-by: Shmuel Kallner <[email protected]> --------- Signed-off-by: Shmuel Kallner <[email protected]> Adding pprof endpoints to metrics port (kubernetes-sigs#1069) feat: Add a context.Context to the plugins.HAndle interface (kubernetes-sigs#1076) * Added a context.Context to the plugins.Handle interface Signed-off-by: Shmuel Kallner <[email protected]> * Changes due to changes in internal APIs Signed-off-by: Shmuel Kallner <[email protected]> * Changes to tests due to changes in internal APIs Signed-off-by: Shmuel Kallner <[email protected]> --------- Signed-off-by: Shmuel Kallner <[email protected]> convert subset filter from a plugin to logic in director (kubernetes-sigs#1088) * convert subset filter from a plugin to logic in director Signed-off-by: Nir Rozenbaum <[email protected]> * replace interface{} with any Signed-off-by: Nir Rozenbaum <[email protected]> * make linter happy Signed-off-by: Nir Rozenbaum <[email protected]> * address code review comments Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> chore(deps): bump golang.org/x/sync from 0.14.0 to 0.15.0 (kubernetes-sigs#1096) Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.14.0 to 0.15.0. - [Commits](golang/sync@v0.14.0...v0.15.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-version: 0.15.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Introduce plugins.TypedName to be used for Plugin base implementation (kubernetes-sigs#1086) * introduce TypedName to reduce boilerplate, modify plugins Signed-off-by: Etai Lev Ran <[email protected]> * implement GetTypedName() Signed-off-by: Etai Lev Ran <[email protected]> * Remove Type() and Name() from Plugin interface Signed-off-by: Etai Lev Ran <[email protected]> * use TypedName as private field, not embedded Signed-off-by: Etai Lev Ran <[email protected]> --------- Signed-off-by: Etai Lev Ran <[email protected]> move the conversion from pod metrics to scheduler pod representation one level up (kubernetes-sigs#1104) * move the converstion from pod metrics to scheduler pod representation one level up Signed-off-by: Nir Rozenbaum <[email protected]> * minor change in helper func Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> handle picking multiple destinations in scheduling layer (kubernetes-sigs#1059) * implement multiple destination as the output of the scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * updated max score picker unit tests to cover multiple pods Signed-off-by: Nir Rozenbaum <[email protected]> * imports Signed-off-by: Nir Rozenbaum <[email protected]> * unit-test fix Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]> refactor: 🔨 use the more explicit singular form (kubernetes-sigs#1129)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 24, 2025

k8s-ci-robot requested review from ahg-g and robscott June 24, 2025 08:31

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 24, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 24, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 24, 2025

k8s-ci-robot assigned liu-cong Jun 24, 2025

liu-cong reviewed Jun 24, 2025

View reviewed changes

shmuelk force-pushed the decision-tree-factory branch from daf6a59 to ad7f764 Compare June 25, 2025 11:57

liu-cong reviewed Jun 25, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2025

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2025

shmuelk closed this Jun 26, 2025

shmuelk force-pushed the decision-tree-factory branch from 4ce4092 to b4d1e67 Compare June 26, 2025 15:08

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2025

shmuelk added 4 commits June 26, 2025 18:12

Added a factory function for the DecisionTreeFilter

9e6223b

Signed-off-by: Shmuel Kallner <[email protected]>

Added tests of the factory function of the DecisionTreeFilter

b88054e

Signed-off-by: Shmuel Kallner <[email protected]>

Registered the factory function of the DecisionTreeFilter

5e38e72

Signed-off-by: Shmuel Kallner <[email protected]>

Refactored the configuration loading

293a4f0

Signed-off-by: Shmuel Kallner <[email protected]>

shmuelk reopened this Jun 26, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 26, 2025

k8s-ci-robot assigned nirrozenbaum Jun 26, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2025

k8s-ci-robot merged commit 8dc40a5 into kubernetes-sigs:main Jun 26, 2025
9 checks passed

shmuelk deleted the decision-tree-factory branch June 26, 2025 16:46

nirrozenbaum mentioned this pull request Jun 30, 2025

Current Configuration API doesn't support DecisionTreeFilter #1011

Closed

shmuelk mentioned this pull request Sep 25, 2025

REQUEST: New membership for shmuelk kubernetes/org#5864

Closed

11 tasks

feat: Added a factory function for the DecisionTree filter #1053

feat: Added a factory function for the DecisionTree filter #1053

Uh oh!

Conversation

shmuelk commented Jun 24, 2025

Uh oh!

netlify bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Jun 24, 2025

Uh oh!

ahg-g commented Jun 24, 2025

Uh oh!

ahg-g commented Jun 24, 2025

Uh oh!

liu-cong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmuelk Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmuelk Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmuelk commented Jun 25, 2025

Uh oh!

shmuelk commented Jun 25, 2025

Uh oh!

liu-cong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum commented Jun 25, 2025

Uh oh!

nirrozenbaum commented Jun 26, 2025

Uh oh!

shmuelk commented Jun 26, 2025

Uh oh!

shmuelk commented Jun 26, 2025

Uh oh!

nirrozenbaum commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

netlify bot commented Jun 24, 2025 •

edited

Loading

shmuelk Jun 25, 2025 •

edited

Loading

shmuelk Jun 25, 2025 •

edited

Loading