feat: scheduler (7/): add initial parts of scheduler framework #385
Conversation
zhiying-lin
left a comment
zhiying-lin
left a comment
There was a problem hiding this comment.
minor comments, others LGTM :)
pkg/scheduler/framework/framework.go
Outdated
| Handle | ||
|
|
||
| // RunSchedulerCycleFor performs scheduling for a policy snapshot. | ||
| RunSchedulingCycleFor(ctx context.Context, policy *fleetv1.ClusterPolicySnapshot, resources *fleetv1.ClusterResourceSnapshot) (result ctrl.Result, err error) |
There was a problem hiding this comment.
| RunSchedulingCycleFor(ctx context.Context, policy *fleetv1.ClusterPolicySnapshot, resources *fleetv1.ClusterResourceSnapshot) (result ctrl.Result, err error) | |
| RunSchedulingCycleForPlacement(ctx context.Context, policy *fleetv1.ClusterPolicySnapshot, resources *fleetv1.ClusterResourceSnapshot) (result ctrl.Result, err error) |
changed to RunSchedulingCycleForPlacement?
There was a problem hiding this comment.
[This For pattern is commonly used in Kubernetes](https://github.com/search?q=repo%3Akubernetes%2Fkubernetes%20For(&type=code).
|
|
||
| const ( | ||
| // eventRecorderNameTemplate is the template used to format event recorder name for a scheduler framework. | ||
| eventRecorderNameTemplate = "scheduler-framework-%s" |
There was a problem hiding this comment.
is this event msg readable for user?
There was a problem hiding this comment.
Hi Ryan! Ah, this isn't the event message, but the name of the event recorder in use by the scheduler framework.
|
|
||
| // Handle is an interface which allows plugins to access some shared structs (e.g., client, manager) | ||
| // and set themselves up with the scheduler framework (e.g., sign up for an informer). | ||
| type Handle interface { |
There was a problem hiding this comment.
In general, I think interface name is better to be a noun while the function name is a verb. ie
type Writer interface {
Write(p []byte) (n int, err error)
}
We have the exact opposite pattern here.
There was a problem hiding this comment.
The name is also from Kubernetes; Handler would be a little bit weird, it's really more of a handle that allows plugins to touch the framework (in a limited manner).
michaelawyu
force-pushed
the
scheduler-framework-logic
branch
from
b966bce to
15a0f88
Compare
michaelawyu
force-pushed
the
scheduler-framework-logic
branch
from
26ca9c7 to
2271a50
Compare
|
Hi Ryan! I took the liberty to merge this PR first; if you have any further concerns about it, please let me know. |
3 participants
Description of your changes
This PR is part of the PRs that implement the Fleet workload scheduling.
It features the scaffolding for scheduler framework.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
N/A.
To accommodate the uncertainties forward, and considering the fact that there is no easy way to fake a controller runtime manager, this PR does not include unit test (sanity check) for NewFramework(). Tests will be checked in with future PRs.
Special notes for your reviewer