Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Single instance admission check. #1635

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Single instance admission check. #1635

merged 3 commits into from
Mar 11, 2024

Conversation

trasc
Copy link
Contributor

@trasc trasc commented Jan 23, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

  • Add the possibility for an AdmissionCheck controller to mark the AdmissionCheck manage by it for single usage within a ClusterQueue.
  • Only allow one multikueue managed AdmissionCheck to be added to one ClusterQueue.

Which issue(s) this PR fixes:

Relates to #693

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot
Copy link
Contributor

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 k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 23, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 23, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit f086d3c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65eea836a6699f0008522c5c

@trasc
Copy link
Contributor Author

trasc commented Jan 23, 2024

/test all

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2024
@trasc trasc force-pushed the ac-single-instance branch from 12e3dff to 182fc18 Compare January 23, 2024 10:35
@trasc trasc marked this pull request as ready for review January 23, 2024 10:35
@k8s-ci-robot 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 Jan 23, 2024
@trasc trasc mentioned this pull request Jan 26, 2024
Merged
mimowo
mimowo reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass to make progress, but I think this should not be configurable

pkg/cache/clusterqueue_test.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue_test.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue_test.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue_test.go Outdated Show resolved Hide resolved
@trasc trasc force-pushed the ac-single-instance branch from 26bb442 to 0a36eb1 Compare January 30, 2024 15:08
mimowo
mimowo reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor test question / ask. The code LGTM.

However, I still think this could be simpler for us (in code), and to users if we have just "MultipleMultiKueueAdmissionChecks" as the reason for being inactive, and not make this thing configurable, because as pointed out, this is MultiKueue specific. I think seeing "MultipleSingleInstanceControllerChecks" would be unclear to users.

/assign @tenzen-y @alculquicondor

pkg/cache/clusterqueue_test.go Show resolved Hide resolved
@alculquicondor
Copy link
Contributor

The release note should be something that is easy to understand for an end-user.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
@trasc trasc force-pushed the ac-single-instance branch from 0a36eb1 to bf79060 Compare January 31, 2024 09:01
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 31, 2024
@trasc trasc force-pushed the ac-single-instance branch from bf79060 to a1006f4 Compare January 31, 2024 10:33
mimowo
mimowo reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the caveat that it is still surfaced at the API level, in status. I think we could hardcode this entirely, but don't want to block it.

pkg/controller/admissionchecks/multikueue/admissioncheck.go Outdated Show resolved Hide resolved
@trasc trasc changed the title Add single instance admission check controllers list. Single instance admission check. Jan 31, 2024
tenzen-y
tenzen-y reviewed Jan 31, 2024
View reviewed changes
apis/kueue/v1beta1/admissioncheck_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/admissioncheck_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/admissioncheck_types.go Outdated Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/admissioncheck.go Outdated Show resolved Hide resolved
@mimowo
Copy link
Contributor

mimowo commented Feb 20, 2024

I don't think it's a good idea hard-code a list of admission-check controllers in the cache given the plug-in nature of these controllers. (I think we have already discussed this).

I see, I don't recall this discussion though. Anyway, I will defer the decision for API reviewers / approvers in Kueue. IMO making such decisions on PRs without a KEP is tricky, because it requires reverted order of reviews - first by api approvers, then other reviewers (at least for the field).

@tenzen-y
Copy link
Member

I don't think it's a good idea hard-code a list of admission-check controllers in the cache given the plug-in nature of these controllers. (I think we have already discussed this).

I see, I don't recall this discussion though. Anyway, I will defer the decision for API reviewers / approvers in Kueue. IMO making such decisions on PRs without a KEP is tricky, because it requires reverted order of reviews - first by api approvers, then other reviewers (at least for the field).

I fully agree with you. As much as possible, I'd like to have the dedicated KEP, but if @trasc thinks that discussion points are self-evident, we can discuss in the issue. Although, I guess that it would be better to create small KEP or update existing admission-check KEP.

@trasc
Copy link
Contributor Author

trasc commented Feb 20, 2024

To decrease the impact on the API we can use a condition instead of adding a new status field. It's a bit less clear in my opinion but it could do the job.

@alculquicondor @mimowo @tenzen-y WDYT?

@alculquicondor
Copy link
Contributor

alculquicondor commented Feb 20, 2024 •
edited
Loading

I agree with having a hardcoded list.
And we can leave having this configurable based on user feedback.

@trasc
Copy link
Contributor Author

trasc commented Feb 20, 2024

I agree with having a hardcoded list. And we can leave having this configurable based on user feedback.

The hard-coded list needs to be in cache or passed to cache and needs to contain the name of the multikueue admission check controller. The cache implementation should be agnostic in regards of the existence of a specific admission check controller.

@alculquicondor
Copy link
Contributor

Why the cache? isn't the workload reconcilier enough?

Passing the list of admission checks as an argument seems reasonable.

@trasc
Copy link
Contributor Author

trasc commented Feb 20, 2024

Why the cache? isn't the workload reconcilier enough?

Passing the list of admission checks as an argument seems reasonable.

Having two multikueue AC set in a ClusterQueue is a ClusterQueue configuration issue.
We manage the active state o the cluster Queues in cache.

@alculquicondor
Copy link
Contributor

alculquicondor commented Feb 20, 2024 •
edited
Loading

Oops, sorry, I meant to say the Clusterqueue reconciler. But ok, I can understand that the logic needs to be in the cache.

@trasc trasc force-pushed the ac-single-instance branch from 7a309ed to abd7250 Compare March 6, 2024 15:17
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2024
@trasc trasc force-pushed the ac-single-instance branch from abd7250 to 4973cdf Compare March 7, 2024 12:35
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2024
alculquicondor
alculquicondor reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProvisioningRequest should also be unique, although that can be done in a follow up

/approve

apis/kueue/v1beta1/admissioncheck_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/admissioncheck_types.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2024
@trasc trasc force-pushed the ac-single-instance branch from 4406915 to f086d3c Compare March 11, 2024 06:44
@alculquicondor
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d9a577d92a48bd9811c87078cb46b79aac8b67dc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, trasc

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot merged commit 3047d6f into kubernetes-sigs:main Mar 11, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.7 milestone Mar 11, 2024
@trasc trasc deleted the ac-single-instance branch March 11, 2024 14:05
vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024
@trasc @vsoch
Single instance admission check. (kubernetes-sigs#1635)
d5e6357 
* Add support for single instance admission check.

* Fix tests after rebase.

* Review Remarks
This was referenced Apr 19, 2024
Closed
Open
@mimowo mimowo mentioned this pull request Sep 19, 2024
Closed
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@trasc @kannon92
Single instance admission check. (kubernetes-sigs#1635)
b3b3fd3 
* Add support for single instance admission check.

* Fix tests after rebase.

* Review Remarks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y tenzen-y left review comments

@mimowo mimowo mimowo left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@alculquicondor alculquicondor

@tenzen-y tenzen-y

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.
5 participants
@trasc @k8s-ci-robot @alculquicondor @tenzen-y @mimowo