feat: configurable arguments for the scheduler plugins #210
Conversation
| @@ -15,6 +15,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # | |||
| # TODO: migrate to k8s.io/code-generator/kube_codegen.sh | |||
There was a problem hiding this comment.
both generate-groups and generate-internal-groups seems to be gone with v1.30.
There was a problem hiding this comment.
Could you please resolve this TODO? What does "gone" mean for this script?
Typically, I try to follow this: https://github.com/kubernetes/sample-controller/blob/master/hack/update-codegen.sh
There was a problem hiding this comment.
generate-internal-groups.sh and generate-groups.sh which this script depends have been deprecated since 1.29 and removed in 1.30.
The recommendation is to switch to kube_codegen.sh instead.
Happy to include the change here, just wanted to keep the scope small.
There was a problem hiding this comment.
so I ended up updating the k8s deps in the process including controller-runtime and the change is here (+boilerplate changes):
Thoughts on having this as a separate follow-up PR? I guess it doesn't matter much since I noticed PRs aren't merged squashed but it would be easier to review the diff separately. Happy to do either, let me know.
| @@ -0,0 +1,44 @@ | |||
| /* | |||
| * Copyright 2020 The Multicluster-Scheduler Authors. | |||
There was a problem hiding this comment.
thoughts on dropping the year from the boilerplate header?
There was a problem hiding this comment.
You can update it in https://github.com/admiraltyio/admiralty/blob/master/hack/boilerplate.go.txt
| } | ||
|
|
||
| var _ framework.PreFilterPlugin = &Plugin{} | ||
| var _ framework.ReservePlugin = &Plugin{} | ||
| var _ framework.PreBindPlugin = &Plugin{} | ||
|
|
||
| // Name is the name of the plugin used in the plugin registry and configurations. | ||
| const Name = "candidate" | ||
| const Name = "Candidate" |
There was a problem hiding this comment.
The framework expects the args to be of the form pluginName + "Args" so I changed the casing here.
marwanad
force-pushed
the
plugin-args
branch
2 times, most recently
from
752ad80 to
538e75e
Compare
| @@ -15,6 +15,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # | |||
| # TODO: migrate to k8s.io/code-generator/kube_codegen.sh | |||
There was a problem hiding this comment.
Could you please resolve this TODO? What does "gone" mean for this script?
Typically, I try to follow this: https://github.com/kubernetes/sample-controller/blob/master/hack/update-codegen.sh
| @@ -0,0 +1,44 @@ | |||
| /* | |||
| * Copyright 2020 The Multicluster-Scheduler Authors. | |||
There was a problem hiding this comment.
You can update it in https://github.com/admiraltyio/admiralty/blob/master/hack/boilerplate.go.txt
| */ | ||
|
|
||
| // +k8s:deepcopy-gen=package | ||
| // +groupName=kubescheduler.config.k8s.io |
There was a problem hiding this comment.
Why is this not config.admiralty.io?
Taking a step back, why don't you simply add the config types to pkg/apis/multicluster?
If you do need a separate group, declaring a v1 seems premature.
There was a problem hiding this comment.
So it's my understanding that for out of tree scheduler plugins, it has to be registered to the same GVK as the scheduler configs since the loading of those hardcodes and expects to parse as the plugin name + "Args" when converting to Go types.
Since 1.29, all alphas/betas are deprecated so only the v1 is supported. kubescheduler.config.k8s.io/v1 is supported since 1.25.
| // FilterWaitDurationSeconds specifies how long the filter extension point waits for a terminal | ||
| // signal (reserved or unscheduled) from the pod chaperon |
There was a problem hiding this comment.
Could you document why one might want to use a lower or higher value here? In short, please describe the use cases.
There was a problem hiding this comment.
I don't think that this is resolved.
|
This is a lot of boilerplate to add plugin config options. Note that the proxy scheduler already sources some of its config from environment variables (CLUSTER_NAME) and from (Cluster)Target CRDs. Also, you may want different wait durations depending on the cluster, or perhaps depending on the pod. Both for simplicity and flexibility, may I suggest two alternatives:
|
This change adds configurable arguments for both the proxy and candidate plugins and changes the
filterWaitin the proxy plugin andpreBindWaitin the candidate plugin to be configurable.I also updated the scheduler config api version to
v1as part of this change.