-
Notifications
You must be signed in to change notification settings - Fork 262
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
KEP 3589: Uniformly filter manageJobsWithoutQueueNames by namespace #3595
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dgrove-oss The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
In other words, the VAP must implement the same functionality as `jobframework.IsOwnerManagedByKueue` | ||
and cross check controller-references against managed kinds. This approach may be challenging to scale | ||
as the number of Kueue managed kinds increases and ancestor trees become deeper. | ||
For example in the not so distant future we might see: AppWrapper->PyTorchJob->JobSet->Job->Pod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be enough to do the same as is done in jobframework.IsOwnerManagedByKueue
and just check if the parent is manageable by Kueue? So ancestor tree size wouldn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to phrase this better. It isn't that the check has to actually crawl up the ancestor tree. It's that the check has to consider each of the kinds that could be Kueue-managed parents. So it needs a longer list of kinds and it has to somehow know that a particular Kind's Kueue integration is enabled. For kinds like Job or JobSet that could have multiple Kueue-managed parent Kinds, each has to be covered by the check.
jobs without queue names are only managed if both `manageJobsWithoutQueueName` is true and | ||
the jobs namespace matches the `managedJobsNamespaceSelector`. | ||
|
||
Configuration parsing during startup is extended to give a deprecation notice if `podOptions.namespaceSelector` is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note, the proposed configuration doesn't have a way of impersonating podOptions.podSelector
. Would we be okay with deprecating it anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think podOptions.podSelector
is orthogonal to podOptions.namespaceSelector
. This KEP is only proposing to deprecate podOptions.namespaceSelector
in favor of the new namespaceSelector that would apply to all integrations.
What type of PR is this?
/kind feature
What this PR does / why we need it:
KEP to propose a design for #3589
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?