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

KEP-2936: LocalQueue defaulting #3652

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+124 −0
Open

KEP-2936: LocalQueue defaulting #3652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions keps/2936-local-queue-defaulting/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# KEP-2936: LocalQueue defaulting

<!-- toc -->
- [Summary](#summary)
- [Motivation](#motivation)
- [Goals](#goals)
- [Non-Goals](#non-goals)
- [Proposal](#proposal)
- [User Stories](#user-stories)
- [Story 1](#story-1)
- [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
- [Risks and Mitigations](#risks-and-mitigations)
- [Design Details](#design-details)
- [Test Plan](#test-plan)
- [Alternatives](#alternatives)
- [Default field in LocalQueue spec](#default-field-in-localqueue-spec)
- [Add default LocalQueue only to Workload object](#add-default-localqueue-only-to-workload-object)
<!-- /toc -->

## Summary

This KEP introduces a defaulting LocalQueue mechanism for the jobs that don't have
LocalQueue name label specified.

## Motivation

Simplify user experience with Kueue.

### Goals

- default LocalQueue for jobs that don't specify LocalQueue name label.

### Non-Goals

- create a default LocalQueue.

## Proposal
yaroslava-serdiuk marked this conversation as resolved.
Show resolved Hide resolved

If the Job doesn't have label for LocalQueue in its Spec and the LocalQueue
with name `default` is present in Job's namespace and LocalQueueDefaulting feature gate
is enabled, the `kueue.x-k8s.io/queue-name:default` anotation will be added to the Job.

The `default` LocalQueue itself should be created manually by administrator.

### User Stories

#### Story 1

Administrator created a single `default` LocalQueue per namespace. Users doesn't need to specify
the LocalQueue label in order for workload to be scheduled.

### Notes/Constraints/Caveats (Optional)

manageJobsWithoutQueueName doesn't work with this feature, since Kueue add the LocalQueue
label.

### Risks and Mitigations
yaroslava-serdiuk marked this conversation as resolved.
Show resolved Hide resolved

## Design Details
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using the reserved default name for the default localQueue, could we scrape the default queue-name from the cluster?

For example, we might want to prepare the dedicated .spec.default: true|false field in the LocalQueue, and then the webhook looks into the default queue-name when the webhook insert the queue-name to Jobs.

This idea is similar to storageClass.

Copy link
Contributor Author

@yaroslava-serdiuk yaroslava-serdiuk Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

.spec.default: true|false field in the LocalQueue

Having default field in the API brings it owns challenge, I mentioned it earlier in the issue: #2936 (comment), i.e. validating that only one LQ is default.

Since the goal is to simplify workflow, it seems having default LQ is simple and explicit.
However I'm not sure about having a defaulting under the feature gate. In case the feature goes to GA and users are not familiar with the defaulting mechanism, they may create the LQ with default name and jobs without queue label and expecting a different behaviour.
Maybe instead of feature gate it should be a part of kueue config.

Copy link
Contributor Author

@yaroslava-serdiuk yaroslava-serdiuk Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This idea is similar to storageClass.

StorageClass allows multiple defaults and the latest default is an actual default. I also considered this as an alternative (will add it to alternatives section).

I'm thinking about use case. 1) In the case described in the issue, when admin creates only one LQ that is supposed to use, then both solutions (default LQ and spec.default=true) would work fine.
2) In case when there many LQs and one of them is default, it seems having default LQ is better, because it easier to find a default LQ among hundreds of LQs in namespace and modify (not sure if it's possible, maybe recreate) in case of misconfiguration.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense.
I think that the custom default LQ name is still valid request, but it seems that the customization will require an additional mechanism like comparing creationTimestamp between multiple defaults LQs.

So, I agree with the fixed default LQ name, "default" so that we can start this feature as a minimum.
After we obtain some feedbacks for the custom default LQ name, we can revisit here, and consider the customization.


Update defautling webhook for all types of job to add `kueue.x-k8s.io/queue-name:default`
Copy link
Contributor

@mimowo mimowo Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you expand on the proposed approach? The options I see:

  1. default the queue-name label on the Job object in a webhook
  2. default only at the workload spec.queueName level

The webhook-based approach will not work OOTB for external Jobs. Also, with the MAP effort we will want to move away from webhooks over time. On the plus side we have compatibility with tooling.

(2.) would work OOTB for external Jobs. Also, (2.) is less committal, we could always do (1.) in a follow up if needed.

Maybe the issue for (2.) is the compatibility with other tooling like kueuectl (not sure where it looks now), but we could probably adjust them to lookup the workload.

If we go with (1.) we should provide some utility functions so that it is easy for other developers to follow the semantics for the external jobs.

All in all, I'm not sure which one is better, so would be good to put list the pros and cons, and put the rejected one to alternatives describing the reasons for rejecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The webhook-based approach will not work OOTB for external Jobs.

Could you elaborate why it won't work?

Also, with the MAP effort we will want to move away from webhooks over time.

What is MAP?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate why it won't work?

It will work, but not OOTB - the users need to implement the same semantics in their webhooks. AppWrapper is one of the frameworks, but there are also other in-house Jobs.

What is MAP?

Mutating Admission Policies. I think we could actually move the approach from webhook to MAP when it is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, (2.) may not work OOTB either - because if the webhook is not aware of the logic, then it would suspend the job without queue-name when manageJobsWithoutName: true (IIUC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, (2.) may not work OOTB either - because if the webhook is not aware of the logic, then it would suspend the job without queue-name when manageJobsWithoutName: true (IIUC)

yes, this is my understanding

Copy link
Contributor

Choose a reason for hiding this comment

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

It's going to be important that the webhook only do this for Jobs defined in namespaces where a designated default LocalQueue already exists. If the webhook acts without namespace awareness, then we still need the namespace-level filtering mechanism from KEP-3589

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the webhook should be namespace aware, but the awareness would come from KEP-3589, so that we don't duplicate namespace filtering effort.

Copy link
Contributor Author

@yaroslava-serdiuk yaroslava-serdiuk Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I check whether the defaultqueue exists in the namespace: #3610

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but this code does not filter based on the namespaces. If we have filtering, then Kueue would stay away from namespaces which don't pass the filter, even if "default" LQ exists.

Copy link
Member

Choose a reason for hiding this comment

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

If we go with (1.) we should provide some utility functions so that it is easy for other developers to follow the semantics for the external jobs.

I think that this should be enough for the in-house jobs. Additionally, we might want to expand the customJob documentation (https://kueue.sigs.k8s.io/docs/tasks/dev/integrate_a_custom_job/).

Basically, I think that natively supporting custom Jobs is not necessary in here's upstream repository, and we can focus on the upstream supported Jobs like batch/v1 Job, RayJob, and KFJobs.

label if the label is not present and the feature gate is enabled.

### Test Plan

[x] I/we understand the owners of the involved components may require updates to
existing tests to make this code solid enough prior to committing the changes
necessary to implement this enhancement.

## Alternatives

Copy link
Member

Choose a reason for hiding this comment

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

Could you add one more alternative, "default only at the workload spec.queueName level"?

Copy link
Member

Choose a reason for hiding this comment

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

/hold
This is still not addressed, yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

### Default field in LocalQueue spec

Add default field to LocalQueue spec, that could be set to true|false. There are two
scenarios possible:

- If only one LocalQueue is allowed to be set as default, then we should have
a validation webhook which validating the object based on other objects in
the cluster.
- If multiple queues could be set to default, we have to choose which one
is an actual default. Usually the creating timestamp is used for comparison.
However having multiple default LocalQueues with arbitrary names makes their management
more complex.

### Add default LocalQueue only to Workload object

This approach was rejected because:

- The approach is lacking visibility for the user.

- The approach depends on config.manageJobsWithoutQueueName value. If the value is set to false,
the job won't be managed by Kueue.
32 changes: 32 additions & 0 deletions keps/2936-local-queue-defaulting/kep.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
title: LocalQueue defaulting
kep-number: 2936
authors:
- "@yaroslava-serdiuk"
status: implementable
creation-date: 2024-11-25
reviewers:
- "@pbundyra"
- "@tenzen-y"
- "@mimowo"
approvers:
- "@mimowo"

# The target maturity stage in the current dev cycle for this KEP.
stage: alpha

# The most recent milestone for which work toward delivery of this KEP has been
# done. This can be the current (upcoming) milestone, if it is being actively
# worked on.
latest-milestone: "v0.10"

# The milestone at which this feature was, or is targeted to be, at each stage.
milestone:
alpha: "v0.10"
beta:
stable:

# The following PRR answers are required at alpha release
# List the feature gate name and the components for which it must be enabled
feature-gates:
- name: LocalQueueDefaulting
disable-supported: true