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.

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

Introduce AdmissionCheckStrategy API, change assigning AdmissionCheck… #1960

Merged

Conversation

PBundyra
Copy link
Contributor

@PBundyra PBundyra commented Apr 9, 2024

…s to a Workload

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements changes described in #1935

Which issue(s) this PR fixes:

Fixes #1432

Special notes for your reviewer:

I've tested it manually e2e. In an incoming PR I'll add webhook validation.

Does this PR introduce a user-facing change?

You can define AdmissionChecks per ResourceFlavor in the ClusterQueue API, using `admissionChecksStrategy`

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 9, 2024
@k8s-ci-robot 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 Apr 9, 2024
Copy link

netlify bot commented Apr 9, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 181cdca
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/661e8cd95aa1d6000837408c

@PBundyra
Copy link
Contributor Author

PBundyra commented Apr 9, 2024

/assign @alculquicondor
/assign @tenzen-y
/assign @mimowo

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024
@alculquicondor
Copy link
Contributor

/release-note-edit

Users can define AdmissionChecks per ResourceFlavor in the ClusterQueue API.

@alculquicondor
Copy link
Contributor

The original note could have been interpreted as a breaking change, which is not the case.

@alculquicondor
Copy link
Contributor

/release-note-edit

Users can define AdmissionChecks per ResourceFlavor in the ClusterQueue API, using admissionChecksStrategy.

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved
pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved
pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved
site/static/examples/provisioning/plain-prov-req.yaml Outdated Show resolved Hide resolved
site/static/examples/jobs/jobset-sample.yaml Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
@PBundyra PBundyra force-pushed the admissioncheck-per-flavor branch 3 times, most recently from 8237aa8 to 7c77a63 Compare April 10, 2024 11:21
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

My first path is added.

apis/kueue/v1beta1/clusterqueue_types.go Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
pkg/workload/workload.go Outdated Show resolved Hide resolved
pkg/workload/workload_test.go Outdated Show resolved Hide resolved
pkg/workload/workload_test.go Outdated Show resolved Hide resolved
pkg/webhooks/clusterqueue_webhook.go Show resolved Hide resolved
pkg/webhooks/clusterqueue_webhook.go Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
@PBundyra PBundyra force-pushed the admissioncheck-per-flavor branch 6 times, most recently from 0992e5b to dbdc4a2 Compare April 11, 2024 09:17
@PBundyra PBundyra force-pushed the admissioncheck-per-flavor branch 2 times, most recently from 478a256 to 18dd2fa Compare April 12, 2024 13:56
@PBundyra
Copy link
Contributor Author

/retest

pkg/controller/core/workload_controller_test.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue_test.go Outdated Show resolved Hide resolved
pkg/cache/clusterqueue.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved
@alculquicondor
Copy link
Contributor

If the above error is a flaky, please open an issue.

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.

just 2 nits

pkg/cache/snapshot.go Outdated Show resolved Hide resolved
pkg/controller/core/workload_controller_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024
@PBundyra PBundyra force-pushed the admissioncheck-per-flavor branch 3 times, most recently from 2da7665 to 81067e7 Compare April 16, 2024 13:07
@alculquicondor
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2024
…s to a Workload

Add validation webhook

Add TestReconcile unit tests

Add webhook unit tests

Add an additional layer to the API that wraps rules for AdmissionChecks

    Add AdmissionCheckStrategy to ClusterQueues's cache

Change ClusterQueue cache

Sort AdmissionChecks in a Workload
@alculquicondor
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: bbfb53bfb4792ccaa7d3d14764d98b4e2c9e1d78

@k8s-ci-robot k8s-ci-robot merged commit ebecc56 into kubernetes-sigs:main Apr 16, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.7 milestone Apr 16, 2024
apis/kueue/v1beta1/clusterqueue_types.go Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Show resolved Hide resolved
pkg/cache/clusterqueue.go Show resolved Hide resolved
pkg/workload/workload_test.go Show resolved Hide resolved
@tenzen-y
Copy link
Member

Oops, this PR already has been merged :(

@tenzen-y
Copy link
Member

@PBundyra Sorry for my late response. Could you address my comments? Thanks.

vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024
…s to a Workload (kubernetes-sigs#1960)

Add validation webhook

Add TestReconcile unit tests

Add webhook unit tests

Add an additional layer to the API that wraps rules for AdmissionChecks

    Add AdmissionCheckStrategy to ClusterQueues's cache

Change ClusterQueue cache

Sort AdmissionChecks in a Workload
@alculquicondor
Copy link
Contributor

/release-note-edit

You can define AdmissionChecks per ResourceFlavor in the ClusterQueue API, using `admissionChecksStrategy`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AdmissionChecks should be configurable per flavor
5 participants