[MultiKueue] Introduce Admission Check Validation feature gate #3254

mszadkow · 2024-10-17T09:36:31Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

SingleInstanceInClusterQueue and FlavorIndependent are confusing to the multi-cluster users.
We need to wrap them with feature gate, but preserve the logic for MultiKueue Admission Checks constraints without those status conditions.

Which issue(s) this PR fixes:

Fixes #3094

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED: the Admission check status conditions “FlavorIndependent” and “SingleInstanceInClusterQueue” are no longer supported by default.
If you were using any of these conditions for your external AdmissionCheck you need to enable the `AdmissionCheckValidationRules` feature gate. 
For the future releases you will need to provide validation by an external controller.

k8s-ci-robot · 2024-10-17T09:36:33Z

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

k8s-ci-robot · 2024-10-17T09:36:36Z

[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 alculquicondor 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-10-17T09:36:47Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`c8c32ad`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67123b0f6e516600080851a1
😎 Deploy Preview	https://deploy-preview-3254--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mszadkow · 2024-10-17T09:37:00Z

/ok-to-test

pkg/features/kube_features.go

mimowo · 2024-10-17T09:50:57Z

/cc

…tAdmissionCheck status conditions with a feature gate. Preserve the logic without the status conditions.

k8s-ci-robot · 2024-10-18T11:22:47Z

@mszadkow: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-test-unit-main	`c8c32ad`	link	true	`/test pull-kueue-test-unit-main`

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.

mimowo

Overall this looks good.

I'm leaning to use dedicated fields in the cache for the checks so that there is a clear separation of the new and old mechanism. It will make it easier to drop the old mechanism in 0.10. Maybe it would also give us more control over the reasons, it will be better to have dedicated reasons.

mimowo · 2024-10-18T11:15:08Z

pkg/cache/cache.go

+	// TBD: This might not be needed, but we could keep it for readability
+	// because SingleInstanceInClusterQueue and FlavorIndependent will be false if AdmissionCheckValidationRules is off
+	singleInstanceInClusterQueue, flavorIndependent := false, false
+	if features.Enabled(features.AdmissionCheckValidationRules) {


Gating this code makes sense, but I think for reasability we can do:

newAC := AdmissionCheck{ Active Controller // by default SingleInstanceInClusterQueue and FlavorIndependent to false } if features.Enabled(features.AdmissionCheckValidationRules) { newAC. SingleInstanceInClusterQueue =apimeta.IsStatusConditionTrue(ac.Status.Conditions, kueue.AdmissionChecksSingleInstanceInClusterQueue) newAC.FlavorIndependent = apimeta.IsStatusConditionTrue(ac.Status.Conditions, kueue.FlavorIndependentAdmissionCheck) } c.admissionChecks[ac.Name] = newAC

mimowo · 2024-10-18T11:17:04Z

pkg/cache/clusterqueue.go

-			reasons = append(reasons, kueue.ClusterQueueActiveReasonMultipleSingleInstanceControllerAdmissionChecks)
-			for _, controller := range utilmaps.SortedKeys(c.multipleSingleInstanceControllersChecks) {
-				messages = append(messages, fmt.Sprintf("only one AdmissionCheck of %v can be referenced for controller %q", c.multipleSingleInstanceControllersChecks[controller], controller))
+		if features.Enabled(features.AdmissionCheckValidationRules) {


IIUC we don't need any diff here if we used a dedicated cache field.
Then we can also easier drop the code in 0.10. For the hard-coded validation we can use
multipleMultiKueueChecks and perFlavorMultiKueueChecks

mimowo · 2024-10-18T11:17:35Z

pkg/cache/clusterqueue.go

@@ -333,11 +346,22 @@ func (c *clusterQueue) updateWithAdmissionChecks(checks map[string]AdmissionChec
 				inactive = append(inactive, acName)
 			}
 			checksPerController[ac.Controller] = append(checksPerController[ac.Controller], acName)
-			if ac.SingleInstanceInClusterQueue {
+
+			if features.Enabled(features.AdmissionCheckValidationRules) {


Similarly here, avoid the diff by using dedicated cache fields for the new checks

mimowo · 2024-10-18T11:20:59Z

pkg/cache/clusterqueue_test.go

+			wantReason:  "MultipleSingleInstanceControllerAdmissionChecks",
+			wantMessage: "Can't admit new workloads: Cannot use multiple MultiKueue AdmissionChecks on the same ClusterQueue.",


When the feature is disabled set reason as MultipleMultiKueueAdmissionChecks

mimowo · 2024-10-18T11:21:55Z

pkg/cache/clusterqueue_test.go

+			wantReason:  "FlavorIndependentAdmissionCheckAppliedPerFlavor",
+			wantMessage: "Can't admit new workloads: MultiKueue AdmissionCheck cannot be specified per flavor.",


When the gate is disabled use reason MutliKueueAdmissionCheckAppliedPerFlavor

mimowo · 2024-10-18T11:22:58Z

test/integration/controller/core/clusterqueue_controller_test.go

+				{
+					Type:    kueue.ClusterQueueActive,
+					Status:  metav1.ConditionFalse,
+					Reason:  "MultipleSingleInstanceControllerAdmissionChecks",


Make this specific as in the other comment.

trasc · 2024-10-18T13:25:32Z

/uncc

mbobrovskyi · 2024-10-18T15:05:13Z

test/integration/multikueue/multikueue_test.go

+
+		ginkgo.By("Disabling AdmissionCheckValidationRules feature", func() {
+			gomega.Expect(features.SetEnable(features.AdmissionCheckValidationRules, false)).To(gomega.Succeed())
+		})


I think it should be disabled by default, no?

mbobrovskyi · 2024-10-18T15:06:55Z

test/integration/multikueue/multikueue_test.go

+		ginkgo.By("Enabling AdmissionCheckValidationRules feature", func() {
+			gomega.Expect(features.SetEnable(features.AdmissionCheckValidationRules, true)).To(gomega.Succeed())
+		})


Suggested change

ginkgo.By("Enabling AdmissionCheckValidationRules feature", func() {

gomega.Expect(features.SetEnable(features.AdmissionCheckValidationRules, true)).To(gomega.Succeed())

})

ginkgo.By("Enabling AdmissionCheckValidationRules feature", func() {

features.SetFeatureGateDuringTest(ginkgo.GinkgoTB(), features.ConfigurableResourceTransformations, true)

})

mbobrovskyi · 2024-10-18T15:26:46Z

site/content/en/docs/installation/_index.md

@@ -255,6 +255,7 @@ The currently supported features are:
 | `TopologyAwareScheduling`             | `false` | Alpha      | 0.9   |       |
 | `ConfigurableResourceTransformations` | `false` | Alpha      | 0.9   |       |
 | `WorkloadResourceRequestsSummary`     | `false` | Alpha      | 0.9   |       |
+| `AdmissionCheckValidationRules`       | `false` | Alpha      | 0.9   | 0.9   |


k8s-ci-robot · 2024-10-19T02:45:46Z

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 17, 2024

k8s-ci-robot requested review from alculquicondor and trasc October 17, 2024 09:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 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 Oct 17, 2024

mimowo reviewed Oct 17, 2024

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from mimowo October 17, 2024 09:51

mszadkow force-pushed the feature/disable-confusing-ac-conditions branch from 705820a to 219414b Compare October 18, 2024 10:29

mszadkow changed the title ~~[WIP] Admission Check Validation Checks~~ [MultiKueue] Admission Check Validation Checks Oct 18, 2024

Wrap AdmissionChecksSingleInstanceInClusterQueue and FlavorIndependen…

c8c32ad

…tAdmissionCheck status conditions with a feature gate. Preserve the logic without the status conditions.

mszadkow force-pushed the feature/disable-confusing-ac-conditions branch from 219414b to c8c32ad Compare October 18, 2024 10:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024

mszadkow changed the title ~~[MultiKueue] Admission Check Validation Checks~~ [MultiKueue] Introduce Admission Check Validation feature gate Oct 18, 2024

mszadkow marked this pull request as ready for review October 18, 2024 11:10

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 Oct 18, 2024

k8s-ci-robot requested a review from mbobrovskyi October 18, 2024 11:10

mimowo reviewed Oct 18, 2024

View reviewed changes

k8s-ci-robot removed the request for review from trasc October 18, 2024 13:25

mbobrovskyi reviewed Oct 18, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MultiKueue] Introduce Admission Check Validation feature gate #3254

[MultiKueue] Introduce Admission Check Validation feature gate #3254

mszadkow commented Oct 17, 2024 •

edited

Loading

k8s-ci-robot commented Oct 17, 2024

k8s-ci-robot commented Oct 17, 2024

netlify bot commented Oct 17, 2024 •

edited

Loading

mszadkow commented Oct 17, 2024

mimowo commented Oct 17, 2024

k8s-ci-robot commented Oct 18, 2024

mimowo left a comment

mimowo Oct 18, 2024

mimowo Oct 18, 2024

mimowo Oct 18, 2024

mimowo Oct 18, 2024

mimowo Oct 18, 2024

mimowo Oct 18, 2024

trasc commented Oct 18, 2024

mbobrovskyi Oct 18, 2024 •

edited

Loading

mbobrovskyi Oct 18, 2024

mbobrovskyi Oct 18, 2024

k8s-ci-robot commented Oct 19, 2024

		wantReason: "MultipleSingleInstanceControllerAdmissionChecks",
		wantMessage: "Can't admit new workloads: Cannot use multiple MultiKueue AdmissionChecks on the same ClusterQueue.",

		wantReason: "FlavorIndependentAdmissionCheckAppliedPerFlavor",
		wantMessage: "Can't admit new workloads: MultiKueue AdmissionCheck cannot be specified per flavor.",

	\| `AdmissionCheckValidationRules` \| `false` \| Alpha \| 0.9 \| 0.9 \|
	\| `AdmissionCheckValidationRules` \| `false` \| Deprecated \| 0.9 \| 0.9 \|

[MultiKueue] Introduce Admission Check Validation feature gate #3254

Are you sure you want to change the base?

[MultiKueue] Introduce Admission Check Validation feature gate #3254

Conversation

mszadkow commented Oct 17, 2024 • edited Loading

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 Oct 17, 2024

k8s-ci-robot commented Oct 17, 2024

netlify bot commented Oct 17, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

mszadkow commented Oct 17, 2024

mimowo commented Oct 17, 2024

k8s-ci-robot commented Oct 18, 2024

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

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

trasc commented Oct 18, 2024

mbobrovskyi Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 19, 2024

mszadkow commented Oct 17, 2024 •

edited

Loading

netlify bot commented Oct 17, 2024 •

edited

Loading

mbobrovskyi Oct 18, 2024 •

edited

Loading