-
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
UT: workload marked as finished when job completes #1089
UT: workload marked as finished when job completes #1089
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor 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 |
eb3c0bf
to
708a6a6
Compare
/assign @tenzen-y |
708a6a6
to
55da13a
Compare
// +patchStrategy=merge | ||
// +patchMergeKey=type | ||
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` |
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.
Why do we need these markers to add unit tests?
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.
Because the mock client uses StrategicMerge
to implement SSA, which only looks at the patchStrategy, not the mapType.
There is an open issue to fix the mock client to be more like the real apiserver kubernetes/kubernetes#115598
However, in the meantime, @apelisse suggested that I add a patchStrategy. This also gives the ability for external controllers to use client-side apply and easily merge two lists of conditions.
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.
Thanks for clarifying. I'm a bit confused since IIUC, the StrategicMergePatch
can be used only for Client Side Apply (CSA), right?
However, by this PR, the controller uses SSA with StarategicMergePatch
to update workload condition:
kueue/pkg/workload/workload.go
Line 263 in 483ef8a
return c.Status().Patch(ctx, newWl, client.Apply, client.FieldOwner(managerPrefix+"-"+condition.Type)) |
Which apply strategy will the controller use to update workload conditions? CSA vs SSA
Should we use a customized fake patch client to avoid performance issues instead of adding these markers if CSA is used?
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.
Apply
means that it will use SSA.
It's only the fake client that implements SSA as it was CSA.
This is enough for our purposes for now.
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.
Ah, I see. So, CSA would be used only when unit tests.
Thanks again.
UT looks good to me. |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Add unit test for marking workloads as finished when a job completes.
The mock client simulates SSA using strategic merge, which only looks at the
patchStrategy
and notmapType
.Then, this PR also introduces the
patchStrategy:"merge"
. This also allows external controllers or CLIs to do client-side apply, merging two lists of conditions.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?