Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Order gang scheduled jobs based on time of arrival #464

Merged
merged 3 commits into from
Nov 5, 2018

Conversation

adam-marek
Copy link
Contributor

What this PR does / why we need it:
The ordering of equal priority gang scheduled jobs is currently based on the job's namespace/name which is unintuitive when multiple users schedule jobs competing for the same resources.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #454

Special notes for your reviewer:

Release note:

Equal priority gang scheduled jobs will be ordered by their time of creation.

@k8s-ci-robot k8s-ci-robot requested a review from jinzhejz November 1, 2018 22:21
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 1, 2018
@k8s-ci-robot
Copy link
Contributor

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.


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 k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 1, 2018
@adam-marek
Copy link
Contributor Author

I am a member of the CNCF Intel Corporation group. My email is adam.marek@intel.com

@TravisBuddy
Copy link

Travis tests have failed

Hey @adam-marek,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make verify
go build -o _output/bin/deepcopy-gen ./cmd/deepcopy-gen/
_output/bin/deepcopy-gen -i ./pkg/apis/scheduling/v1alpha1/ -O zz_generated.deepcopy
hack/verify-gofmt.sh
diff -u ./pkg/scheduler/plugins/gang/gang.go.orig ./pkg/scheduler/plugins/gang/gang.go
--- ./pkg/scheduler/plugins/gang/gang.go.orig	2018-11-01 22:23:59.289164644 +0000
+++ ./pkg/scheduler/plugins/gang/gang.go	2018-11-01 22:23:59.289164644 +0000
@@ -122,7 +122,7 @@
 		if !lReady && !rReady {
 			if lv.PodGroup.GetCreationTimestamp().Time.Equal(rv.PodGroup.GetCreationTimestamp().Time) {
 				if lv.UID < rv.UID {
-					return -1;
+					return -1
 				}
 			} else if lv.PodGroup.GetCreationTimestamp().Time.Before(rv.PodGroup.GetCreationTimestamp().Time) {
 				return -1
make: *** [verify] Error 1
TravisBuddy Request Identifier: 50fc6850-de27-11e8-b084-61188e6c7303

@@ -120,7 +120,11 @@ func (gp *gangPlugin) OnSessionOpen(ssn *framework.Session) {
}

if !lReady && !rReady {
if lv.UID < rv.UID {
if lv.PodGroup.GetCreationTimestamp().Time.Equal(rv.PodGroup.GetCreationTimestamp().Time) {
Copy link
Contributor

@k82cn k82cn Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... we need to add a new field into JobInfo, the creation-timestamp is from PodGroup or PDB. And we will check jobInfo's creation time here. We need to support PDB as tf-operator is using it, we will remove it at least after 0.4 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something along these lines ? (3f0dfd8)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 4, 2018
@TravisBuddy
Copy link

Hey @adam-marek,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 5954b640-e08b-11e8-90db-29214a2b47d8

Copy link
Contributor

@k82cn k82cn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall! let’s get it merged when CI is happy :)

@k82cn
Copy link
Contributor

k82cn commented Nov 5, 2018

please sign cla

@k82cn
Copy link
Contributor

k82cn commented Nov 5, 2018

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adam-marek, k82cn

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2018
@adam-marek
Copy link
Contributor Author

CLA should be signed now

@k8s-ci-robot 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 Nov 5, 2018
@k8s-ci-robot k8s-ci-robot merged commit 5b1b769 into kubernetes-retired:master Nov 5, 2018
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
…ring

Order gang scheduled jobs based on time of arrival
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
…ring

Order gang scheduled jobs based on time of arrival
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
…ring

Order gang scheduled jobs based on time of arrival
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Order Job/Task by creation time
4 participants