Fix wrong calculation for queue deserved in proportion plugin

[zionwu]

What this PR does / why we need it:

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 #729

[zionwu]

@hex108 @k82cn Could you please review this PR? Thanks.

[k82cn]

is it possible to add an e2e test for that?

[zionwu]

is it possible to add an e2e test for that?

Let me spend some time to see if it is possible to add e2e test.

[zionwu]

@k82cn, I have added the e2e test "Proportion".

Without my fix, the test will fail. You can verify at this CI Test: https://travis-ci.org/kubernetes-sigs/kube-batch/builds/535261793?utm_source=github_status&utm_medium=notification

With my fix, the test can pass, which you can check the latest CI result.

[hex108]

There has already been some problems caused by Less and LessEqual. The problems will be solved If the type MilliCPU, Memory and values of ScalarResources change from float64 to resource.Quantity or int64.

@k82cn What is your option?

[k82cn]

The problems will be solved If the type MilliCPU, Memory and values of ScalarResources change from float64 to resource.Quantity or int64.

Do you mean if we change to int64, this fix is not necessary?

[hex108]

I think so, because it is caused by LessEqual is not <= actually.

[k82cn]

LessEqual is not <= actually.

Can you share some case for that?

[zionwu]

In my opinion, this issue still exist even with resource.Quantity or int64.
The problem is that Resource is consist of MilliCPU, Memory and ScalarResources, and LessEqual requires all 3 members to be lessEqual:

func (r *Resource) LessEqual(rr *Resource) bool {
	isLess := (r.MilliCPU < rr.MilliCPU || math.Abs(rr.MilliCPU-r.MilliCPU) < minMilliCPU) &&
		(r.Memory < rr.Memory || math.Abs(rr.Memory-r.Memory) < minMemory)
	if !isLess {
		return false
	}

	if r.ScalarResources == nil {
		return true
	}

	for rName, rQuant := range r.ScalarResources {
		if rr.ScalarResources == nil {
			return false
		}

		rrQuant := rr.ScalarResources[rName]
		if !(rQuant < rrQuant || math.Abs(rrQuant-rQuant) < minMilliScalarResources) {
			return false
		}
	}

	return true
}

!attr.deserved.LessEqual(attr.request) is true when only one member of attr.deserved is larger than attr.request. But we need all 3 members of attr.deserved larger than attr.request. So we should always use attr.request.Less(attr.deserved) no matter what type of the 3 member is.

[hex108]

Agree with @zionwu , the error is because we use attr.request.LessEqual(attr.deserved) but we should use attr.request.Less(attr.deserved).

[hex108]

s/exiting/Exiting/

[zionwu]

Updated.

[k82cn]

LGTM overall, thanks 👍

[hex108]

/lgtm

for this PR.

[zionwu]

@k82cn Could you approve this PR? Thanks.

[k82cn]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, zionwu

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 [k82cn]

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