-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add validation for child jobs without ownerReference #865
Add validation for child jobs without ownerReference #865
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/assign @kerthcet |
/test pull-kueue-test-e2e-main-1-26 |
friendly ping @kerthcet :) |
0349b30
to
c025f54
Compare
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
c025f54
to
d8a854e
Compare
@kerthcet I have addressed your comments. PTAL. |
var allErrs field.ErrorList | ||
if _, exists := job.Object().GetAnnotations()[constants.ParentWorkloadAnnotation]; exists { | ||
if job.Object().GetOwnerReferences() == nil { | ||
allErrs = append(allErrs, field.Forbidden(parentWorkloadKeyPath, "must not add a parent workload annotation to job without OwnerReference")) |
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 wonder if we could also check that the owner is a well-known job owner.
And maybe there should be a Kueue configuration that allows to say which other APIs are job owners (in addition to the ones implemented in-tree).
This way, it should be possible to write external integration controllers and tell kueue to trust particular owner references and not suspend the jobs.
Once we have this trust, we can potentially simplify this code and not query weather the parent workload has a queue name
kueue/pkg/controller/jobframework/reconciler.go
Lines 121 to 137 in b463573
if !r.manageJobsWithoutQueueName && QueueName(job) == "" { | |
if isStandaloneJob { | |
log.V(3).Info("Neither queue-name label, nor parent-workload annotation is set, ignoring the job", | |
"queueName", QueueName(job), "parentWorkload", ParentWorkloadName(job)) | |
return ctrl.Result{}, nil | |
} | |
isParentJobManaged, err := r.IsParentJobManaged(ctx, job.Object(), namespacedName.Namespace) | |
if err != nil { | |
log.Error(err, "couldn't check whether the parent job is managed by kueue") | |
return ctrl.Result{}, err | |
} | |
if !isParentJobManaged { | |
log.V(3).Info("parent-workload annotation is set, and the parent job doesn't have a queue-name label, ignoring the job", | |
"parentWorkload", ParentWorkloadName(job)) | |
return ctrl.Result{}, nil | |
} | |
} |
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.
Didn't see any disadvantages to trust custom jobs, so I'm ok with this validation.
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.
It sounds good. Let me try to work on the feature in follow-ups.
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
/approve
/hold
for suggestion, but it can also be a follow up
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, tenzen-y 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 |
I would like to work on the feature in follow-ups. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
I added validation not to allow child jobs without ownerReference.
Which issue(s) this PR fixes:
Follow-up: #821 (comment)
Special notes for your reviewer:
Does this PR introduce a user-facing change?