feat: scheduler (8/): add more scheduler framework logic #389

michaelawyu · 2023-06-16T11:49:46Z

Description of your changes

This PR is part of the PRs that implement the Fleet workload scheduling.

It features parts of the scheduler framework logic.

This PR has been updated to reflect the latest design refresh; it also renames an API per the latest design doc.

I have:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Unit tests

Special notes for your reviewer

Per earlier agreements, testing on this PR has a limited focus/scope.

Important: Please postpone reviewing of this PR until the design/API change has finalized.

pkg/scheduler/framework/framework.go

pkg/scheduler/framework/frameworkutils.go

pkg/scheduler/framework/framework_test.go

pkg/utils/common.go

pkg/scheduler/framework/framework.go

pkg/scheduler/framework/frameworkutils.go

pkg/scheduler/framework/framework.go

michaelawyu · 2023-06-21T09:42:00Z

apis/placement/v1beta1/binding_types.go

+
+	// SchedulerFinalizer is added by the scheduler to make sure that a binding can only be deleted if the scheduler
+	// has relieved it from scheduling consideration.
+	SchedulerFinalizer = fleetPrefix + "scheduler-cleanup"


Technically speaking, since at this moment user no longer has the permission to delete bindings manually, we do not need this finalizer any more (it was originally designed for making sure that the scheduler will always get notified when a binding is deleted so that it can creates a new one in replacement).

However, in this PR I preserved this finalizer just for the case where a binding still needs to be manually deleted, in case of, say, troubleshooting, emergency fix, or future support for individual eviction -> it might not be optimal if the only way for a customer to remove resources from a cluster is to trigger a rescheduling via policy update; though at this stage it might not be of too much concern.

michaelawyu · 2023-06-21T09:43:21Z

apis/placement/v1beta1/binding_types.go

+	//
+	// Note also that it is not the scheduler's responsibility to add this label, even though it does
+	// reads this label to inform the scheduling cycle..
+	ActiveBindingLabel = fleetPrefix + "is-active-binding"


The states are kept as labels for easier queries; a field would also work though. Anti-tempering is needed to make sure that the system functions correctly.

ryanzhang-oss · 2023-06-22T23:44:54Z

pkg/scheduler/queue/queue.go

 // more queues, such as a backoff queue, might become necessary.
-type simpleClusterPolicySnapshotKeySchedulingQueue struct {
+type simpleSchedulingPolicySnapshotKeySchedulingQueue struct {
 	clusterPolicySanpshotWorkQueue workqueue.RateLimitingInterface


Suggested change

clusterPolicySanpshotWorkQueue workqueue.RateLimitingInterface

clusterPolicySnapshotWorkQueue workqueue.RateLimitingInterface

ryanzhang-oss · 2023-06-22T23:48:22Z

pkg/scheduler/queue/queue.go

-// Done marks a ClusterPolicySnapshot key as done.
-func (sq *simpleClusterPolicySnapshotKeySchedulingQueue) Done(cpsKey ClusterPolicySnapshotKey) {
+// Done marks a SchedulingPolicySnapshot key as done.
+func (sq *simpleSchedulingPolicySnapshotKeySchedulingQueue) Done(cpsKey SchedulingPolicySnapshotKey) {


Suggested change

func (sq *simpleSchedulingPolicySnapshotKeySchedulingQueue) Done(cpsKey SchedulingPolicySnapshotKey) {

func (sq *simpleSchedulingPolicySnapshotKeySchedulingQueue) Done(spsKey SchedulingPolicySnapshotKey) {

ryanzhang-oss · 2023-06-22T23:51:07Z

pkg/scheduler/queue/queue.go

-// ClusterPolicySnapshotKey is the unique identifier (its name) for a ClusterPolicySnapshot stored in a scheduling queue.
-type ClusterPolicySnapshotKey string
+// SchedulingPolicySnapshotKey is the unique identifier (its name) for a SchedulingPolicySnapshot stored in a scheduling queue.
+type SchedulingPolicySnapshotKey string


IIRC, we are still holding CRP key in the queue, right?

Yeah, we are still holding CRP keys in the queue. Now since the design has evolved maybe I should just change it to be a CRP key queue instead.

ryanzhang-oss · 2023-06-22T23:53:47Z

pkg/utils/common.go

+
+	// Cast the annotation to an integer; throw an error if the cast cannot be completed or the value is negative.
+	numOfClusters, err := strconv.Atoi(numOfClustersStr)
+	if err != nil || numOfClusters < 0 {


is numOfCluster == 0 a valid case? I don't seem to see test for this case.

Ah, this is a case that some clarification might be needed: numOfClusters = 0 is interpreted as removing all bindings without deleting the CRP (i.e., pausing), hence a valid case, is it correct?

ryanzhang-oss · 2023-06-22T23:58:23Z

pkg/scheduler/framework/frameworkutils.go

+		_, activeLabelExists := binding.Labels[fleetv1beta1.ActiveBindingLabel]
+		_, creatingLabelExists := binding.Labels[fleetv1beta1.CreatingBindingLabel]
+		_, obsoleteLabelExists := binding.Labels[fleetv1beta1.ObsoleteBindingLabel]
+		_, noTargetClusterLabelExists := binding.Labels[fleetv1beta1.NoTargetClusterBindingLabel]


Do we need to use label to differentiate? I defined them in the spec.

Yeah, this PR update is sent before the API PR (and the design discussion with Fei) so it is out of date. I will update the PR again when the API PR is merged.

ryanzhang-oss · 2023-06-22T23:59:02Z

pkg/scheduler/framework/frameworkutils.go

+Licensed under the MIT license.
+*/
+
+package framework


can the file name just be utils.go?

Originally it is :D. I changed it to this name as I do not want to imply that code here is utilities for all files in the package but for the framework part only.

ryanzhang-oss · 2023-06-23T00:12:16Z

pkg/scheduler/framework/framework.go


 	// RunSchedulerCycleFor performs scheduling for a policy snapshot.
-	RunSchedulingCycleFor(ctx context.Context, policy *fleetv1beta1.ClusterPolicySnapshot, resources *fleetv1beta1.ClusterResourceSnapshot) (result ctrl.Result, err error)
+	RunSchedulingCycleFor(ctx context.Context, crpName string, policy *fleetv1beta1.SchedulingPolicySnapshot, resources *fleetv1beta1.ClusterResourceSnapshot) (result ctrl.Result, err error)


why does the scheduler need to know ClusterResoruceSnapshot? I just realized that the rollout controller can decide the resource when it activate a "creating" binding. In this way, the scheduler will totally get out of the business of resource (the part that won't affect scheduling decision)

Yeah, originally it is needed for creating the binding (there is a resource snapshot field, and in our old design there was no rollout controller). Though technically speaking the scheduler does not need to fulfill this field if we make it clear that it is the responsibility for the rollout controller to fulfill this field instead (and that the policy snapshot associated with the resource snapshot does not need to align with the one marked on the binding -> it is the outer field that counts)

michaelawyu · 2023-06-29T11:32:53Z

Closing as the design/API has changed; another PR has been sent.

…zure#389)

Added more scheduler logic

9a1de88

michaelawyu changed the title ~~feat: scheduler (9/): add more scheduler framework logic~~ feat: scheduler (8/): add more scheduler framework logic Jun 16, 2023

michaelawyu requested review from ryanzhang-oss and zhiying-lin June 16, 2023 11:49

zhiying-lin reviewed Jun 19, 2023

View reviewed changes

Minor fixes

aa13324

ryanzhang-oss reviewed Jun 19, 2023

View reviewed changes

pkg/scheduler/framework/frameworkutils.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/framework.go Outdated Show resolved Hide resolved

Reflect changes in design refresh

0c7d0bf

michaelawyu commented Jun 21, 2023

View reviewed changes

michaelawyu and others added 2 commits June 21, 2023 17:47

Minor fixes

90d2ec1

Merge branch 'main' into scheduler-framework-logic-2

d3aebd1

ryanzhang-oss reviewed Jun 23, 2023

View reviewed changes

michaelawyu closed this Jun 29, 2023

britaniar added a commit to britaniar/fleet that referenced this pull request Jan 20, 2026

fix: add state and remove generation labels from update run metrics (A…

2a197fd

…zure#389)

	clusterPolicySanpshotWorkQueue workqueue.RateLimitingInterface
	clusterPolicySnapshotWorkQueue workqueue.RateLimitingInterface

	func (sq *simpleSchedulingPolicySnapshotKeySchedulingQueue) Done(cpsKey SchedulingPolicySnapshotKey) {
	func (sq *simpleSchedulingPolicySnapshotKeySchedulingQueue) Done(spsKey SchedulingPolicySnapshotKey) {

feat: scheduler (8/): add more scheduler framework logic #389

feat: scheduler (8/): add more scheduler framework logic #389

Uh oh!

Conversation

michaelawyu commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelawyu commented Jun 16, 2023 •

edited

Loading