Skip to content

Conversation

nirrozenbaum
Copy link
Contributor

@nirrozenbaum nirrozenbaum commented Jul 12, 2025

This PR adds plugins configuration loading from file and is starting to handle some cleanup.
There are still follow up PRs that are needed to complete cleanup, but main path in cmd/runner and in e2e tests is complete.
additional minor changes were done to update some of the plugin names for readability purpose. for example random picker plugin name was called random, update to random-picker since it's not possible to understand what does plugin random means (same update was done to almost all plugins).

out of this PR and handled in follow up PRs to keep each PR scoped and relatively small and focused:

  • conformance test: should move the predictable filter (and its test) that is used in conformance tests to the same place as other plugins and register it in the plugins registry. after that is done, we should be able to load conformance config from file using configmap the exact same way that is done in this PR - done in load plugins from file in conformance tests #1152.
  • removal of NewScheduler function from scheduler.go. this includes updates in several places like integration tests and scheduler unit-test and may be a big PR on its own. therefore left this point to a separate PR which will be tightly scoped for this purpose - done in remove NewScheduler function #1153.
  • removal of initializeScheduler function from runner: after the two first points are handled, we can safely remove this function - done in remove NewScheduler function #1153.

after the above is done, we should make sure any additional env var loading, helper functions or variables that were used to initialize plugins config are all removed. none of those should exist in code anymore.

cc: @ahg-g @kfswain @elevran @shmuelk

Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2025
@k8s-ci-robot k8s-ci-robot requested review from ahg-g and robscott July 12, 2025 21:01
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2025
Copy link

netlify bot commented Jul 12, 2025

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

Name Link
🔨 Latest commit b54a49c
🔍 Latest deploy log https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68754e23cfe6f30008a27d42
😎 Deploy Preview https://deploy-preview-1151--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
Copy link
Contributor

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see the runner.go file getting lighter!

How did we test that the config file is generating the same hard coded configuration?

@nirrozenbaum
Copy link
Contributor Author

Great to see the runner.go file getting lighter!

How did we test that the config file is generating the same hard coded configuration?

@ahg-g I tested it manually, both options default + v2, and added some logging to this PR to validate that.

plugins:
- pluginRef: low-latency-filter
- pluginRef: random-picker
scheduler-v2.yaml: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably useful to use some prefix indicating that this will be removed, perhaps experimental-with-prefix-cache-scorer.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scheduler-v2 will not be removed, the default will be removed.
v2 will become the new default.

Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
@ahg-g
Copy link
Contributor

ahg-g commented Jul 14, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2025
@k8s-ci-robot k8s-ci-robot merged commit 0b141c6 into kubernetes-sigs:main Jul 14, 2025
9 checks passed
@nirrozenbaum nirrozenbaum deleted the plugins-from-file branch July 14, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants