support fair share by lminzhw · Pull Request #358 · volcano-sh/volcano

lminzhw · 2019-07-19T02:17:28Z

Implementation of: https://github.com/volcano-sh/volcano/blob/master/docs/design/fairshare.md

support fair share based on weight among namespaces.

Part of: #244

TravisBuddy · 2019-07-19T02:19:17Z

Travis tests have failed

Hey @lminzhw,
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.

TravisBuddy Request Identifier: 83fd1f90-a9cb-11e9-b808-dd0e8a2f961c

k82cn · 2019-07-19T02:49:52Z

/cc @hex108 @Jeffwan @kinglion811 @jeefy

volcano-sh-bot · 2019-07-19T02:50:03Z

@k82cn: GitHub didn't allow me to request PR reviews from the following users: jeefy.

Note that only volcano-sh members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @hex108 @Jeffwan @kinglion811 @jeefy

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.

TravisBuddy · 2019-07-19T03:19:23Z

Travis tests have failed

Hey @lminzhw,
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.

TravisBuddy Request Identifier: ffa49300-a9d3-11e9-b808-dd0e8a2f961c

hzxuzhonghu · 2019-07-19T04:05:01Z

+	// map[queueId]PriorityQueue(namespaceName)
+	namespaceMap := map[api.QueueID]*util.PriorityQueue{}
+	// map[queueId]map[namespaceName]PriorityQueue(*api.JobInfo)
+	jobInNamespaceMap := map[api.QueueID]map[string]*util.PriorityQueue{}


Add more comments

add more comments to describe these two maps.

hzxuzhonghu · 2019-07-19T04:20:35Z

+}
+
+// AddResourceQuota add ResourceQuota to scheduler cache
+func (sc *SchedulerCache) AddResourceQuota(obj interface{}) {


I can not see where you filterout resource quota that does not set vaolcano.sh/xx.weight

The namespace without this label would default to 1

In itemFromQuota function, any ResourceQuota without xx.weight key would have weight with default value 1

hzxuzhonghu · 2019-07-19T07:01:19Z

+		namespaceAllocation := map[string]*api.Resource{}

 		for _, preemptee := range preemptees {
+			if namespaceOrderEnabled && preemptor.Namespace != preemptee.Namespace {


more comments

Better refactor this part. since the change is little too hardcoded.

refactor this part, namespace order policy is now a separate code block out of job order policy.

hzxuzhonghu

I think we need more tests, especially for namespace job allocate part.

Also i noticed a problem: The namespce may not get requested resources even when there is enough quota. Say we have queue1 and queue2, ns1 and ns2, all with weight 1. Jobs in ns1 (requires 50% of all resources) are using queue1 and queue2. Jobs in ns2(requires 50% all resources) are using queue2 only.

If queue order is queue2 ->queue1, then ns2 can only get 25% of the resources. Is this correct?

TommyLike · 2019-07-19T08:14:49Z

+}
+
+// AddResourceQuota add ResourceQuota to scheduler cache
+func (sc *SchedulerCache) AddResourceQuota(obj interface{}) {


The namespace without this label would default to 1

TommyLike · 2019-07-19T08:23:50Z

+	// TODO(lminzhw): if all NamespaceOrderFn treat these two namespace as the same,
+	// we should make the job order have its affect among namespaces.
+	// or just schedule namespace one by one
+	lv := l.(string)


Do we need to check if this converting failing?

this actions follows the code pattern in other CompareFn, like taskOrder or jobOrder in priority plugin.

TommyLike · 2019-07-19T08:26:34Z

+}
+
+// NamespaceOrderFn invoke namespaceorder function of the plugins
+func (ssn *Session) NamespaceOrderFn(l, r interface{}) bool {


So the order of namespace is decided by the first plugin if it has enabled the namespace order?

Yes, if some plugin decide namespace A is prior to B, this decision is just the final result.

This logic follows the code pattern in other CompareFn, like JobOrderFn.

TommyLike · 2019-07-19T08:39:22Z

+		namespaceAllocation := map[string]*api.Resource{}

 		for _, preemptee := range preemptees {
+			if namespaceOrderEnabled && preemptor.Namespace != preemptee.Namespace {


Better refactor this part. since the change is little too hardcoded.

TommyLike · 2019-07-19T08:44:21Z

I think we need more tests, especially for namespace job allocate part.

Also i noticed a problem: The namespce may not get requested resources even when there is enough quota. Say we have queue1 and queue2, ns1 and ns2, all with weight 1. Jobs in ns1 (requires 50% of all resources) are using queue1 and queue2. Jobs in ns2(requires 50% all resources) are using queue2 only.

If queue order is queue2 ->queue1, then ns2 can only get 25% of the resources. Is this correct?

ns2 would both get resource from q1 and q2 I guess so.

TravisBuddy · 2019-07-23T08:06:43Z

Travis tests have failed

Hey @lminzhw,
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.

TravisBuddy Request Identifier: cca67310-ad20-11e9-a8f1-bbdc807b8a3b

lminzhw · 2019-07-23T08:59:21Z

LGTM, but i would expect e2e tests as well.

We have 2 more e2e test cases and a XXL PR now. : )

hzxuzhonghu · 2019-07-23T09:26:31Z

@lminzhw Would suggest not squash all commits, it is hard to review.

hzxuzhonghu · 2019-07-23T10:06:49Z

 		err = waitJobPhaseReady(context, job)
 		Expect(err).NotTo(HaveOccurred())
 	})
+


Better add a case with both queue and namespace fair share?

This RP is too big now, I'll submit a new test next PR.

hex108 · 2019-07-24T13:07:21Z

+)
+
+// NamespaceName is name of namespace
+type NamespaceName string


Would Namespace be other types other than string? If not, it might be better to use string directly. Then it avoids many type conversion

I just want to make codes more readable, so choose to mark it as a new type but not mark it in comment.

hex108 · 2019-07-24T13:28:07Z

+		// pick namespace from namespaces PriorityQueue
+		namespace := namespaces.Pop().(api.NamespaceName)
+
+		queueInNamespace := jobsMap[namespace]


A little confused. What is the relationship between queue and namespace? According to the doc motivation, a namespace works like a user, and there might be many users(namespaces) in a queue. Will be many queues in a namespace?

@hex108 As I understand, generally speaking, different users (namespace) could schedule jobs in different Queues. This doc I think concentrates more on the faire resource sharing in one queue between users. This solution is mainly trying to resolve starvation problem in resource sharing.

hex108 · 2019-07-24T13:32:12Z

+		var queue *api.QueueInfo
+		for queueId := range queueInNamespace {
+			currentQueue := ssn.Queues[queueId]
+			if ssn.Overused(currentQueue) {


If the queue is overused, could not the queue get resource even the cluster has idle resource?

Yes, the overused queue will be skipped in master code too.

hex108 · 2019-07-24T13:39:57Z

 }

+// NamespaceOrderEnabled returns the NamespaceOrder for this plugin is enabled in this session or not
+func (drf *drfPlugin) NamespaceOrderEnabled(ssn *framework.Session) bool {


What is the difference between it and func (ssn *Session) NamespaceOrderEnabled()?

The NamespaceOrderEnabled in plugin just return the value of that plugin. This func return the result of this session and used in allocate action.

And in the newest version code, I delete this func and will open a Issue to adjust the behavior when the NamespaceOrder is disabled or other cases.

k82cn · 2019-07-26T00:47:01Z

/approve

Jeffwan · 2019-07-30T00:29:43Z

+		// pick namespace from namespaces PriorityQueue
+		namespace := namespaces.Pop().(api.NamespaceName)
+
+		queueInNamespace := jobsMap[namespace]


@hex108 As I understand, generally speaking, different users (namespace) could schedule jobs in different Queues. This doc I think concentrates more on the faire resource sharing in one queue between users. This solution is mainly trying to resolve starvation problem in resource sharing.

Jeffwan · 2019-07-30T00:33:52Z

+	// Name is the name of this namespace
+	Name NamespaceName
+	// Weight is the highest weight among many ResourceQuota.
+	Weight int64


if we use signed integer, what's the best practice here? Do we suggest users to always use weights > 0 or it could be negative numbers?

Yes, weight < 1 is meaningless in this case, because we have a default value which is 1. Any weight < 1 will be ignored.

volcano-sh-bot · 2019-07-30T00:36:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, k82cn, lminzhw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [k82cn]

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

TravisBuddy · 2019-08-23T03:42:26Z

Travis tests have failed

Hey @lminzhw,
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.

TravisBuddy Request Identifier: ef788370-c557-11e9-8712-75d78f9b457f

lminzhw · 2019-08-23T04:15:17Z

@ALL, sorry for the delayed reply.

And this PR leaves two unsolved problem now:

add a case with both Queue and Namespace fair share
the behavior when disable NamespaceOrder is not so good, I will adjust it.

Waiting for your response~

k82cn · 2019-08-26T06:11:18Z

/lgtm

help to re-label lgtm, and it LGTM to :)

volcano-sh-bot requested review from hzxuzhonghu and k82cn July 19, 2019 02:17

volcano-sh-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 19, 2019

lminzhw force-pushed the drf_fair_share branch from b754fd2 to a274368 Compare July 19, 2019 02:32

volcano-sh-bot requested review from Jeffwan, hex108 and kinglion811 July 19, 2019 02:49