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

add dynamic weight by available replicas #841

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

Garrybest
Copy link
Member

Signed-off-by: Garrybest [email protected]

What type of PR is this?
/kind api-change
/kind feature

What this PR does / why we need it:
According to #730, I think we could add a new division preference instead of mixing with Weighted preference. ReplicaDivisionPreferenceDispersal divides replicas into clusters as many as possible, while respecting clusters' resource availabilities during the division.

Which issue(s) this PR fixes:
Fixes #730

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ReplicaDivisionPreferenceDispersal divides replicas into clusters as many as possible, while respecting clusters' resource availabilities during the division.

@karmada-bot karmada-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 20, 2021
@Garrybest
Copy link
Member Author

@karmada-bot
Copy link
Collaborator

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

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

In response to this:

/cc @kevin-wangzefeng @RainbowMango @qianjun1993 @mrlihanbo @XiShanYongYe-Chang @gf457832386

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.

@RainbowMango
Copy link
Member

/assign

pkg/apis/policy/v1alpha1/propagation_types.go Show resolved Hide resolved
Comment on lines 190 to 209
// ReplicaDivisionPreferenceDispersal divides replicas into clusters as many as possible,
// while respecting clusters' resource availabilities during the division.
ReplicaDivisionPreferenceDispersal ReplicaDivisionPreference = "Dispersal"
Copy link
Member

Choose a reason for hiding this comment

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

Should we describe the algorithm a little bit here, about how to divide the replicas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, if we divide replicas only by MaxAvailableReplicas, it may cause that a cluster is running out of memory but has a lot of CPU left. Do we need to think about solving the problem of "resource fragmentation". ( I didn't express it clearly in last meeting...emmm @RainbowMango

Copy link
Member Author

Choose a reason for hiding this comment

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

As @gf457832386 said, divide replica by MaxAvailableReplicas. For example, when the scheduler tries to schedule workload X in Dispersal mode:

Cluster A max available replica: 6
Cluster B max available replica: 12
Cluster C max available replica: 18

So we could divide the replica by 6:12:18, as 2 of cluster A, 4 of cluster B and 6 of cluster C. It is obvious that the division type has a benefit with cluster load balance.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be better to add some documents about all scheduling algorithms and user guide. Besides, more comments could be added first. Let me make a change.

@karmada-bot karmada-bot 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 Oct 24, 2021
@Garrybest
Copy link
Member Author

Does this PR need more discussion? I think we already had a deal on last meeting. But I'm not sure.

@RainbowMango
Copy link
Member

@gf457832386 Do you have any comments?

@RainbowMango
Copy link
Member

/lgtm
/assign @kevin-wangzefeng
Let's invite Kevin to take a look.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2021
@RainbowMango
Copy link
Member

At the last meeting, @kevin-wangzefeng also mentioned another solution that putting the preference into weight list as a dynamic weight list(ClusterPreferences reserved the space for dynamic weight list).

I tried to work out this solution based on @Garrybest's work, and let's see which solution is better.

// ClusterPreferences describes weight for each cluster or for each group of cluster.
type ClusterPreferences struct {
	// StaticWeightList defines the static cluster weight.
	// +optional
	StaticWeightList []StaticClusterWeight `json:"staticWeightList,omitempty"`
	// DynamicWeight specifies the factor to generates dynamic weight list.
	// +optional
	DynamicWeight DynamicWeightFactor `json:"dynamicWeight,omitempty"`
}

// DynamicWeightFactor represents the weight factor.
// For now only support 'AvailableResource', more factors could be extended if there is a need.
type DynamicWeightFactor string

const (
	// DynamicWeightByAvailableResource represents the cluster weight list should be generated according to
	// available resource(available replicas). e.g.
	// The scheduler has selected 3 clusters(A/B/C) and should divide 12 replicas to them.
	//
	// Workload:
	//   Desired replica: 12
	//
	// Cluster:
	//   A:
	//     Max available replica: 6
	//   B:
	//     Max available replica: 12
	//   C:
	//     Max available replica: 18
	//
	// The weight of cluster A:B:C will be 6:12:18(equals to 1:2:3). At last, the assignment would be 'A: 2, B: 4, C: 6'.
	DynamicWeightByAvailableResource DynamicWeightFactor = "AvailableResource"
)

@Garrybest
Copy link
Member Author

Wow, it's nice. What about AvailableReplica instead of AvailableResource? I think the weight is based on available replicas exactly.

@gf457832386
Copy link
Contributor

At the last meeting, @kevin-wangzefeng also mentioned another solution that putting the preference into weight list as a dynamic weight list(ClusterPreferences reserved the space for dynamic weight list).

I tried to work out this solution based on @Garrybest's work, and let's see which solution is better.

// ClusterPreferences describes weight for each cluster or for each group of cluster.
type ClusterPreferences struct {
	// StaticWeightList defines the static cluster weight.
	// +optional
	StaticWeightList []StaticClusterWeight `json:"staticWeightList,omitempty"`
	// DynamicWeight specifies the factor to generates dynamic weight list.
	// +optional
	DynamicWeight DynamicWeightFactor `json:"dynamicWeight,omitempty"`
}

// DynamicWeightFactor represents the weight factor.
// For now only support 'AvailableResource', more factors could be extended if there is a need.
type DynamicWeightFactor string

const (
	// DynamicWeightByAvailableResource represents the cluster weight list should be generated according to
	// available resource(available replicas). e.g.
	// The scheduler has selected 3 clusters(A/B/C) and should divide 12 replicas to them.
	//
	// Workload:
	//   Desired replica: 12
	//
	// Cluster:
	//   A:
	//     Max available replica: 6
	//   B:
	//     Max available replica: 12
	//   C:
	//     Max available replica: 18
	//
	// The weight of cluster A:B:C will be 6:12:18(equals to 1:2:3). At last, the assignment would be 'A: 2, B: 4, C: 6'.
	DynamicWeightByAvailableResource DynamicWeightFactor = "AvailableResource"
)

A better idea!

@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2021
@kevin-wangzefeng
Copy link
Member

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2021
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevin-wangzefeng

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2021
@karmada-bot karmada-bot merged commit 085fdd2 into karmada-io:master Oct 29, 2021
@Garrybest Garrybest deleted the pr_dispersal branch October 29, 2021 07:10
@Garrybest Garrybest changed the title add dispersal replica division preference add dynamic weight by available resource Oct 29, 2021
@Garrybest Garrybest changed the title add dynamic weight by available resource add dynamic weight by available replicas Oct 29, 2021
@RainbowMango
Copy link
Member

Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion about the behavior of replica scheduling weight preference
5 participants