scheduler: merge Reserve and Unreserve plugins #92200

adtac · 2020-06-16T20:54:51Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Previously, separate interfaces were defined for Reserve and Unreserve
plugins. However, in nearly all cases, a plugin that allocates a
resource using Reserve will likely want to register itself for Unreserve
as well in order to free the allocated resource at the end of a failed
scheduling/binding cycle. Having separate plugins for Reserve and
Unreserve also adds unnecessary config toil. To that end, this patch
aims to merge the two plugins into a single interface called a
ReservationPlugin that requires implementing both the Reserve and
Unreserve methods.

Which issue(s) this PR fixes:

Fixes #90724

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Unreserve extension point for scheduler plugins is merged into Reserve extension point

k8s-ci-robot · 2020-06-16T20:54:55Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-ci-robot · 2020-06-16T20:54:59Z

Welcome @adtac!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-06-16T20:55:00Z

Hi @adtac. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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

adtac · 2020-06-16T21:06:55Z

CLA signed.

alculquicondor · 2020-06-16T21:08:31Z

/ok-to-test

alculquicondor · 2020-06-16T21:14:42Z

pkg/scheduler/apis/config/types.go

@@ -192,8 +192,8 @@ type Plugins struct {
 	// Score is a list of plugins that should be invoked when ranking nodes that have passed the filtering phase.
 	Score *PluginSet

-	// Reserve is a list of plugins invoked when reserving a node to run the pod.
-	Reserve *PluginSet
+	// Reservation is a list of plugins invoked when reserving and unreserving a node to run the pod.


I would prefer that we keep the name Reserve to avoid increasing the backwards-incompatible changes from v1alpha2 to v1beta1.
I don't think the name Reserve is deceiving. We have Score, which has 2 functions: Score and NormalizeScore.

WDYT @ahg-g @Huang-Wei ?

keeping the name Reserve sgtm.

pkg/scheduler/framework/plugins/examples/multipoint/multipoint.go

pkg/scheduler/framework/v1alpha1/framework.go

pkg/scheduler/framework/v1alpha1/interface.go

alculquicondor · 2020-06-16T21:29:42Z

pkg/scheduler/scheduler.go

-	// Run "reserve" plugins.
-	if sts := prof.RunReservePlugins(schedulingCycleCtx, state, assumedPod, scheduleResult.SuggestedHost); !sts.IsSuccess() {
+	// Run the Reserve method of reservation plugins.
+	if sts := prof.RunReserveReservationPlugins(schedulingCycleCtx, state, assumedPod, scheduleResult.SuggestedHost); !sts.IsSuccess() {
 		sched.recordSchedulingFailure(prof, assumedPodInfo, sts.AsError(), SchedulerError, "")


Here we should call Unreserve #83557

We will need a test for it.

I'd like to implement the merge of the two plugins first and then handle the case of failing reserves later in a separate PR to keep this PR small, atomic, and contained to one issue.

Could I recreate #83557 after this PR will be merged?

@adtac could take it, since he is already modifying the code here. Unless you @mrkm4ntr have some specific need?

I'd like to see this feature to completion, but if you'd strongly like to work on that feature after this PR, I don't mind :)

Oh, I misunderstood. I have nothing to do :)

adtac · 2020-06-23T19:00:04Z

The go fmt errors seem to be unrelated to my PR?

alculquicondor · 2020-06-23T19:11:36Z

Probably #92402 ?

adtac · 2020-06-23T19:52:13Z

staticcheck is failing, but gofmt is failing too and it seems like that issue doesn't cover that

adtac · 2020-06-23T20:00:08Z

/retest

alculquicondor · 2020-06-24T19:56:40Z

/retest

Previously, separate interfaces were defined for Reserve and Unreserve plugins. However, in nearly all cases, a plugin that allocates a resource using Reserve will likely want to register itself for Unreserve as well in order to free the allocated resource at the end of a failed scheduling/binding cycle. Having separate plugins for Reserve and Unreserve also adds unnecessary config toil. To that end, this patch aims to merge the two plugins into a single interface called a ReservePlugin that requires implementing both the Reserve and Unreserve methods.

alculquicondor · 2020-06-24T21:22:54Z

/lgtm

alculquicondor · 2020-06-24T22:30:33Z

/hold cancel

/retest

fejta-bot · 2020-06-25T01:45:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.