Ensure all group members own the workload [pod groups] #1683
Conversation
k8s-ci-robot
added
release-note
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/assign @yaroslava-serdiuk |
mimowo
changed the title
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: f29ecfebd83643e59760f1b1f44f7b4910f79c04
|
pkg/util/testing/wrappers.go
Outdated
| BlockOwnerDeletion: &blockDeletion, | ||
| }, | ||
| } | ||
| func (w *WorkloadWrapper) OwnerReference(gvk schema.GroupVersionKind, name, uid string, controller, blockDeletion *bool) *WorkloadWrapper { |
There was a problem hiding this comment.
Is this change necessary? This new approach leads to much more code.
There was a problem hiding this comment.
Not really, I reverted them by keeping the OwnerReference function. For the need of my unit test I now introduce SetOwnerReference and SetControllerReference. These are analogs of the controllerutil functions, which either pass (nil, nil) or (true, true). So now, the changes are scoped to the TestReconcile unit test. If you like this approach I could have a follow up to eliminate the OwnerReference function and replace it with either of the two new functions.
Still, inside this test I add a bunch of "SetOwnerReference" and "SetControllerReference" lines. The alternatives I see:
- have an extra param in the unit test like "verifyOwnerReferences" that would (1) skip the code of adding owner references, (2) use another set of cmpopts for the workload.
- have a dedicated unit test for this scenario, say TestReconcileWithOwnerReferences
- do not add unit tests for owner references
IMO none of the alternatives is tempting. Indeed adding a bunch of lines seems like a lot of code, but easy to understand. WDYT?
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
mimowo
force-pushed
the
fix-workload-deletion-with-unit-tests
branch
from
574a95e
to
e160356
Compare
k8s-ci-robot
removed
the
lgtm
mimowo
force-pushed
the
fix-workload-deletion-with-unit-tests
branch
from
e160356
to
0f52368
Compare
# Conflicts: # pkg/controller/jobframework/interface.go
mimowo
force-pushed
the
fix-workload-deletion-with-unit-tests
branch
from
0f52368
to
312157f
Compare
| func (w *WorkloadWrapper) OwnerReference(gvk schema.GroupVersionKind, name, uid string, controller, blockDeletion bool) *WorkloadWrapper { | ||
| w.OwnerReferences = []metav1.OwnerReference{ | ||
| { | ||
| APIVersion: gvk.GroupVersion().String(), | ||
| Kind: gvk.Kind, | ||
| Name: name, | ||
| UID: types.UID(uid), | ||
| Controller: &controller, | ||
| BlockOwnerDeletion: &blockDeletion, | ||
| }, | ||
| } | ||
| w.appendOwnerReference(gvk, name, uid, ptr.To(controller), ptr.To(blockDeletion)) | ||
| return w | ||
| } | ||
|
|
||
| func (w *WorkloadWrapper) SetControllerReference(gvk schema.GroupVersionKind, name, uid string) *WorkloadWrapper { | ||
| w.appendOwnerReference(gvk, name, uid, ptr.To(true), ptr.To(true)) | ||
| return w | ||
| } | ||
|
|
||
| func (w *WorkloadWrapper) SetOwnerReference(gvk schema.GroupVersionKind, name, uid string) *WorkloadWrapper { | ||
| w.appendOwnerReference(gvk, name, uid, nil, nil) | ||
| return w | ||
| } |
There was a problem hiding this comment.
for a follow up: we should only have OwnerReference and ControllerReference
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7a1292bfcc8bbf49a205835f2e980616ce24a240
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mimowo 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 |
k8s-ci-robot
added
the
approved
|
/release-note-edit Unreleased feature. |
k8s-ci-robot
added
release-note-none
What type of PR is this?
/kind bug
What this PR does / why we need it:
The root cause for the workload removal was that the workload was only owned by the originally created pods, but not replacements. Once the job completes the workload is marked finished, and finalizers from all pods are removed.
Now, the preempted pods (originally created) are getting removed from the API server at this point, and because they are the only owners of the workloads, the workload has no more owners. As a consequence the workload gets removed by the garbage-collector.
This is inconsistent with workloads completing without getting preempted. In that case the originally created pods are not deleted (even though they have their finalizers removed), and thus the workload is not removed.
Which issue(s) this PR fixes:
Fixes #1557
Special notes for your reviewer:
This PR will conflict with #1667 in
TestReconcileinpod_controller_test.go, because both add a test case. The other PR introduces asserts on events in the unit test, which will be handy to verify the event introduced in this PR.Does this PR introduce a user-facing change?