-
Notifications
You must be signed in to change notification settings - Fork 228
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
Added an option to control processing jobs with no queue name set #205
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g 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 |
/hold |
@@ -104,6 +133,11 @@ func (r *JobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R | |||
|
|||
log := ctrl.LoggerFrom(ctx).WithValues("job", klog.KObj(&job)) | |||
ctx = ctrl.LoggerInto(ctx, log) | |||
if queueName(&job) == "" && !r.processJobsWithoutQueueName { | |||
log.V(2).Info(fmt.Sprintf("%s annotation is not set, ignoring the job", constants.QueueAnnotation)) |
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.
V(3)?
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.
done
@@ -221,3 +244,38 @@ var _ = ginkgo.Describe("Job controller", func() { | |||
}, framework.Timeout, framework.Interval).Should(gomega.BeTrue()) | |||
}) | |||
}) | |||
|
|||
var _ = ginkgo.Describe("Job controller for workloads with no queue set", func() { |
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.
nit: I would use a single ginkgo.Describe
with two ginkgo.Context
, because you are describing the same controller.
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.
the indentation is too much for me :)
fwk.Teardown() | ||
}) | ||
ginkgo.It("Should reconcile jobs only when queue is set", func() { | ||
ginkgo.By("checking the job gets suspended when created unsuspended") |
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.
Did you forget a line for this?
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.
done.
Wait, I see the option, but I don't see the command line flag for it. |
…e name set; by default those jobs are not processed
No command-line flag, this is an option in the controller that we will expose via Component Config. I don't want to add a flag just so we deprecate it right away. I will send a PR after this one for CC, it is not that complicated. |
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.
/lgtm
/hold cancel |
@@ -104,6 +133,11 @@ func (r *JobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R | |||
|
|||
log := ctrl.LoggerFrom(ctx).WithValues("job", klog.KObj(&job)) | |||
ctx = ctrl.LoggerInto(ctx, log) | |||
if queueName(&job) == "" && !r.processJobsWithoutQueueName { |
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.
Q: Should this be queueName(&job) || "" && !r.processJobsWithoutQueueName
, if users don't set the queue name, we do not have a default queue name for them now IIRC.
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.
Can you clarify again the question? this basically says if r.processJobsWithoutQueueName=false
, then don't manage jobs that don't set the queue-name.
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.
A default queue name can be added, but it needs to be set via a webhook at job creation time if desired.
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.
Can you clarify again the question? this basically says if
r.processJobsWithoutQueueName=false
, then don't manage jobs that don't set the queue-name.
I mean whether we should only check r.processJobsWithoutQueueName
here and if queueName not set, we will add a default queue name like default
automatically in ConstructWorkloadFor
. But this means we should have default queue and default cq in initialization.
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.
The problem we are trying to solve here is not that the queue is missing per se; see #63 (comment)
basically we don't want to miss up with potentially existing jobs when kueue is first installed.
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.
moved discussion here #208
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds an internal flag to control whether or not the job-controller reconciles jobs with no queue name set; by default those jobs are not processed.
This is important so users are not surprised that their existing running jobs get suspended when they first try Kueue.
This flag will be exposed to be configurable via ComponentConfig once we add it.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: