WIP: Use SSA in TrainJob Controller #2309
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
| //applyConfig := v2alpha1applyconfiguration.TrainJob(trainJob.Name, trainJob.Namespace).WithSpec( | ||
| // v2alpha1applyconfiguration.TrainJobSpec()) | ||
|
|
||
| if err := r.client.Patch(ctx, obj, client.Apply, client.FieldOwner("trainjob-controller"), client.ForceOwnership); err != nil { |
There was a problem hiding this comment.
@tenzen-y I'm wondering if we need to support applyconfigurations right away when we aren't sure of the exact fields that need to be specified? Even if we need to use applyconfiguration instead of passing the object, controller-runtime doesn't support it natively, we will have to pass on kubeclient instead.
r.client.Patch should take care of the create+update for now (I'll look into filtering out metadata fields though)
There was a problem hiding this comment.
I'm wondering if we need to support applyconfigurations right away when we aren't sure of the exact fields that need to be specified?
I agree with you. I do not imagine introducing the applyconfiguration now. Let's just directly use client.Object.
r.client.Patch should take care of the create+update for now (I'll look into filtering out metadata fields though)
I think so too.
I'm wondering if we can just copy objectMeta (only for uid, name, namespace, generation), typeMeta, and GVK like this: https://github.com/kubernetes-sigs/kueue/blob/90ef60760b849e475f7cf32d07669bb91bbb479f/pkg/workload/workload.go#L503
WDYT?
There was a problem hiding this comment.
Makes sense. I'll rebase this PR.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #
Checklist: