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

Move resource-based priority functions to their Score plugins #86725

Merged
merged 1 commit into from Dec 31, 2019

Conversation

@notpad
Copy link
Member

notpad commented Dec 30, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Move resource_allocation.go to plugins/noderesources/ and move the logic in Least/Most/Balanced and RequestedToCapacityRatio logic to their Score plugins

Which issue(s) this PR fixes:

Fixes #86445

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/sig scheduling
/priority important-soon
/assign @ahg-g

Copy link
Member

ahg-g left a comment

Thanks, this is great.

Please address the comments in a separate commit (i.e., don't amend).

@@ -49,7 +49,6 @@ var DefaultRequestedRatioResources = ResourceToWeightMap{v1.ResourceMemory: 1, v
// It will use `scorer` function to calculate the score.
func (r *ResourceAllocationPriority) PriorityMap(

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

lets name it score (make it local)

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package priorities
package noderesources

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

let change ResourceAllocationPriority to just resourceAllocationScorer (i.e., make it local to the package as well as drop the Priority suffix)

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

So, I would rearrange the types/variables in this files as follows (public ones first and then the package local ones):

// ResourceToWeightMap contains resource name and weight.
type ResourceToWeightMap map[v1.ResourceName]int64

// DefaultRequestedRatioResources is used to set default requestToWeight map for CPU and memory
var DefaultRequestedRatioResources = ResourceToWeightMap{v1.ResourceMemory: 1, v1.ResourceCPU: 1}

// resourceAllocationPriority contains information to calculate resource allocation priority.
type resourceAllocationScorer struct {
	name                string
	scorer              func(requested, allocable ResourceToValueMap, includeVolumes bool, requestedVolumes int, allocatableVolumes int) int64
	resourceToWeightMap ResourceToWeightMap
}

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

handle framework.FrameworkHandle
prioritize priorities.PriorityMapFunction
handle framework.FrameworkHandle
priority *ResourceAllocationPriority

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

lets make it an unnamed variable so that we call score (the new name of PriorityMap function) directly

type RequestedToCapacityRatio struct {
   handle   framework.FrameworkHandle
   resourceAllocationScorer
}
@@ -69,12 +88,114 @@ func (pl *RequestedToCapacityRatio) Score(ctx context.Context, _ *framework.Cycl
if err != nil {
return 0, framework.NewStatus(framework.Error, fmt.Sprintf("getting node %q from Snapshot: %v", nodeName, err))
}
// Note that RequestedToCapacityRatioPriority doesn't use priority metadata, hence passing nil here.
s, err := pl.prioritize(pod, nil, nodeInfo)
s, err := pl.priority.PriorityMap(pod, nodeInfo)
return s.Score, migration.ErrorToFrameworkStatus(err)

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

remove the dependency on the migration package, also if you make the changes below you can just return directly:

return pl.score(pod, nil, nodeInfo)	
@@ -49,7 +49,6 @@ var DefaultRequestedRatioResources = ResourceToWeightMap{v1.ResourceMemory: 1, v
// It will use `scorer` function to calculate the score.
func (r *ResourceAllocationPriority) PriorityMap(
pod *v1.Pod,
meta interface{},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

the function should now return (int64, framework.Status) not (framework.NodeScore, error)

For example, line 55:
return 0, framework.NewStatus(framework.Error, "node not found")

mostResourcePriority := &ResourceAllocationPriority{MostAllocatedName, mostResourceScorer, mostRequestedRatioResources}
// mostResourcePriority.PriorityMap is a priority function that favors nodes with most requested resources.
// It calculates the percentage of memory and CPU requested by pods scheduled on the node, and prioritizes
// based on the maximum of the average of the fraction of requested to capacity.
// Details: (cpu(10 * sum(requested) / capacity) + memory(10 * sum(requested) / capacity)) / 2
s, err := mostResourcePriority.PriorityMap(pod, nodeInfo)
return s.Score, migration.ErrorToFrameworkStatus(err)
Comment on lines 54 to 60

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

this will be replaced with

return ma.score(pod, nodeInfo)
@@ -32,7 +31,10 @@ type LeastAllocated struct {
handle framework.FrameworkHandle
}

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

make it the same as RequestedToCapacityRatio:

type LeastAllocated struct {
  handle framework.FrameworkHandle
  resourceAllocationScorer
}
var _ = framework.ScorePlugin(&LeastAllocated{})
var (
_ = framework.ScorePlugin(&LeastAllocated{})
leastRequestedRatioResources = DefaultRequestedRatioResources

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

no need for this variable

leastResourcePriority := &ResourceAllocationPriority{LeastAllocatedName, leastResourceScorer, leastRequestedRatioResources}
// leastResourcePriority.PriorityMap is a priority function that favors nodes with fewer requested resources.
// It calculates the percentage of memory and CPU requested by pods scheduled on the node, and
// prioritizes based on the minimum of the average of the fraction of requested to capacity.
//
// Details:
// (cpu((capacity-sum(requested))*10/capacity) + memory((capacity-sum(requested))*10/capacity))/2
s, err := leastResourcePriority.PriorityMap(pod, nodeInfo)
return s.Score, migration.ErrorToFrameworkStatus(err)
Comment on lines 54 to 62

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member
return la.score(pod, nodeInfo)
@@ -63,3 +71,27 @@ func (la *LeastAllocated) ScoreExtensions() framework.ScoreExtensions {
func NewLeastAllocated(_ *runtime.Unknown, h framework.FrameworkHandle) (framework.Plugin, error) {
return &LeastAllocated{handle: h}, nil
}

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 30, 2019

Member

This will become something like this:

return &LeastAllocated{
  handle: h,
  resourceAllocationScorer: {LeastAllocatedName, leastResourceScorer, DefaultRequestedRatioResources},
 }, nil
@notpad

This comment has been minimized.

Copy link
Member Author

notpad commented Dec 31, 2019

@ahg-g Really appreciate for your review and detailed explanation, I have addressed the comments, could you take another look? Thanks~

Copy link
Member

ahg-g left a comment

few nits, please squash.

handle: handle,
priority: p,
handle: handle,
resourceAllocationScorer: resourceAllocationScorer{RequestedToCapacityRatioName, buildRequestedToCapacityRatioScorerFunction(args.FunctionShape, args.ResourceToWeightMap), args.ResourceToWeightMap},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 31, 2019

Member

nit, please break this into multiple lines so that it is easier to read:

resourceAllocationScorer: resourceAllocationScorer{
           RequestedToCapacityRatioName, 
           buildRequestedToCapacityRatioScorerFunction(args.FunctionShape, args.ResourceToWeightMap), 
           args.ResourceToWeightMap,
},
return &LeastAllocated{handle: h}, nil
return &LeastAllocated{
handle: h,
resourceAllocationScorer: resourceAllocationScorer{LeastAllocatedName, leastResourceScorer, DefaultRequestedRatioResources},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 31, 2019

Member

nit, please break into multiple lines:

resourceAllocationScorer: resourceAllocationScorer{
   LeastAllocatedName, 
   leastResourceScorer, 
   DefaultRequestedRatioResources,
},
return &BalancedAllocation{handle: h}, nil
return &BalancedAllocation{
handle: h,
resourceAllocationScorer: resourceAllocationScorer{BalancedAllocationName, balancedResourceScorer, DefaultRequestedRatioResources},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 31, 2019

Member

nit: please break in multiple lines:

resourceAllocationScorer: resourceAllocationScorer{
  BalancedAllocationName, 
  balancedResourceScorer, 
  DefaultRequestedRatioResources,
 },
return &MostAllocated{handle: h}, nil
return &MostAllocated{
handle: h,
resourceAllocationScorer: resourceAllocationScorer{MostAllocatedName, mostResourceScorer, DefaultRequestedRatioResources},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 31, 2019

Member
resourceAllocationScorer: resourceAllocationScorer{
  MostAllocatedName, 
  mostResourceScorer, 
  DefaultRequestedRatioResources,
},
@notpad notpad force-pushed the notpad:feature/scheduler branch from 5500c47 to 7f79516 Dec 31, 2019
@notpad

This comment has been minimized.

Copy link
Member Author

notpad commented Dec 31, 2019

Code squashed.

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 31, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 31, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@notpad

This comment has been minimized.

Copy link
Member Author

notpad commented Dec 31, 2019

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit 8743d98 into kubernetes:master Dec 31, 2019
15 checks passed
15 checks passed
cla/linuxfoundation notpad authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
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
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 31, 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.