Merge pull request #143463 from jdzikowski/podgrouppostfilter-warning

KEP-5710: Log error when PostFilter plugin does not implement PodGroupPostFilterPlugin

pkg/scheduler/framework/runtime/framework.go

@@ -524,7 +524,8 @@ func NewFramework(ctx context.Context, r Registry, profile *config.KubeScheduler
 		f.computeBatchablePlugins()
 	}
 
-	// Put default preemption as the only RankNodesPlugin
+	// Put default preemption as the only RankNodesPlugin and check that all PostFilter
+	// plugins are also enabled at PodGroupPostFilter, as PodGroups only run the latter.
 	if utilfeature.DefaultFeatureGate.Enabled(features.GenericWorkload) {
 		hasPodGroupPostFilter := slices.ContainsFunc(f.podGroupPostFilterPlugins, func(pl fwk.PodGroupPostFilterPlugin) bool {
 			return pl.Name() == names.DefaultPreemption
@@ -541,6 +542,18 @@ func NewFramework(ctx context.Context, r Registry, profile *config.KubeScheduler
 		} else {
 			logger.V(2).Info("GenericWorkload is enabled, but default preemption plugin does not fulfill RankNodesPlugin interface. Node ranking will not be used.")
 		}
+
+		for _, pl := range f.postFilterPlugins {
+			if !slices.ContainsFunc(f.podGroupPostFilterPlugins, func(pgpl fwk.PodGroupPostFilterPlugin) bool {
+				return pgpl.Name() == pl.Name()
+			}) {
+				msg := "PostFilter plugin does not implement PodGroupPostFilterPlugin; it will not be run for PodGroups or CompositePodGroups"
+				if _, ok := pl.(fwk.PodGroupPostFilterPlugin); ok {
+					msg = "PostFilter plugin implements PodGroupPostFilterPlugin but is not enabled; it will not be run for PodGroups or CompositePodGroups"
+				}
+				logger.Error(nil, msg, "plugin", pl.Name(), "profile", f.profileName)
+			}
+		}
 	}
 
 	if options.captureProfile != nil {

pkg/scheduler/framework/runtime/framework_test.go

@@ -40,6 +40,7 @@ import (
 	clientsetfake "k8s.io/client-go/kubernetes/fake"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/component-base/metrics/testutil"
+	"k8s.io/klog/v2"
 	"k8s.io/klog/v2/ktesting"
 	fwk "k8s.io/kube-scheduler/framework"
 	"k8s.io/kubernetes/pkg/features"
@@ -72,6 +73,10 @@ const (
 	placementGeneratePlugin           = "placement-generate-plugin"
 	placementGeneratePlugin2          = "placement-generate-plugin-2"
 	defaultPreemptionPlugin           = names.DefaultPreemption
+	postFilterOnlyPlugin1             = "post-filter-only-plugin-1"
+	postFilterOnlyPlugin2             = "post-filter-only-plugin-2"
+	filterAndPostFilterPlugin         = "filter-and-post-filter-plugin"
+	bothPostFilterPlugin              = "post-filter-and-pod-group-post-filter-plugin"
 
 	testProfileName                   = "test-profile"
 	testPercentageOfNodesToScore      = 35
@@ -496,6 +501,10 @@ var registry = func() Registry {
 	r.Register(placementGeneratePlugin2, newTestPlacementGeneratePlugin2)
 	r.Register(placementScorePlugin1, newPlacementScorePluginFactory(placementScorePlugin1))
 	r.Register(defaultPreemptionPlugin, newMockDefaultPreemptionPlugin)
+	r.Register(postFilterOnlyPlugin1, newPostFilterOnlyPluginFactory(postFilterOnlyPlugin1))
+	r.Register(postFilterOnlyPlugin2, newPostFilterOnlyPluginFactory(postFilterOnlyPlugin2))
+	r.Register(filterAndPostFilterPlugin, newFilterAndPostFilterPlugin)
+	r.Register(bothPostFilterPlugin, newBothPostFilterPlugin)
 	return r
 }()
 
@@ -802,6 +811,220 @@ func TestPodGroupPostFilterPlugins(t *testing.T) {
 	}
 }
 
+func TestPostFilterWithoutPodGroupPostFilterWarning(t *testing.T) {
+	const (
+		wantNotImplementedMsg = "PostFilter plugin does not implement PodGroupPostFilterPlugin; it will not be run for PodGroups or CompositePodGroups"
+		wantNotEnabledMsg     = "PostFilter plugin implements PodGroupPostFilterPlugin but is not enabled; it will not be run for PodGroups or CompositePodGroups"
+	)
+	tests := []struct {
+		name                       string
+		genericWorkloadEnabled     bool
+		multiPointPlugins          []config.Plugin
+		filterPlugins              []config.Plugin
+		postFilterPlugins          []config.Plugin
+		podGroupPostFilterPlugins  []config.Plugin
+		podGroupPostFilterDisabled []config.Plugin
+		wantErrorLogs              []ktesting.LogEntry
+	}{
+		{
+			name:                   "GenericWorkload enabled, PostFilter plugin is also enabled at PodGroupPostFilter",
+			genericWorkloadEnabled: true,
+			postFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+			podGroupPostFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, PostFilter plugin implementing PodGroupPostFilterPlugin is enabled only at PostFilter",
+			genericWorkloadEnabled: true,
+			postFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+			wantErrorLogs: []ktesting.LogEntry{
+				{Type: ktesting.LogError, Message: wantNotEnabledMsg, ParameterKVList: []any{"plugin", defaultPreemptionPlugin, "profile", testProfileName}},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, PostFilter plugin does not implement PodGroupPostFilterPlugin",
+			genericWorkloadEnabled: true,
+			postFilterPlugins: []config.Plugin{
+				{Name: postFilterOnlyPlugin1},
+			},
+			wantErrorLogs: []ktesting.LogEntry{
+				{Type: ktesting.LogError, Message: wantNotImplementedMsg, ParameterKVList: []any{"plugin", postFilterOnlyPlugin1, "profile", testProfileName}},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, multiple PostFilter plugins with and without PodGroupPostFilterPlugin",
+			genericWorkloadEnabled: true,
+			postFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+				{Name: postFilterOnlyPlugin1},
+				{Name: postFilterOnlyPlugin2},
+			},
+			podGroupPostFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+			wantErrorLogs: []ktesting.LogEntry{
+				{Type: ktesting.LogError, Message: wantNotImplementedMsg, ParameterKVList: []any{"plugin", postFilterOnlyPlugin1, "profile", testProfileName}},
+				{Type: ktesting.LogError, Message: wantNotImplementedMsg, ParameterKVList: []any{"plugin", postFilterOnlyPlugin2, "profile", testProfileName}},
+			},
+		},
+		{
+			name:                   "GenericWorkload disabled, PostFilter plugin is also enabled at PodGroupPostFilter",
+			genericWorkloadEnabled: false,
+			postFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+			podGroupPostFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+		},
+		{
+			name:                   "GenericWorkload disabled, PostFilter plugin implementing PodGroupPostFilterPlugin is enabled only at PostFilter",
+			genericWorkloadEnabled: false,
+			postFilterPlugins: []config.Plugin{
+				{Name: defaultPreemptionPlugin},
+			},
+		},
+		{
+			name:                   "GenericWorkload disabled, PostFilter plugin does not implement PodGroupPostFilterPlugin",
+			genericWorkloadEnabled: false,
+			postFilterPlugins: []config.Plugin{
+				{Name: postFilterOnlyPlugin1},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, MultiPoint plugin does not implement PodGroupPostFilterPlugin",
+			genericWorkloadEnabled: true,
+			multiPointPlugins: []config.Plugin{
+				{Name: postFilterOnlyPlugin1},
+				{Name: bothPostFilterPlugin},
+			},
+			wantErrorLogs: []ktesting.LogEntry{
+				{Type: ktesting.LogError, Message: wantNotImplementedMsg, ParameterKVList: []any{"plugin", postFilterOnlyPlugin1, "profile", testProfileName}},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, MultiPoint plugin disabled at PodGroupPostFilter",
+			genericWorkloadEnabled: true,
+			multiPointPlugins: []config.Plugin{
+				{Name: bothPostFilterPlugin},
+			},
+			podGroupPostFilterDisabled: []config.Plugin{
+				{Name: bothPostFilterPlugin},
+			},
+			wantErrorLogs: []ktesting.LogEntry{
+				{Type: ktesting.LogError, Message: wantNotEnabledMsg, ParameterKVList: []any{"plugin", bothPostFilterPlugin, "profile", testProfileName}},
+			},
+		},
+		{
+			name:                   "GenericWorkload enabled, plugin implementing PostFilter is enabled only at Filter",
+			genericWorkloadEnabled: true,
+			filterPlugins: []config.Plugin{
+				{Name: filterAndPostFilterPlugin},
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericWorkload, tc.genericWorkloadEnabled)
+
+			logger := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true)))
+			ctx, cancel := context.WithCancel(klog.NewContext(context.Background(), logger))
+			defer cancel()
+
+			profile := config.KubeSchedulerProfile{
+				SchedulerName: testProfileName,
+				Plugins: &config.Plugins{
+					MultiPoint: config.PluginSet{
+						Enabled: tc.multiPointPlugins,
+					},
+					Filter: config.PluginSet{
+						Enabled: tc.filterPlugins,
+					},
+					PostFilter: config.PluginSet{
+						Enabled: tc.postFilterPlugins,
+					},
+					PodGroupPostFilter: config.PluginSet{
+						Enabled:  tc.podGroupPostFilterPlugins,
+						Disabled: tc.podGroupPostFilterDisabled,
+					},
+				},
+			}
+			if _, err := newFrameworkWithQueueSortAndBind(ctx, registry, profile); err != nil {
+				t.Fatalf("Unexpected framework initialization error: %v", err)
+			}
+			var gotErrorLogs []ktesting.LogEntry
+			for _, entry := range logger.GetSink().(ktesting.Underlier).GetBuffer().Data() {
+				if entry.Type == ktesting.LogError {
+					gotErrorLogs = append(gotErrorLogs, entry)
+				}
+			}
+			if diff := cmp.Diff(tc.wantErrorLogs, gotErrorLogs, cmpopts.IgnoreFields(ktesting.LogEntry{}, "Timestamp")); diff != "" {
+				t.Errorf("Unexpected error logs (-want,+got):\n%s", diff)
+			}
+		})
+	}
+}
+
+type testPostFilterOnlyPlugin struct {
+	name string
+}
+
+func (pl *testPostFilterOnlyPlugin) Name() string {
+	return pl.name
+}
+
+func (pl *testPostFilterOnlyPlugin) PostFilter(_ context.Context, _ fwk.CycleState, _ *v1.Pod, _ fwk.NodeToStatusReader) (*fwk.PostFilterResult, *fwk.Status) {
+	return nil, nil
+}
+
+var _ fwk.PostFilterPlugin = &testPostFilterOnlyPlugin{}
+
+func newPostFilterOnlyPluginFactory(name string) func(context.Context, runtime.Object, fwk.Handle) (fwk.Plugin, error) {
+	return func(context.Context, runtime.Object, fwk.Handle) (fwk.Plugin, error) {
+		return &testPostFilterOnlyPlugin{name: name}, nil
+	}
+}
+
+// testFilterAndPostFilterPlugin implements FilterPlugin and PostFilterPlugin, but not PodGroupPostFilterPlugin.
+type testFilterAndPostFilterPlugin struct {
+	testPostFilterOnlyPlugin
+}
+
+func (pl *testFilterAndPostFilterPlugin) Filter(_ context.Context, _ fwk.CycleState, _ *v1.Pod, _ fwk.NodeInfo) *fwk.Status {
+	return nil
+}
+
+var (
+	_ fwk.FilterPlugin     = &testFilterAndPostFilterPlugin{}
+	_ fwk.PostFilterPlugin = &testFilterAndPostFilterPlugin{}
+)
+
+func newFilterAndPostFilterPlugin(_ context.Context, _ runtime.Object, _ fwk.Handle) (fwk.Plugin, error) {
+	return &testFilterAndPostFilterPlugin{testPostFilterOnlyPlugin: testPostFilterOnlyPlugin{name: filterAndPostFilterPlugin}}, nil
+}
+
+// testBothPostFilterPlugin implements PostFilterPlugin and PodGroupPostFilterPlugin only, so
+// MultiPoint expansion does not add it to QueueSort or Bind.
+type testBothPostFilterPlugin struct {
+	testPostFilterOnlyPlugin
+}
+
+func (pl *testBothPostFilterPlugin) PodGroupPostFilter(_ context.Context, _ fwk.PodGroupCycleState, _ fwk.PodGroupInfo, _ fwk.PodGroupSchedulingFunc) (*fwk.PodGroupPostFilterResult, *fwk.Status) {
+	return nil, nil
+}
+
+var _ fwk.PodGroupPostFilterPlugin = &testBothPostFilterPlugin{}
+
+func newBothPostFilterPlugin(_ context.Context, _ runtime.Object, _ fwk.Handle) (fwk.Plugin, error) {
+	return &testBothPostFilterPlugin{testPostFilterOnlyPlugin: testPostFilterOnlyPlugin{name: bothPostFilterPlugin}}, nil
+}
+
 // mockDefaultPreemptionPlugin is used to simulate a default preemption plugin that implements PodGroupPostFilterPlugin
 type mockDefaultPreemptionPlugin struct {
 	TestPlugin