chore(scheduler): use framework.Features in scheduler plugins #130414

googs1025 · 2025-02-25T13:34:21Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

use framework.Features in scheduler plugins

Which issue(s) this PR fixes:

Fixes #130406

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

k8s-ci-robot · 2025-02-25T13:34:32Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

macsko · 2025-02-25T14:14:18Z

pkg/scheduler/framework/plugins/noderesources/resource_allocation.go

@@ -37,6 +36,7 @@ type scorer func(args *config.NodeResourcesFitArgs) *resourceAllocationScorer
 // resourceAllocationScorer contains information to calculate resource allocation score.
 type resourceAllocationScorer struct {
 	Name string
+	fts  feature.Features


Let's create fields for specific features, i.e. enableInPlacePodVerticalScaling and enablePodLevelResources. See other plugins for reference

done. and also change other part

googs1025 · 2025-02-26T02:11:36Z

pkg/scheduler/framework/plugins/nodevolumelimits/utils.go

-
-	// The plugin name should be listed in the CSINode object annotation.
-	// This indicates that the plugin has been migrated to a CSI driver in the node.
-	csiNodeAnn := csiNode.GetAnnotations()


https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130414/pull-kubernetes-verify/1894553032322977792

this part seem to be unreachable code

haosdent · 2025-02-26T06:50:13Z

pkg/scheduler/framework/plugins/volumebinding/binder.go

@@ -1033,7 +1037,7 @@ func (a byPVCSize) Less(i, j int) bool {
 }

 // isCSIMigrationOnForPlugin checks if CSI migration is enabled for a given plugin.
-func isCSIMigrationOnForPlugin(pluginName string) bool {
+func isCSIMigrationOnForPlugin(enableCSIMigrationPortworx bool, pluginName string) bool {


Would it better that we adjust the order of args

Suggested change

func isCSIMigrationOnForPlugin(enableCSIMigrationPortworx bool, pluginName string) bool {

func isCSIMigrationOnForPlugin(pluginName string, enableCSIMigrationPortworx bool) bool {

googs1025 · 2025-02-26T08:33:09Z

/test pull-kubernetes-integration

macsko · 2025-02-26T08:35:19Z

pkg/scheduler/framework/plugins/nodevolumelimits/utils.go

 )

 // isCSIMigrationOn returns a boolean value indicating whether
 // the CSI migration has been enabled for a particular storage plugin.
-func isCSIMigrationOn(csiNode *storagev1.CSINode, pluginName string) bool {
+func isCSIMigrationOn(pluginName string, enableCSIMigrationPortworx bool, csiNode *storagev1.CSINode) bool {


Suggested change

func isCSIMigrationOn(pluginName string, enableCSIMigrationPortworx bool, csiNode *storagev1.CSINode) bool {

func isCSIMigrationOn(pluginName string, csiNode *storagev1.CSINode, enableCSIMigrationPortworx bool) bool {

macsko · 2025-02-26T09:40:40Z

pkg/scheduler/framework/plugins/nodevolumelimits/utils.go

-		if !utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationPortworx) {
-			return false
-		}
+		return enableCSIMigrationPortworx


Is it a correct change? Previously, if feature gate was enabled, annotations were checked. Was that check unnecessary?

Yes, I ignored this problem... I also rolled back the delete changes #130414 (comment)

macsko · 2025-02-26T10:37:35Z

pkg/scheduler/framework/plugins/nodevolumelimits/utils.go

 )

 // isCSIMigrationOn returns a boolean value indicating whether
 // the CSI migration has been enabled for a particular storage plugin.
-func isCSIMigrationOn(csiNode *storagev1.CSINode, pluginName string) bool {
+func isCSIMigrationOn(pluginName string, csiNode *storagev1.CSINode, enableCSIMigrationPortworx bool) bool {


nit: We could even do:

Suggested change

func isCSIMigrationOn(pluginName string, csiNode *storagev1.CSINode, enableCSIMigrationPortworx bool) bool {

func isCSIMigrationOn(csiNode *storagev1.CSINode, pluginName string, enableCSIMigrationPortworx bool) bool {

Not to change the order

googs1025 · 2025-02-26T11:21:49Z

/test pull-kubernetes-e2e-gce

macsko · 2025-02-26T11:33:48Z

/lgtm
I'll approve after second lgtm

k8s-ci-robot · 2025-02-26T11:33:56Z

LGTM label has been added.

Git tree hash: a837aa521c5e3de47bcecff552daeb3d10e1a4fc

haosdent · 2025-02-26T11:43:48Z

/lgtm

macsko · 2025-02-26T11:53:12Z

/approve

k8s-ci-robot · 2025-02-26T11:53:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: googs1025, macsko

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:

~~pkg/scheduler/OWNERS~~ [macsko]

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

googs1025 · 2025-02-26T12:18:17Z

/test pull-kubernetes-e2e-gce

k8s-ci-robot requested review from denkensk and mauriciopoppe February 25, 2025 13:34

macsko reviewed Feb 25, 2025

View reviewed changes

googs1025 force-pushed the chore/scheduler_features branch 2 times, most recently from 52c1ce2 to 94cf0ca Compare February 26, 2025 02:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2025

googs1025 commented Feb 26, 2025

View reviewed changes

googs1025 mentioned this pull request Feb 26, 2025

[BugFix]Update ImageLocality plugin to account ImageVolume images #130231

Open

haosdent reviewed Feb 26, 2025

View reviewed changes

googs1025 force-pushed the chore/scheduler_features branch from 94cf0ca to 9f769f4 Compare February 26, 2025 07:34

macsko reviewed Feb 26, 2025

View reviewed changes

googs1025 force-pushed the chore/scheduler_features branch from 9f769f4 to a0e500e Compare February 26, 2025 10:04

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2025

macsko reviewed Feb 26, 2025

View reviewed changes

chore(scheduler): use framework.Features in scheduler plugins

239aad8

googs1025 force-pushed the chore/scheduler_features branch from a0e500e to 239aad8 Compare February 26, 2025 11:16

k8s-ci-robot assigned macsko Feb 26, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2025

googs1025 requested a review from haosdent February 26, 2025 11:39

k8s-ci-robot assigned haosdent Feb 26, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2025

k8s-ci-robot merged commit 50ba483 into kubernetes:master Feb 26, 2025
15 checks passed

k8s-ci-robot added this to the v1.33 milestone Feb 26, 2025

	func isCSIMigrationOnForPlugin(enableCSIMigrationPortworx bool, pluginName string) bool {
	func isCSIMigrationOnForPlugin(pluginName string, enableCSIMigrationPortworx bool) bool {

	func isCSIMigrationOn(pluginName string, enableCSIMigrationPortworx bool, csiNode *storagev1.CSINode) bool {
	func isCSIMigrationOn(pluginName string, csiNode *storagev1.CSINode, enableCSIMigrationPortworx bool) bool {

	func isCSIMigrationOn(pluginName string, csiNode *storagev1.CSINode, enableCSIMigrationPortworx bool) bool {
	func isCSIMigrationOn(csiNode *storagev1.CSINode, pluginName string, enableCSIMigrationPortworx bool) bool {

chore(scheduler): use framework.Features in scheduler plugins #130414

chore(scheduler): use framework.Features in scheduler plugins #130414

Uh oh!

Conversation

googs1025 commented Feb 25, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googs1025 commented Feb 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googs1025 commented Feb 26, 2025

Uh oh!

macsko commented Feb 26, 2025

Uh oh!

k8s-ci-robot commented Feb 26, 2025

Uh oh!

haosdent commented Feb 26, 2025

Uh oh!

macsko commented Feb 26, 2025

Uh oh!

k8s-ci-robot commented Feb 26, 2025

Uh oh!

googs1025 commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!