Reintroduce Plugin.Name() #1057

elevran · 2025-06-24T17:40:18Z

This is a follow up to #1038.
Fixes #1008

Plugin changes (cc: @ahg-g @kfswain @nirrozenbaum)

Name() method to Plugin interface
Name() implementation for all Plugin implementations
Rename Factory function to FactoryFunc - similar to (e.g.,) http.HandlerFunc (to differentiate from http.Handler interface)

EPP Configuration changes, see 1038 comment (cc: @shmuelk @ahg-g)

Refactor field name from PluginName to Type
changes to config format to use type instead of pluginName
(unrelated) added comments on configuration test cases (since some names could be misleading)

netlify · 2025-06-24T17:40:23Z

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

Name	Link
🔨 Latest commit	`24382f3`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/685ba4b9a30e7d000860e9e0
😎 Deploy Preview	https://deploy-preview-1057--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-24T17:40:27Z

Hi @elevran. 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.

nirrozenbaum · 2025-06-24T17:41:56Z

@elevran needs rebase...

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

JSON tags are not changed yet as it would impact all test files/sample YAML. Will be in a follow up commit. Signed-off-by: Etai Lev Ran <elevran@gmail.com>

Added comments describing test beyond the already descriptive name (e.g., field Plugins was initially called PluginReference so names can be a little misleading when using PluginRef - it's not the scheduling profile's references). Removed redundant test case (coverage is still 100%). Signed-off-by: Etai Lev Ran <elevran@gmail.com>

Change the JSON tag on type Change test files and test cases' text to use type instead of pluginName Signed-off-by: Etai Lev Ran <elevran@gmail.com>

nirrozenbaum · 2025-06-24T18:18:17Z

@elevran I'm waiting with deeper review for a rebase.
but I do have a general comment.

I think the motivation behind this PR (and the previous one) was to support multiple Plugins of the same type with different instance names (e.g., for logging). this should be the case when using configuration file but also when configuring plugins via code.

I think current implementation has two problems:
first, it assumes name is used only when using a configuration file. configuring via code should allow the same.

second, it creates plugin not through the New function. this may be fine for plugins that have no logic in their New function but could be problematic for ones that do have some initialization. an example can be seen in the prefix plugin where the factory function doesn't use the New function, and this introduces a bug - in the New function there is a config validation if capacity <= 0 { while in the factory function we don't have it. meaning that using the factory function we might get negative value (this was not introduced by your PR, but now I caught it cause I saw you used the same pattern in other plugins).

More generally, I think all factory functions should call the matching New function to avoid these kind of bugs and to avoid code duplication (we don't want to do the same initialization code in both factory and new functions).
to solve the Name handling elegantly, I propose to use the .WithName pattern, create this function in each plugin.
and in the Name() function you can add a check, if name is empty, return Type().

This should allow us to configure name optionally via code (e.g., NewPrefixScorer().WithName("my-prefix-scorer")).

let me know if this makes sense.

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

elevran · 2025-06-24T18:31:01Z

More generally, I think all factory functions should call the matching New function to avoid these kind of bugs and to avoid code duplication (we don't want to do the same initialization code in both factory and new functions).
to solve the Name handling elegantly, I propose to use the .WithName pattern, create this function in each plugin.
and in the Name() function you can add a check, if name is empty, return Type().

Agree on both points. Will address this as part of this PR.

Plugins (except testing only plugins) now allow calling WithName() to assign their name. Facory functions work by calling New() to avoid code duplication and centralize parameter checking. When constructing an unnamed Plugin, its name defaults to Type() Signed-off-by: Etai Lev Ran <elevran@gmail.com>

elevran · 2025-06-24T19:25:03Z

@nirrozenbaum rebased and added WithName() as suggested - you can review

ahg-g · 2025-06-24T20:04:20Z

/ok-to-test

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

kfswain · 2025-06-24T20:20:21Z

this should be the case when using configuration file but also when configuring plugins via code.

Why should the two paths be different? If you are configuring via code, you can just build the literal object that the config API models, no?

nirrozenbaum · 2025-06-24T20:37:38Z

If you are configuring via code, you can just build the literal object that the config API models, no?

@kfswain it works the other way around.
when we create config api, we are building the object same as configuring via code.
config api will eventually get translated into code.
so instead of translating code -> config api -> back to code, the translation path is just config api -> code.

Why should the two paths be different?

they shouldn't, that was my comment :).
factoryFunc should call the New function to make sure both have similar behavior.

ahg-g · 2025-06-24T20:47:53Z

If you are configuring via code, you can just build the literal object that the config API models, no?

@kfswain it works the other way around. when we create config api, we are building the object same as configuring via code. config api will eventually get translated into code. so instead of translating code -> config api -> back to code, the translation path is just config api -> code.

+1; I don't think we need to provide a path for configuration other than the config api, unifying the ux on configuring the epp means we don't need to expose how the configuration get translated internally. So in main.go, NewRunner should have the option to accept an instance of a config api struct.

ahg-g · 2025-06-24T20:58:08Z

For what its worth, this PR looks good to me.

kfswain · 2025-06-24T21:08:34Z

so instead of translating code -> config api -> back to code, the translation path is just config api -> code.

Right, thats what I am suggesting. The config API is modeling a scheduler. You can build the scheduler as needed.

I guess I'm not understanding the need to make special consideration of configuring via code when that is the easier path, and not really impacted AFAICT

nirrozenbaum · 2025-06-24T21:12:45Z

For what its worth, this PR looks good to me.

@ahg-g lgtm from my side as well. 👍
left just one minor comment which spans across all plugins so it might be worth fixing it before merge.
I assume @elevran will handle this tomorrow at the latest.

pkg/epp/common/config/loader/configloader.go

pkg/epp/plugins/plugins.go

Set plugin name to its type explicitly in New() Remove empty name check from WithName() Refine error message in config loading (added plugin name for missing type) Signed-off-by: Etai Lev Ran <elevran@gmail.com>

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

elevran · 2025-06-25T07:27:41Z

@nirrozenbaum PTAL all suggested changes have been implemented

nirrozenbaum · 2025-06-25T07:43:30Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2025-06-25T07:43:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

* rename Factory function as FactoryFunc Signed-off-by: Etai Lev Ran <elevran@gmail.com> * Refactor field name from PluginName to Type JSON tags are not changed yet as it would impact all test files/sample YAML. Will be in a follow up commit. Signed-off-by: Etai Lev Ran <elevran@gmail.com> * Review test cases Added comments describing test beyond the already descriptive name (e.g., field Plugins was initially called PluginReference so names can be a little misleading when using PluginRef - it's not the scheduling profile's references). Removed redundant test case (coverage is still 100%). Signed-off-by: Etai Lev Ran <elevran@gmail.com> * Change pluginName to type in encoding Change the JSON tag on type Change test files and test cases' text to use type instead of pluginName Signed-off-by: Etai Lev Ran <elevran@gmail.com> * implement Name() for all plugins Signed-off-by: Etai Lev Ran <elevran@gmail.com> * Added WithName() to all plugins Plugins (except testing only plugins) now allow calling WithName() to assign their name. Facory functions work by calling New() to avoid code duplication and centralize parameter checking. When constructing an unnamed Plugin, its name defaults to Type() Signed-off-by: Etai Lev Ran <elevran@gmail.com> * implement review comments Set plugin name to its type explicitly in New() Remove empty name check from WithName() Refine error message in config loading (added plugin name for missing type) Signed-off-by: Etai Lev Ran <elevran@gmail.com> * change Name() comment Signed-off-by: Etai Lev Ran <elevran@gmail.com> --------- Signed-off-by: Etai Lev Ran <elevran@gmail.com>

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 nirrozenbaum and robscott June 24, 2025 17:40

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2025

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2025

elevran added 4 commits June 24, 2025 20:53

rename Factory function as FactoryFunc

6ed3cb6

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

Refactor field name from PluginName to Type

b6ed710

JSON tags are not changed yet as it would impact all test files/sample YAML. Will be in a follow up commit. Signed-off-by: Etai Lev Ran <elevran@gmail.com>

Change pluginName to type in encoding

491f01d

Change the JSON tag on type Change test files and test cases' text to use type instead of pluginName Signed-off-by: Etai Lev Ran <elevran@gmail.com>

implement Name() for all plugins

87ffd10

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

elevran force-pushed the plugin-name branch from 407aa87 to 87ffd10 Compare June 24, 2025 18:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2025

elevran changed the title ~~Reintroduce Plugin.Name()~~ [WIP] Reintroduce Plugin.Name() Jun 24, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2025

elevran changed the title ~~[WIP] Reintroduce Plugin.Name()~~ Reintroduce Plugin.Name() Jun 24, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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

nirrozenbaum reviewed Jun 24, 2025

View reviewed changes

pkg/epp/scheduling/framework/plugins/filter/least_kvcache_filter.go Outdated Show resolved Hide resolved

nirrozenbaum reviewed Jun 24, 2025

View reviewed changes

pkg/epp/scheduling/framework/plugins/filter/least_kvcache_filter.go Outdated Show resolved Hide resolved

nirrozenbaum reviewed Jun 25, 2025

View reviewed changes

pkg/epp/common/config/loader/configloader.go Outdated Show resolved Hide resolved

nirrozenbaum reviewed Jun 25, 2025

View reviewed changes

pkg/epp/plugins/plugins.go Outdated Show resolved Hide resolved

elevran added 2 commits June 25, 2025 10:16

implement review comments

5f25ebc

Set plugin name to its type explicitly in New() Remove empty name check from WithName() Refine error message in config loading (added plugin name for missing type) Signed-off-by: Etai Lev Ran <elevran@gmail.com>

change Name() comment

24382f3

Signed-off-by: Etai Lev Ran <elevran@gmail.com>

k8s-ci-robot assigned nirrozenbaum Jun 25, 2025

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2025

k8s-ci-robot merged commit fe5dea3 into kubernetes-sigs:main Jun 25, 2025
9 checks passed

elevran deleted the plugin-name branch June 25, 2025 12:18

elevran mentioned this pull request Jul 24, 2025

REQUEST: New membership for elevran kubernetes/org#5729

Closed

11 tasks

Reintroduce Plugin.Name() #1057

Reintroduce Plugin.Name() #1057

Uh oh!

Conversation

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

Uh oh!

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!

nirrozenbaum commented Jun 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

elevran commented Jun 24, 2025

Uh oh!

elevran commented Jun 24, 2025

Uh oh!

ahg-g commented Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nirrozenbaum commented Jun 24, 2025

Uh oh!

ahg-g commented Jun 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elevran commented Jun 25, 2025

Uh oh!

nirrozenbaum commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

elevran commented Jun 24, 2025 •

edited

Loading

netlify bot commented Jun 24, 2025 •

edited

Loading

nirrozenbaum commented Jun 24, 2025 •

edited

Loading

kfswain commented Jun 24, 2025 •

edited

Loading

ahg-g commented Jun 24, 2025 •

edited

Loading

kfswain commented Jun 24, 2025 •

edited

Loading

nirrozenbaum commented Jun 24, 2025 •

edited

Loading