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

Implementation of KEP-3589: uniformly filter manageJobsWithoutQueueNames by namespace #3712

Merged
merged 16 commits into from
Dec 5, 2024
Merged

Implementation of KEP-3589: uniformly filter manageJobsWithoutQueueNames by namespace #3712

merged 16 commits into from
Dec 5, 2024

Conversation

dgrove-oss
Copy link
Contributor

@dgrove-oss dgrove-oss commented Dec 3, 2024 •
edited by k8s-ci-robot
Loading

What type of PR is this?

/feature

What this PR does / why we need it:

Implements core feature of KEP-3589 by filtering default suspension of newly created jobs by a namespace selector.

Which issue(s) this PR fixes:

Part of #3589

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds a managedJobsNamespaceSelector to the Kueue configuration that enables namespace-based control of whether Jobs submitted without a `kueue.x-k8s.io/queue-name` label are managed by Kueue for all supported Job Kinds.

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 3, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 3, 2024
@netlify Netlify
Copy link

netlify bot commented Dec 3, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit c2f149a
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6750e6169af3480008d5ff68

mimowo
mimowo reviewed Dec 3, 2024
View reviewed changes
keps/3589-manage-jobs-selectively/kep.yaml Outdated Show resolved Hide resolved
pkg/config/validation.go Outdated Show resolved Hide resolved
pkg/controller/jobframework/defaults.go Outdated Show resolved Hide resolved
pkg/controller/jobframework/defaults.go Outdated Show resolved Hide resolved
pkg/controller/jobframework/defaults.go Show resolved Hide resolved
@dgrove-oss dgrove-oss force-pushed the 3589-impl branch 2 times, most recently from 7630192 to 11e7309 Compare December 4, 2024 02:35
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 4, 2024
@dgrove-oss
Copy link
Contributor Author

unit and existing integration tests now passing.

I don't quite understand the kueue-verify-main error is trying to tell me. Any hints?

pkg/controller/jobframework/defaults.go:26: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(sigs.k8s.io/kueue) -s blank -s dot (gci)
	"sigs.k8s.io/controller-runtime/pkg/client"

ideally, we'd add additional integration tests that verified the new feature across all the integrations, but I ran out of time today. Could wait for a followup PR if necessary due to 0.10 deadline.

tenzen-y
tenzen-y reviewed Dec 4, 2024
View reviewed changes
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.

I did not review the entire codes. I just left a comment how to resolve CI errors.

pkg/controller/jobframework/defaults.go Outdated Show resolved Hide resolved
@mimowo
Copy link
Contributor

mimowo commented Dec 4, 2024 •
edited
Loading

ideally, we'd add additional integration tests that verified the new feature across all the integrations, but I ran out of time today. Could wait for a followup PR if necessary due to 0.10 deadline.

I think we are good to move the release for tomorrow or EOW.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2024
dgrove-oss and others added 14 commits December 4, 2024 13:18
@dgrove-oss
core implementation of managedJobsNamespaceSelector
786ee93
@dgrove-oss
add commented out example managedJobsNamespaceSelector to config
023836d
@dgrove-oss
validation of managedJobsNamespaceSelector
f609516
@dgrove-oss
defaulting of managedJobsNamespaceSelector
b48bee5
@dgrove-oss
update generated files
7217982
@dgrove-oss
generate-apiref
5e0b237
@dgrove-oss
kep status is implementable
b8ba6eb
@dgrove-oss
add ManagedJobsNamespaceSelector to featuregate table
048302e
@dgrove-oss
linter fix: context.Context should be first argument
c4197a7
@dgrove-oss
address comments from first round of reviews
7c10137 
1. parse label selector during startup and used cached result
2. guard all uses with feature gate
@dgrove-oss
fixes to get unit tests to pass
5f15bb5
@dgrove-oss
linter fix
41e92bd
@dgrove-oss
updating integration tests to pass new jobframework option
85e66fc
@dgrove-oss @tenzen-y
Update pkg/controller/jobframework/defaults.go
50563f7 
Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2024
@dgrove-oss
Copy link
Contributor Author

/cc @mimowo @tenzen-y I've added integration tests for the new functionality across most of the job integrations. PR should be in good shape for hopefully final review.

@dgrove-oss dgrove-oss changed the title Core implementation of KEP-3589: uniformly filter manageJobsWithoutQueueNames by namespace Implementation of KEP-3589: uniformly filter manageJobsWithoutQueueNames by namespace Dec 4, 2024
@mimowo
Copy link
Contributor

mimowo commented Dec 5, 2024 •
edited
Loading

/lgtm
/approve
Please add "Part of #3589" in the description. As for a follow up, I think we should respect the new selector in webhooks for deployment and StatefulSet, before we set the PodGroup labels and annotations. Feel free to follow up within the issue. We also need a docs update.

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

LGTM label has been added.

Git tree hash: a7b213485338e5debf15a13714c587eca8336d90

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrove-oss, mimowo

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 Dec 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit 17e0fe8 into kubernetes-sigs:main Dec 5, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.10 milestone Dec 5, 2024
@mimowo mimowo mentioned this pull request Dec 5, 2024
Closed
3 tasks
@dgrove-oss dgrove-oss deleted the 3589-impl branch December 5, 2024 14:29
@tenzen-y
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 11, 2024
@tenzen-y
Copy link
Member

/release-note-edit

Adds a managedJobsNamespaceSelector to the Kueue configuration that enables namespace-based control of whether Jobs submitted without a `kueue.x-k8s.io/queue-name` label are managed by Kueue for all supported Job Kinds.

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

@mbobrovskyi mbobrovskyi mbobrovskyi left review comments

@mimowo mimowo mimowo left review comments

@PBundyra PBundyra Awaiting requested review from PBundyra

@trasc trasc Awaiting requested review from trasc

Assignees

@mimowo mimowo

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.10
Development

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