refactor: Standardize config loading and system default injection #1953
Conversation
Refactors the configuration loader to clarify the two-stage loading process and standardize default injection. This change: - Merges the loading lifecycle into a linear pipeline: Load Raw -> Instantiate -> Apply System Defaults -> Validate -> Build. - Introduces a `registerDefaultPlugin` helper to standardize how mandatory components (like MaxScorePicker and SingleProfileHandler) are injected when omitted by the user. - Improves error wrapping context throughout the loading process.
Rewrites `configloader_test.go` to use table-driven tests and improves overall test hygiene. Key improvements: - Separates tests for "Raw Loading" (Phase 1) and "Instantiation" (Phase 2) to mirror the new production architecture. - Adds comprehensive test data constants in `testdata.go` to prevent drift between YAML and Go struct definitions. - Adds deep validation callbacks to verify post-instantiation state, ensuring defaults are injected correctly.
Updates the EPP runner to use the refactored loader functions. The flow is updated to: 1. `loader.LoadRawConfig` to parse bytes and retrieve feature gates. 2. Initialize the plugin handle. 3. `loader.InstantiateAndConfigure` to build the final configuration.
k8s-ci-robot
added
the
kind/cleanup
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LukeAVanDrie The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @LukeAVanDrie. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
size/XXL
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
ahg-g
left a comment
ahg-g
left a comment
There was a problem hiding this comment.
Is there really a major change here other than deduping the saturation detection config defaulting (it was done in two places, now it is one place, which is good)?
| r.registerInTreePlugins() | ||
|
|
||
| rawConfig, featureGates, err := loader.LoadConfigPhaseOne(configBytes, logger) | ||
| rawConfig, featureGates, err := loader.LoadRawConfig(configBytes, logger) |
There was a problem hiding this comment.
nit: this is still run as phase one and phase two, see calling function names.
There was a problem hiding this comment.
Ack, I am scoping this change to config/loader package. Will clean this up in runner.go in a separate pass. For now, I just want these names to be semantically clearer. I have a draft for those runner.go changes, I can share the relevant snippet if you would like.
| cfg *configapi.EndpointPickerConfig, | ||
| handle plugins.Handle, | ||
| name string, | ||
| pluginType string, |
There was a problem hiding this comment.
nit: can pass the type only and default the name to the type
|
Not against it, nice cleanup, but want to confirm my read of the PR which mostly affirms the existing design with clearer naming of the phases |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| logger.V(1).Info("Loaded raw configuration", "config", rawConfig) |
There was a problem hiding this comment.
nit: can we use logger utils consts instead of numbers?
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR refactors the configuration loading architecture to standardize how the system instantiates plugins and injects mandatory architectural components (like ProfileHandlers and Pickers).
Reasoning:
Currently, configuration loading is split into "Phase 1" and "Phase 2" with ad-hoc mutation logic scattered between them. This refactor clearly scopes these phases and unifies the complex instantiation logic into a linear pipeline.
Key Changes:
LoadConfigPhaseOnetoLoadRawConfig(pure parsing/struct validation).LoadConfigPhaseTwointoInstantiateAndConfigure. This function now encapsulates the entire system construction workflow:Instantiate Plugins->Apply System Defaults->Deep Validate->Build Scheduler.MaxScorePickerandSingleProfileHandlerwith a unifiedregisterDefaultPluginpattern. This allows us to guarantee architectural integrity using generic code.configloader_test.goto use table-driven tests, proper parallelism, and stricter validation of post-instantiation state.Architectural Benefit:
This establishes the pattern required for promoting other hardcoded components (like the Saturation Detector) to plugins in the future. It moves the system from an imperative loading script to a declarative architectural assurance model.
Which issue(s) this PR fixes:
Tracks #1793 -- This makes future changes easier for modeling saturation control as a EPP default plugin rather than a standalone component (we require similar handling to WeightedScorer and default Picker).
Does this PR introduce a user-facing change?: