Added backfill #805

pdgetrf · 2019-04-19T06:31:45Z

Added backfill action that allows lower priority jobs to run when higher priority jobs cannot run but still hold resources

Tested with added unit tests, e2e tests and local manual test

k8s-ci-robot · 2019-04-19T06:31:47Z

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 email the CNCF helpdesk: helpdesk@rt.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 · 2019-04-19T06:31:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pdgetrf
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: k82cn

If they are not already assigned, you can assign the PR to them by writing /assign @k82cn in a comment when ready.

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:

OWNERS

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

k8s-ci-robot · 2019-04-19T06:31:58Z

Hi @pdgetrf. Thanks for your PR.

I'm waiting for a kubernetes-sigs or 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.

pdgetrf · 2019-04-19T06:38:55Z

signed CLA

config/kube-batch-conf.yaml

k82cn · 2019-04-22T01:06:41Z

btw, please also open a PR on design doc.

k82cn · 2019-04-22T01:07:32Z

and please also append the review comments last time and highlight how those comments were addressed.

dhzhuo · 2019-04-22T22:03:07Z

@k82cn Design doc of backfill and starvation prevention can be found in PR 821.

pkg/scheduler/actions/backfill/backfill.go

k82cn · 2019-04-30T01:42:22Z

pkg/scheduler/framework/session.go

+					defer wg.Done()
+
+					annotation := map[string]string{v1alpha1.BackfillAnnotationKey: "true"}
+					err := ssn.PatchAnnotation(pendingTask, annotation)


The annotation should be also cached, there maybe out of sync between scheduler cache & apiserver.

pkg/scheduler/framework/session_plugins.go

k82cn · 2019-04-30T01:43:55Z

pkg/scheduler/framework/session_plugins.go

+func (ssn *Session) BackFillEligible(obj interface{}) bool {
+	for _, tier := range ssn.Tiers {
+		for _, plugin := range tier.Plugins {
+			jrf, found := ssn.backFillEligibleFns[plugin.Name]


should be also configurable

"configurable" as in "can be configured to turn on and off"?

Backfill function can be enabled/disabled from configurations. BackfillEligible is a function provided by each plugin to decide which job is eligible to be a backfill job. What is the use case to conditionally turn on/off BackfillEligible function?

What is the use case to conditionally turn on/off BackfillEligible function?

Different plugin may provide this callback, we need a configuration to controll it.

pkg/scheduler/scheduler.go

k82cn · 2019-04-30T01:52:27Z

pkg/scheduler/util.go


 	"github.com/kubernetes-sigs/kube-batch/pkg/scheduler/conf"
 	"github.com/kubernetes-sigs/kube-batch/pkg/scheduler/framework"
 	"github.com/kubernetes-sigs/kube-batch/pkg/scheduler/plugins"
+	"gopkg.in/yaml.v2"


keep the original format.

what's original format?

"io/ioutil" "gopkg.in/yaml.v2" "github.com/kubernetes-sigs/kube-batch/pkg/scheduler/conf"

don't we have tools to enforce this? oh right, it's reverted!

will wait the tool is brought back to fix this. this shouldn't be fixed by hand.

pkg/scheduler/util.go

pdgetrf · 2019-05-01T00:29:51Z

still working on a few issues. the e2e tests from the previous rollback in the master branch (not related to this PR) wrecked a mess. attempting to fix it now at least for backfill.

k82cn · 2019-05-02T00:12:15Z

pkg/scheduler/framework/session.go

+		case err := <-errChannel:
+			if err != nil {
+				fmt.Println("error ", err)
+				return err


What happen if Pod is patched but did not dispatch because of other error?

If dispatch fails, the pod remains unbound in the current scheduling round, and therefore will not be started. In the next scheduling round, when we create TaskInfo for a Pod in NewTaskInfo, we will noticed that the Pod is unbound but has backfill annotation. In this case, we will remove backfill annotation from the Pod.

@pdgetrf It looks to me that we do not yet have the logic to remove backfill annotation in NewTaskInfo.

In this case, we will remove backfill annotation from the Pod.

It seems not implemented in this PR, right?

this can be fixed in a following up PR which, as you already suggested, is smaller and easier to review. I do not see how this should block the current PR. It will only make this one even bigger. And we are going to address this in the starvation PR anyway. And backfill will be disabled in this PR anyway. If your purpose is to further delay the starvation PR including all the necessary testing, yes I agree we should fix it here in this PR.

What happen if Pod is patched but did not dispatch because of other error?

code has the answer.

pkg/scheduler/util.go

…iority jobs cannot run but still hold resources

k8s-ci-robot · 2019-05-18T23:27:17Z

@pdgetrf: PR needs rebase.

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.

fejta-bot · 2019-08-16T23:42:58Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-09-16T00:27:04Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-10-16T01:11:52Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-10-16T01:12:00Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 19, 2019

k8s-ci-robot requested review from animeshsingh and hex108 April 19, 2019 06:31

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 19, 2019

pdgetrf force-pushed the backfill branch from a207b97 to ace750f Compare April 19, 2019 06:50

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2019

pdgetrf force-pushed the backfill branch 4 times, most recently from cf61bce to bb6056e Compare April 19, 2019 23:23

k82cn reviewed Apr 20, 2019

View reviewed changes

config/kube-batch-conf.yaml Outdated Show resolved Hide resolved

dhzhuo mentioned this pull request Apr 22, 2019

Added design doc for backfill and starvation prevention #821

Closed

pdgetrf force-pushed the backfill branch from d683683 to a5335e6 Compare April 23, 2019 03:40

dhzhuo mentioned this pull request Apr 24, 2019

[Features] improve scheduling performance on batch jobs #492

Closed

pdgetrf force-pushed the backfill branch 4 times, most recently from d18c098 to 457a2d3 Compare April 26, 2019 02:32

dhzhuo reviewed Apr 26, 2019

View reviewed changes

pkg/scheduler/actions/backfill/backfill.go Outdated Show resolved Hide resolved