refactor coscheduling to use controller-runtime client #652
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei 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 |
k8s-ci-robot
added
the
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.
|
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/XXL
pkg/coscheduling/core/core.go
Outdated
| pg, err := pgMgr.pgLister.PodGroups(pod.Namespace).Get(pgName) | ||
| if err != nil { | ||
| var pg v1alpha1.PodGroup | ||
| if err := pgMgr.client.Get(context.Background(), types.NamespacedName{Namespace: pod.Namespace, Name: pgName}, &pg); err != nil { |
There was a problem hiding this comment.
is there any intention to use background context here but todo there https://github.com/kubernetes-sigs/scheduler-plugins/pull/652/files#diff-d693bb3d21cc3922d5718e715b2f4412aa52d99d7b5f75f4e29cca14a015fa27R222?
There was a problem hiding this comment.
it's basically the same.
There was a problem hiding this comment.
updated to reuse context.
pkg/coscheduling/core/core.go
Outdated
| pg, err := pgMgr.pgLister.PodGroups(pod.Namespace).Get(pgName) | ||
| if err != nil { | ||
| var pg v1alpha1.PodGroup | ||
| if err := pgMgr.client.Get(context.Background(), types.NamespacedName{Namespace: pod.Namespace, Name: pgName}, &pg); err != nil { |
There was a problem hiding this comment.
nit: can we unified to use context.TODO? Furthermore, maybe we can pass in the context
There was a problem hiding this comment.
yes and yes. Regarding "pass in the context", for some of the functions, we may want scheduler framework to provide a NewWithContext() signature to pass the context all the way down; otherwise, each scheduler framework consumer has to implement this NewWithContext() wrapper.
|
@zwpaper @kerthcet I refactored a bit to pass in context in places where context obj is derivable. However for some places like Less(), it's still infeasible w/o changes in scheduler framework for now #652 (comment) |
|
LGTM. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
As #485 stated, we're migrating all plugins and controllers to use controller-runtime client. With the migration's completion, we should be able to eliminate all typed clients along with its generated bits.
This PR aims to migrate coscheduling.
Which issue(s) this PR fixes:
Part of #485
Special notes for your reviewer:
This PR also polished existing tests.
Does this PR introduce a user-facing change?