[multikueue] add support for custom multikueue controller #2405
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mszadkow The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @mszadkow. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:default="kueue.x-k8s.io/multikueue" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="field is immutable" | ||
| ControllerName string `json:"controllerName"` |
There was a problem hiding this comment.
We deliberately didn't make it configurable if the first iteration to limit the potential of misconfiguring the system. Could you first open the issue to discuss the need for it?
There was a problem hiding this comment.
The status of a multikueue cluster follows the state of a contreller-runtime client in the kueue-controller-manager, if we want to be able to have multiple controller managers working with the clusters we need a way to mark who is managing each cluster.
There was a problem hiding this comment.
I see, but I'm wondering if we could alternatively still use MultiKueue controller, but have a concept of external plugin?
To make it easier to track of an object is managed by MultiKueue or not. I feel this would help OnCall, but maybe I need to think about this more.
Since this required API changes should we not first go via KEP to discuss alternatives? WDYT @alculquicondor, @tenzen-y?
There was a problem hiding this comment.
if we want to be able to have multiple controller managers working with the clusters we need a way to mark who is managing each cluster.
There should only be one controller manager modifying these objects (Kueue).
Other controller managers should only be in charge of mirroring the Job CRDs, but otherwise they can use all the same objects.
I would like to see at least a description of what the plan is, how components should interact, what each of them is responsible of. Maybe a KEP is worth, but please start with some description before writing more code.
There was a problem hiding this comment.
I see, but I'm wondering if we could alternatively still use MultiKueue controller, but have a concept of external plugin?
That will be a lot more complicated and will most likely require some additional API object type(s) to communicate between that custom controller to multikueue admission check controller.
The target of this PR is to be able to instantiate multikueue admission check controllers having different sets of adapters that can work in the same cluster at the same time.
There should only be one controller manager modifying these objects (Kueue).
MultiKueueClusters should not be that different then AdmissionChecks, which by design are managed by an AdmissionCheckController running in or outside of Kueue.
There was a problem hiding this comment.
The current approach will lose the consistency between the kueue-manager and external controllers since it seems that the current approach allows us to re-implement the multikueue admission controller.
Can we consider an alternative approach to prevent the losing specification consistency?
As I described in the #2349, nice to start in the synchronizing design and the actual implementations.
|
/assign @trasc |
k8s-ci-robot
added
the
needs-rebase
mszadkow
force-pushed
the
feature/multikueue-support-for-custom-jobs
branch
from
b5aef69
to
6333bb8
Compare
|
With the refactor and documentation PRs merged, can you now propose an update to the KEP to add custom jobs support? |
Pass the given ControllerName to the reconcilers Update tests
to be able to pass custom multikueue controller and adapters
mszadkow
force-pushed
the
feature/multikueue-support-for-custom-jobs
branch
from
6333bb8
to
ee5a798
Compare
k8s-ci-robot
added
size/XL
Yes, I am working on KEP now. |
|
/retest |
|
@mszadkow: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| } | ||
| } | ||
|
|
||
| // WithAdapters - sets all the MultiKueue adaptors. |
| } | ||
| } | ||
|
|
||
| // WithAdapters - sets or updates the adadpter of the MultiKueue adaptors. |
|
/retest |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@mszadkow: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Provide a way to pass custom controller to multikueue admission check controller.
Which issue(s) this PR fixes:
Fixes #2349
Special notes for your reviewer:
Consider base branch: #2373
Does this PR introduce a user-facing change?