GetMultiKueueAdapters() returns only registered and enabled adapters #3603
+416
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
the
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
k8s-ci-robot
added
the
size/M
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
for MK behavior in case of uninstalled CRDs
mszadkow
force-pushed
the
fix/constraint-mk-adapters-with-enabled-integrations
branch
from
c80ad1d
to
0b07501
Compare
|
[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 |
k8s-ci-robot
added
size/L
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @mbobrovskyi |
trasc
reviewed
Nov 21, 2024
Comment on lines
+217
to
+220
| adapters, err := jobframework.GetMultiKueueAdapters(sets.New([]string{ | ||
| "batch/job", "kubeflow.org/mpijob", "ray.io/rayjob", "ray.io/raycluster", | ||
| "jobset.x-k8s.io/jobset", "kubeflow.org/mxjob", "kubeflow.org/paddlejob", | ||
| "kubeflow.org/pytorchjob", "kubeflow.org/tfjob", "kubeflow.org/xgboostjob"}...)) |
There was a problem hiding this comment.
Suggested change
| adapters, err := jobframework.GetMultiKueueAdapters(sets.New([]string{ | |
| "batch/job", "kubeflow.org/mpijob", "ray.io/rayjob", "ray.io/raycluster", | |
| "jobset.x-k8s.io/jobset", "kubeflow.org/mxjob", "kubeflow.org/paddlejob", | |
| "kubeflow.org/pytorchjob", "kubeflow.org/tfjob", "kubeflow.org/xgboostjob"}...)) | |
| adapters, err := jobframework.GetMultiKueueAdapters(sets.New[string]( | |
| "batch/job", "kubeflow.org/mpijob", "ray.io/rayjob", "ray.io/raycluster", | |
| "jobset.x-k8s.io/jobset", "kubeflow.org/mxjob", "kubeflow.org/paddlejob", | |
| "kubeflow.org/pytorchjob", "kubeflow.org/tfjob", "kubeflow.org/xgboostjob")) |
Also the slice of enabled integrations can be an argument to this function, whit this you can move the tests from "./failedsetup" , in this suite (but sure in their own var _ = ginkgo.Describe( )
trasc
reviewed
Nov 21, 2024
| @@ -264,10 +265,16 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, cCache *cache.Cache | |||
| } | |||
|
|
|||
| if features.Enabled(features.MultiKueue) { | |||
| adapters, err := jobframework.GetMultiKueueAdapters(sets.New(cfg.Integrations.Frameworks...)) | |||
| if err != nil { | |||
| setupLog.Error(err, "Could not setup MultiKueue controller") | |||
There was a problem hiding this comment.
Suggested change
| setupLog.Error(err, "Could not setup MultiKueue controller") | |
| setupLog.Error(err, "Could not get the enabled multikueue adapters") |
Comment on lines
+380
to
+381
| enabledIntegrations := sets.New([]string{"batch/job"}...) | ||
| adapters, _ := jobframework.GetMultiKueueAdapters(enabledIntegrations) |
There was a problem hiding this comment.
Suggested change
| enabledIntegrations := sets.New([]string{"batch/job"}...) | |
| adapters, _ := jobframework.GetMultiKueueAdapters(enabledIntegrations) | |
| adapters, _ := jobframework.GetMultiKueueAdapters(sets.New("batch/job")) |
The same goes in a couple of other places.
| @@ -51,6 +52,7 @@ var ( | |||
| ) | |||
|
|
|||
| func TestWlReconcile(t *testing.T) { | |||
| t.Cleanup(jobframework.EnableIntegrationsForTest(t, "batch/job")) | |||
| util.ExpectObjectToBeDeleted(managerTestCluster.ctx, managerTestCluster.client, managerMultikueueSecret1, true) | ||
| }) | ||
|
|
||
| ginkgo.It("Should not create a MPIJob workload, because MPIJob CRD is not installed", func() { |
There was a problem hiding this comment.
Suggested change
| ginkgo.It("Should not create a MPIJob workload, because MPIJob CRD is not installed", func() { | |
| ginkgo.It("Should not create a MPIJob workload, when MPIJob adapter is not enabled", func() { |
| gomega.Expect(apimeta.IsNoMatchError(err)).To(gomega.BeTrue()) | ||
|
|
||
| wlLookupKey := types.NamespacedName{Name: workloadmpijob.GetWorkloadNameForMPIJob(mpijob.Name, mpijob.UID), Namespace: managerNs.Name} | ||
| ginkgo.By("workload not found", func() { |
There was a problem hiding this comment.
The MpiJob integration should be enabled, and we should check that it's workload get's rejected by multikueue die to missing adapter.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
MK adapter didn't contain the condition to only allow enabled integrations.
Thus while using multi cluster setup, all registered MK adapters were used to setup controllers.
In result we get
ClientConnectionFailed error with no matches for kind "XYZ" in "version v1" messageWhich issue(s) this PR fixes:
Fixes #3582
Special notes for your reviewer:
Does this PR introduce a user-facing change?