Remove newScheduler for reducing complexity

[kerthcet]

Signed-off-by: kerthcet kerthcet@gmail.com

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

For readability and maintainability.

Which issue(s) this PR fixes:

Fixes #108788

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.:

[k8s-ci-robot]

@kerthcet: 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/test-infra repository.

[kerthcet]

/retest

[kerthcet]

cc @kubernetes/sig-scheduling-leads

[alculquicondor]

I think it's more common in the codebase that we use withX

[kerthcet]

I think it's more common in the codebase that we use withX

Yes, that's what came into my mind first. But I found we already have schedulerOptions under the package. It seems conflicts in names and easy to confuse.

This pattern is widely used in cli.

kubernetes/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go

Lines 730 to 733 in 9c9b290

	func (b *Builder) Latest() *Builder {
	b.latest = true
	return b
	}

[alculquicondor]

oh right... This seems to be another legacy leftover from the times when the scheduler was split in two objects.

We should just get rid of newScheduler instead (and migrate the tests that use it to New).

[Huang-Wei]

Is that newScheduler() is still present b/c all usecases need the 4 parameters? Or, you're going to get rid of this function as well?

[kerthcet]

Yes, these 4 parameters are widely used in summary.

[ahg-g]

I suggest the following: remove the newScheduler function and create an applyDefaultHandlers function for Scheduler that sets SchedulePod and FailureHandler.

In New and the tests, just create a Scheduler struct and invoke applyDefaultHandlers right after (unless the handlers needs to be set to something different).

[kerthcet]

oh right... This seems to be another legacy leftover from the times when the scheduler was split in two objects.

We should just get rid of newScheduler instead (and migrate the tests that use it to New).

It would be a little difficult, scheduler.New() helps to instantiate a scheduler with API-level options, like apiVersion, KubeSchedulerProfile, but the newScheduler here instantiates with internal parameters, they're from different layer. As a result, lots of unit tests depend on newScheduler to mock the server, I think it works well.

[alculquicondor]

I'm against adding more setup complexity just because unit tests use the low level builder.

I think changing the unit tests is the right direction.

[kerthcet]

I'm against adding more setup complexity just because unit tests use the low level builder.

I think changing the unit tests is the right direction.

In unit tests, scheduler should not care much about api level fields, e.g. in extender tests, we use the framework extender which is an interface, it's easy to mock the implementation

kubernetes/pkg/scheduler/extender_test.go

Lines 43 to 51 in 575031b

	func TestSchedulerWithExtenders(t *testing.T) {
	tests := []struct {
	name string
	registerPlugins []st.RegisterPluginFunc
	extenders []st.FakeExtender
	nodes []string
	expectedResult ScheduleResult
	expectsErr bool
	}{

But if we change to api lever extender, we have to define many fields, it's verbosely.

kubernetes/pkg/scheduler/apis/config/types.go

Lines 246 to 252 in 575031b

	type Extender struct {
	// URLPrefix at which the extender is available
	URLPrefix string
	// Verb for the filter call, empty if not supported. This verb is appended to the URLPrefix when issuing the filter call to extender.
	FilterVerb string
	// Verb for the preempt call, empty if not supported. This verb is appended to the URLPrefix when issuing the preempt call to extender.
	PreemptVerb string

[kerthcet]

To reduce the complexity, I'll follow @ahg-g advices, create a Scheduler directly.

[ahg-g]

please squash

[kerthcet]

Squashed @ahg-g

[ahg-g]

/approve
/hold
leaving lgtm to Wei or Aldo

[alculquicondor]

/lgtm
/approve
way better 😃

[alculquicondor]

/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, alculquicondor, kerthcet

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 [ahg-g,alculquicondor]

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