GetMultiKueueAdapters() returns only registered and enabled adapters #3603

mszadkow · 2024-11-20T15:27:02Z

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" message

Which issue(s) this PR fixes:

Fixes #3582

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2024-11-20T15:27:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2024-11-20T15:27:18Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`8d915b9`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/674078db084dbf00080f8d37

mszadkow · 2024-11-20T15:40:44Z

/ok-to-test

k8s-ci-robot · 2024-11-21T15:00:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mszadkow
Once this PR has been reviewed and has the lgtm label, please assign kerthcet, trasc for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mimowo · 2024-11-21T15:33:15Z

/assign @mbobrovskyi
for the first pass

trasc · 2024-11-21T16:16:42Z

test/integration/multikueue/suite_test.go

+	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"}...))


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( )

I am taking the nit, but actually the difference comes with installed CRDs.
I know it's kinda administrator responsibility to make sure CRDs are there, but we would like to document the behaviour when the CRD is missing - that what I think is the reasoning here.

trasc · 2024-11-21T17:42:39Z

cmd/kueue/main.go

@@ -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")


Suggested change

setupLog.Error(err, "Could not setup MultiKueue controller")

setupLog.Error(err, "Could not get the enabled multikueue adapters")

trasc · 2024-11-21T17:44:07Z

pkg/controller/admissionchecks/multikueue/multikueuecluster_test.go

+			enabledIntegrations := sets.New([]string{"batch/job"}...)
+			adapters, _ := jobframework.GetMultiKueueAdapters(enabledIntegrations)


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.

trasc · 2024-11-21T17:44:49Z

pkg/controller/admissionchecks/multikueue/workload_test.go

@@ -51,6 +52,7 @@ var (
 )

 func TestWlReconcile(t *testing.T) {
+	t.Cleanup(jobframework.EnableIntegrationsForTest(t, "batch/job"))


Is this still needed?

not needed indeed

trasc · 2024-11-21T17:46:27Z

test/integration/multikueue/failedsetup/multikueue_test.go

+		util.ExpectObjectToBeDeleted(managerTestCluster.ctx, managerTestCluster.client, managerMultikueueSecret1, true)
+	})
+
+	ginkgo.It("Should not create a MPIJob workload, because MPIJob CRD is not installed", func() {


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() {

trasc · 2024-11-21T17:49:41Z

test/integration/multikueue/failedsetup/multikueue_test.go

+		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() {


The MpiJob integration should be enabled, and we should check that it's workload get's rejected by multikueue die to missing adapter.

This is another test imho, equally important

The workload associated to an MPIJob is created by the MPIJob integration, agnostic to multikueue and multikueue configuration.

for MK behavior in case of disabled integration

mimowo

overall lgtm, but I would like yet to add the "positive" integration test to clearly demonstrate the issue is fixed

mimowo · 2024-11-22T13:32:22Z

test/integration/multikueue/multikueue_test.go

+		util.ExpectObjectToBeDeleted(managerTestCluster.ctx, managerTestCluster.client, managerMultikueueSecret1, true)
+	})
+
+	ginkgo.It("Should not create a MPIJob workload, when MPIJob adapter is not enabled", func() {


This test looks ok, but I think we need to add a test case for using the positive scenario (which was broken), that some integrations can be used (e.g. batch/Job) while not all are enabled, maybe "should create batch/Job workload when some frameworks are disabled"

mimowo · 2024-11-22T13:32:47Z

test/integration/multikueue/multikueue_test.go

@@ -1638,6 +1644,157 @@ var _ = ginkgo.Describe("Multikueue no GC", ginkgo.Ordered, ginkgo.ContinueOnFai
 	})
 })

+var _ = ginkgo.Describe("Multikueue with disabled integrations", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {


Suggested change

var _ = ginkgo.Describe("Multikueue with disabled integrations", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {

var _ = ginkgo.Describe("Multikueue with some integration disabled", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 20, 2024

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 20, 2024

k8s-ci-robot requested review from mbobrovskyi and mimowo November 20, 2024 15:27

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 20, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 20, 2024

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 20, 2024

GetMultiKueueAdapters() returns only registered and enabled adapters

1f35e35

mszadkow force-pushed the fix/constraint-mk-adapters-with-enabled-integrations branch from c80ad1d to 0b07501 Compare November 21, 2024 15:00

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 21, 2024

mszadkow marked this pull request as ready for review November 21, 2024 15:02

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2024

k8s-ci-robot requested a review from denkensk November 21, 2024 15:02

k8s-ci-robot assigned mbobrovskyi Nov 21, 2024

trasc reviewed Nov 21, 2024

View reviewed changes

Add integration test

8d915b9

for MK behavior in case of disabled integration

mszadkow force-pushed the fix/constraint-mk-adapters-with-enabled-integrations branch from 0b07501 to 8d915b9 Compare November 22, 2024 12:28

mimowo reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetMultiKueueAdapters() returns only registered and enabled adapters #3603

GetMultiKueueAdapters() returns only registered and enabled adapters #3603

mszadkow commented Nov 20, 2024

k8s-ci-robot commented Nov 20, 2024

netlify bot commented Nov 20, 2024 •

edited

Loading

mszadkow commented Nov 20, 2024

k8s-ci-robot commented Nov 21, 2024

mimowo commented Nov 21, 2024

trasc Nov 21, 2024

mszadkow Nov 22, 2024

trasc Nov 21, 2024

trasc Nov 21, 2024

trasc Nov 21, 2024

mszadkow Nov 22, 2024

trasc Nov 21, 2024

trasc Nov 21, 2024

mszadkow Nov 22, 2024

trasc Nov 22, 2024

mimowo left a comment

mimowo Nov 22, 2024

mimowo Nov 22, 2024

	setupLog.Error(err, "Could not setup MultiKueue controller")
	setupLog.Error(err, "Could not get the enabled multikueue adapters")

		enabledIntegrations := sets.New([]string{"batch/job"}...)
		adapters, _ := jobframework.GetMultiKueueAdapters(enabledIntegrations)

	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() {

	var _ = ginkgo.Describe("Multikueue with disabled integrations", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {
	var _ = ginkgo.Describe("Multikueue with some integration disabled", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {

GetMultiKueueAdapters() returns only registered and enabled adapters #3603

Are you sure you want to change the base?

GetMultiKueueAdapters() returns only registered and enabled adapters #3603

Conversation

mszadkow commented Nov 20, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Nov 20, 2024

netlify bot commented Nov 20, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mszadkow commented Nov 20, 2024

k8s-ci-robot commented Nov 21, 2024

mimowo commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Nov 20, 2024 •

edited

Loading