Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending RequestedToCapacityRatio priority function to support resource bin packing of extended resources #77688

Merged

Conversation

@sudeshsh
Copy link
Contributor

commented May 9, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Extend RequestedToCapacityRatio Priority Function to allow users to use the best fit polices during scheduling. It will allow users to apply bin packing on core resources like CPU, Memory as well as extended resources like accelerators.

Which issue(s) this PR fixes:

Fixes ##68036
#kubernetes/enhancements#964

Special notes for your reviewer:
Config file for enabling bin packing

{
	"kind": "Policy",
	"apiVersion": "v1",
	"predicates": [{
			"name": "PodFitsHostPorts"
		},
		{
			"name": "PodFitsResources"
		},
		{
			"name": "NoDiskConflict"
		},
		{
			"name": "MatchNodeSelector"
		},
		{
			"name": "HostName"
		}
	],
	"priorities": [{
			"name": "LeastRequestedPriority",
			"weight": 1
		},
		{
			"name": "BalancedResourceAllocation",
			"weight": 1
		},
		{
			"name": "ServiceSpreadingPriority",
			"weight": 1
		},
		{
			"name": "EqualPriority",
			"weight": 1
		},
		{
			"name": "RequestedToCapacityRatioPriority",
			"weight": 2,
			"argument": {
				"requestedToCapacityRatioArguments": {
					"shape": [{
							"utilization": 0,
							"score": 0
						},
						{
							"utilization": 100,
							"score": 10
						}
					],
					"resources": [{
							"name": "intel.com/foo",
							"weight": 3
						},
						{
							"name": "intel.com/bar",
							"weight": 5
						}
					]
				}
			}
		}
	],
	"hardPodAffinitySymmetricWeight": 10
}

Patch Nodes

curl --header "Content-Type: application/json-patch+json" \
--request PATCH \
--data '[{"op": "add", "path": "/status/capacity/intel.com~1foo", "value": "4"}]' \
http://localhost:8001/api/v1/nodes/<node-name>/status

Does this PR introduce a user-facing change?:

Updates the requestedToCapacityRatioArguments to add resources parameter that allows the users to specify the resource name along with weights for each resource to score nodes based on the request to capacity ratio.   

kubernetes/website PR kubernetes/website#15990

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Hi @sudeshsh. 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.

@k82cn

This comment has been minimized.

Copy link
Member

commented May 11, 2019

/assign

@k82cn

This comment has been minimized.

Copy link
Member

commented May 11, 2019

/ok-to-test

@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from 8d163b0 to 42b7e63 May 13, 2019
@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels May 13, 2019
@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from 42b7e63 to d846d9b May 13, 2019
@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from 817255a to 21698ce May 20, 2019
@sudeshsh sudeshsh changed the title [WIP] initial implementation of ExtendingRequestedToCapacityRatio priority function to support resource bin packing of extended resources initial implementation of ExtendingRequestedToCapacityRatio priority function to support resource bin packing of extended resources May 20, 2019
@sudeshsh sudeshsh changed the title initial implementation of ExtendingRequestedToCapacityRatio priority function to support resource bin packing of extended resources [WIP]initial implementation of ExtendingRequestedToCapacityRatio priority function to support resource bin packing of extended resources May 20, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@sudeshsh: Those labels are not set on the issue: sig/testing, sig/, sig/storage, sig/api-machinery, sig/cli

In response to this:

/remove-sig testing
/remove-sig storage
/remove-sig api-machinery
/remove-sig cli
/remove-sig cluster-lifecycle

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

This comment has been minimized.

Copy link

commented Aug 26, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Copy link
Member

left a comment

few more comments, sorry about this.

Btw, I ran the benchmark, this PR has no performance impact, which is good.

pkg/scheduler/algorithm/priorities/least_requested.go Outdated Show resolved Hide resolved
pkg/scheduler/algorithm/priorities/most_requested.go Outdated Show resolved Hide resolved
pkg/scheduler/algorithm/priorities/least_requested.go Outdated Show resolved Hide resolved
pkg/scheduler/algorithm/priorities/most_requested.go Outdated Show resolved Hide resolved
type ResourceToWeightMap map[v1.ResourceName]int64

// ResourceToValueMap contains resource name and score.
type ResourceToValueMap map[v1.ResourceName]int64

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 27, 2019

Member

we don't need to export this, right? s/ResourceToValueMap/resourceToValueMap

Copy link
Member

left a comment

Comments below. Will take another round of review later.

pkg/scheduler/api/compatibility/compatibility_test.go Outdated Show resolved Hide resolved
pkg/scheduler/algorithm/priorities/least_requested.go Outdated Show resolved Hide resolved
if r.ResourceToWeightMap == nil {
return schedulerapi.HostPriority{}, fmt.Errorf("resources not found")
}
requested := make(ResourceToValueMap, len(r.ResourceToWeightMap))

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 27, 2019

Member

Add a TODO here: we should benchmark this to make us comfortable that it doesn't bring significant overhead.

@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from 7b1f55c to a7e44ce Aug 27, 2019
@@ -150,7 +150,8 @@ type LabelPreference struct {
// RequestedToCapacityRatioArguments holds arguments specific to RequestedToCapacityRatio priority function
type RequestedToCapacityRatioArguments struct {
// Array of point defining priority function shape
UtilizationShape []UtilizationShapePoint
UtilizationShape []UtilizationShapePoint
ResourceToWeightMap []ResourceToWeight

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 27, 2019

Member

s/ResourceToWeightMap/Resources

@@ -161,6 +162,14 @@ type UtilizationShapePoint struct {
Score int
}

// ResourceToWeight represents single resource for bin packing of priority RequestedToCapacityRatioArguments
type ResourceToWeight struct {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 27, 2019

Member

s/ResourceToWeight/ResourceSpec

type ResourceToWeight struct {
// Name of the resources to be managed by RequestedToCapacityRatio function
Name v1.ResourceName
// Weight of the resources

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 27, 2019

Member

s/resources/resource

also, end with a period here and the comments above.

@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

/approve
/lgtm
/hold

@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from a7e44ce to 451910b Aug 27, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 27, 2019
…rce bin packing of extended resources
@sudeshsh sudeshsh force-pushed the sudeshsh:extended_resource_bin_packing branch from 451910b to 9ae5059 Aug 27, 2019
@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Aug 27, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, sudeshsh

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

@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

/test pull-kubernetes-conformance-kind-ipv6

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@sudeshsh: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-conformance-kind-ipv6 9ae5059 link /test pull-kubernetes-conformance-kind-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@sudeshsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 668bf42 into kubernetes:master Aug 28, 2019
24 checks passed
24 checks passed
cla/linuxfoundation sudeshsh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.