search

kubernetes-sigs / kueue

Kubernetes-native Job Queueing
https://kueue.sigs.k8s.io
Apache License 2.0
1.47k stars 262 forks source link

Cleanup legacy preemption logic #3602

Open gabesaba opened 5 days ago

gabesaba commented 5 days ago

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Cleanup MultiplePreemptions=false branch. Simplify code base.

Which issue(s) this PR fixes:

Fixes #3601

Special notes for your reviewer:

Does this PR introduce a user-facing change?

We deprecate legacy preemption logic, with MultiplePreemptions being the only behavior
k8s-ci-robot commented 5 days ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gabesaba Once this PR has been reviewed and has the lgtm label, please assign tenzen-y 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](https://github.com/kubernetes-sigs/kueue/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
netlify[bot] commented 5 days ago

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
Latest commit 70af31c3a1d0b3f0a574fc718f8e2534643aebde
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/673df02976843e000852785c
mimowo commented 5 days ago

Thank you for the cleanup. It basically means MultiplePreemptions is graduated to stable, so we should reflect that in the kube_features. cc @tenzen-y ?

EDIT: what we typically do is we set the feature gate to stable, but keep it declared, so that users are not broken on upgrade if they have --feature-gate=MultiplePreemptions=true. Then we drop the declaration one or two releases later.

mimowo commented 3 days ago

I think we shouldn't be cleaning up the logic while keeping the feature in Beta. I would be supportive of turning the PR into a feature which graduates the feature gate to stable.

tenzen-y commented 6 hours ago

Thank you for the cleanup. It basically means MultiplePreemptions is graduated to stable, so we should reflect that in the kube_features. cc @tenzen-y ?

EDIT: what we typically do is we set the feature gate to stable, but keep it declared, so that users are not broken on upgrade if they have --feature-gate=MultiplePreemptions=true. Then we drop the declaration one or two releases later.

Yes, we should keep the feature gate for a while like 2 minor versions.

mimowo commented 5 hours ago

@gabesaba please update it in kube_features, and just add a comment, "// remove in 1.12"