Address reviewer feedback.

Author: LukeAVanDrie

pkg/epp/config/loader/configloader.go

@@ -52,17 +52,14 @@ func RegisterFeatureGate(gate string) {
 // LoadRawConfig parses the raw configuration bytes, applies initial defaults, and extracts feature gates.
 // It does not instantiate plugins.
 func LoadRawConfig(configBytes []byte, logger logr.Logger) (*configapi.EndpointPickerConfig, map[string]bool, error) {
-	// Decode JSON/YAML.
 	rawConfig, err := decodeRawConfig(configBytes)
 	if err != nil {
 		return nil, nil, err
 	}
-	logger.V(1).Info("Loaded raw configuration", "config", rawConfig)
+	logger.Info("Loaded raw configuration", "config", rawConfig)
 
-	// Sanitize data.
 	applyStaticDefaults(rawConfig)
 
-	// Early validation of Feature Gates.
 	// We validate gates early because they might dictate downstream loading logic.
 	if err := validateFeatureGates(rawConfig.FeatureGates); err != nil {
 		return nil, nil, fmt.Errorf("feature gate validation failed: %w", err)
@@ -80,23 +77,19 @@ func InstantiateAndConfigure(
 	logger logr.Logger,
 ) (*config.Config, error) {
 
-	// Instantiate user-configured plugins.
 	if err := instantiatePlugins(rawConfig.Plugins, handle); err != nil {
 		return nil, fmt.Errorf("plugin instantiation failed: %w", err)
 	}
 
-	// Fill in the architectural gaps (inject required system plugins).
 	if err := applySystemDefaults(rawConfig, handle); err != nil {
 		return nil, fmt.Errorf("system default application failed: %w", err)
 	}
 	logger.Info("Effective configuration loaded", "config", rawConfig)
 
-	// Deep validation checks relationships between now-finalized profiles and plugins.
 	if err := validateConfig(rawConfig); err != nil {
 		return nil, fmt.Errorf("configuration validation failed: %w", err)
 	}
 
-	// Build scheduler config.
 	schedulerConfig, err := buildSchedulerConfig(rawConfig.SchedulingProfiles, handle)
 	if err != nil {
 		return nil, fmt.Errorf("scheduler config build failed: %w", err)
@@ -150,7 +143,6 @@ func buildSchedulerConfig(
 
 	profiles := make(map[string]*framework.SchedulerProfile)
 
-	// Build profiles.
 	for _, cfgProfile := range configProfiles {
 		fwProfile := framework.NewSchedulerProfile()
 
@@ -178,7 +170,6 @@ func buildSchedulerConfig(
 		profiles[cfgProfile.Name] = fwProfile
 	}
 
-	// Find Profile Handler (singleton check).
 	var profileHandler framework.ProfileHandler
 	for name, plugin := range handle.GetAllPluginsWithNames() {
 		if ph, ok := plugin.(framework.ProfileHandler); ok {
@@ -194,7 +185,6 @@ func buildSchedulerConfig(
 		return nil, errors.New("no profile handler configured")
 	}
 
-	// Validate SingleProfileHandler usage.
 	if profileHandler.TypedName().Type == profile.SingleProfileHandlerType && len(profiles) > 1 {
 		return nil, errors.New("SingleProfileHandler cannot support multiple scheduling profiles")
 	}
@@ -203,27 +193,23 @@ func buildSchedulerConfig(
 }
 
 func loadFeatureConfig(gates configapi.FeatureGates) map[string]bool {
-	// Initialize with all false.
 	config := make(map[string]bool, len(registeredFeatureGates))
 	for gate := range registeredFeatureGates {
 		config[gate] = false
 	}
-	// Apply overrides.
 	for _, gate := range gates {
 		config[gate] = true
 	}
 	return config
 }
 
 func buildSaturationConfig(apiConfig *configapi.SaturationDetector) *saturationdetector.Config {
-	// 1. Initialize with Defaults
 	cfg := &saturationdetector.Config{
 		QueueDepthThreshold:       saturationdetector.DefaultQueueDepthThreshold,
 		KVCacheUtilThreshold:      saturationdetector.DefaultKVCacheUtilThreshold,
 		MetricsStalenessThreshold: saturationdetector.DefaultMetricsStalenessThreshold,
 	}
 
-	// 2. Apply Overrides (if valid)
 	if apiConfig != nil {
 		if apiConfig.QueueDepthThreshold > 0 {
 			cfg.QueueDepthThreshold = apiConfig.QueueDepthThreshold

pkg/epp/config/loader/configloader_test.go

@@ -141,7 +141,7 @@ func TestLoadRawConfiguration(t *testing.T) {
 	}
 }
 
-// --- Test: Phase 2 (Instantiation, Architecture Injection, Deep Validation) ---
+// --- Test: Phase 2 (Instantiation, System Defaulting, Deep Validation) ---
 
 func TestInstantiateAndConfigure(t *testing.T) {
 	// Not parallel because it modifies global plugin registry.
@@ -300,7 +300,7 @@ func TestInstantiateAndConfigure(t *testing.T) {
 	}
 }
 
-// Add this new test function to verify the builder logic specifically
+// Verify the SaturationConfig builder specifically.
 func TestBuildSaturationConfig(t *testing.T) {
 	t.Parallel()
 

pkg/epp/config/loader/defaults.go

@@ -31,43 +31,40 @@ const DefaultScorerWeight = 1
 
 var defaultScorerWeight = DefaultScorerWeight
 
-// applyStaticDefaults standardizes the configuration object before plugin instantiation.
-// It handles simple structural defaults that do not require knowledge of the plugin registry.
+// applyStaticDefaults sanitizes the configuration object before plugin instantiation.
+// It handles "Static" defaults: simple structural changes to the API object that do not require access to the plugin
+// registry.
 func applyStaticDefaults(cfg *configapi.EndpointPickerConfig) {
-	// Infer plugin names. If a plugin has a Type but no Name, the Type becomes the Name.
 	for idx, pluginConfig := range cfg.Plugins {
 		if pluginConfig.Name == "" {
 			cfg.Plugins[idx].Name = pluginConfig.Type
 		}
 	}
 
-	// Initialize feature gates.
 	if cfg.FeatureGates == nil {
 		cfg.FeatureGates = configapi.FeatureGates{}
 	}
 }
 
-// applySystemDefaults injects required architectural components that were omitted from the config.
-// It inspects the instantiated plugins (via the handle) and ensures the system graph is complete.
+// applySystemDefaults injects required components that were omitted from the config.
+// It handles "System" defaults: logic that requires inspecting instantiated plugins (via the handle) to ensure the
+// system graph is complete.
 func applySystemDefaults(cfg *configapi.EndpointPickerConfig, handle plugins.Handle) error {
 	allPlugins := handle.GetAllPluginsWithNames()
-
-	// Ensure the scheduling layer has profiles, pickers, and handlers.
-	if err := ensureSchedulingArchitecture(cfg, handle, allPlugins); err != nil {
+	if err := ensureSchedulingLayer(cfg, handle, allPlugins); err != nil {
 		return fmt.Errorf("failed to apply scheduling system defaults: %w", err)
 	}
-
 	return nil
 }
 
-// ensureSchedulingArchitecture guarantees that a valid scheduling profile exists and that all profiles have valid
-// Pickers and Handlers.
-func ensureSchedulingArchitecture(
+// ensureSchedulingLayer guarantees that the scheduling subsystem is structurally complete.
+// It ensures a valid profile exists and injects missing architectural components (like Pickers and ProfileHandlers) if
+// they are not explicitly configured.
+func ensureSchedulingLayer(
 	cfg *configapi.EndpointPickerConfig,
 	handle plugins.Handle,
 	allPlugins map[string]plugins.Plugin,
 ) error {
-	// Ensure at least one Scheduling Profile exists.
 	if len(cfg.SchedulingProfiles) == 0 {
 		defaultProfile := configapi.SchedulingProfile{Name: "default"}
 		// Auto-populate the default profile with all Filter, Scorer, and Picker plugins found.
@@ -80,7 +77,6 @@ func ensureSchedulingArchitecture(
 		cfg.SchedulingProfiles = []configapi.SchedulingProfile{defaultProfile}
 	}
 
-	// Ensure profile handler.
 	// If there is only 1 profile and no handler is explicitly configured, use the SingleProfileHandler.
 	if len(cfg.SchedulingProfiles) == 1 {
 		hasHandler := false
@@ -91,13 +87,12 @@ func ensureSchedulingArchitecture(
 			}
 		}
 		if !hasHandler {
-			if err := registerDefaultPlugin(cfg, handle, profile.SingleProfileHandlerType, profile.SingleProfileHandlerType); err != nil {
+			if err := registerDefaultPlugin(cfg, handle, profile.SingleProfileHandlerType); err != nil {
 				return err
 			}
 		}
 	}
 
-	// Ensure Picker(s) and Scorer weights.
 	// Find or Create a default MaxScorePicker to reuse across profiles.
 	var maxScorePickerName string
 	for name, p := range allPlugins {
@@ -106,32 +101,28 @@ func ensureSchedulingArchitecture(
 			break
 		}
 	}
-	// If no Picker exists anywhere, create one.
+
 	if maxScorePickerName == "" {
-		if err := registerDefaultPlugin(cfg, handle, picker.MaxScorePickerType, picker.MaxScorePickerType); err != nil {
+		if err := registerDefaultPlugin(cfg, handle, picker.MaxScorePickerType); err != nil {
 			return err
 		}
 		maxScorePickerName = picker.MaxScorePickerType
 	}
 
-	// Update profiles.
 	for i, prof := range cfg.SchedulingProfiles {
 		hasPicker := false
 		for j, pluginRef := range prof.Plugins {
 			p := handle.Plugin(pluginRef.PluginRef)
 
-			// Default Scorer weight.
 			if _, ok := p.(framework.Scorer); ok && pluginRef.Weight == nil {
 				cfg.SchedulingProfiles[i].Plugins[j].Weight = &defaultScorerWeight
 			}
 
-			// Check for Picker.
 			if _, ok := p.(framework.Picker); ok {
 				hasPicker = true
 			}
 		}
 
-		// Inject default Picker if missing.
 		if !hasPicker {
 			cfg.SchedulingProfiles[i].Plugins = append(
 				cfg.SchedulingProfiles[i].Plugins,
@@ -148,15 +139,14 @@ func ensureSchedulingArchitecture(
 func registerDefaultPlugin(
 	cfg *configapi.EndpointPickerConfig,
 	handle plugins.Handle,
-	name string,
 	pluginType string,
 ) error {
+	name := pluginType
 	factory, ok := plugins.Registry[pluginType]
 	if !ok {
 		return fmt.Errorf("plugin type '%s' not found in registry", pluginType)
 	}
 
-	// Instantiate with nil config (factory must handle defaults).
 	plugin, err := factory(name, nil, handle)
 	if err != nil {
 		return fmt.Errorf("failed to instantiate default plugin '%s': %w", name, err)