[multikueue] Remove remote objects synchronously when reachable. #2347
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
trasc
force-pushed
the
multikueue-delete-delegates-on-local-wl-delete
branch
from
85ee548
to
8c87ac6
Compare
|
/assign @mimowo |
| @@ -59,6 +61,7 @@ type wlReconciler struct { | |||
| clusters *clustersReconciler | |||
| origin string | |||
| workerLostTimeout time.Duration | |||
| deletedWlCache *utilmaps.SyncMap[string, *kueue.Workload] | |||
There was a problem hiding this comment.
I'm wondering if this could leak a key-value pair if MultiKueueGC removes the workload in the meanwhile, when the synchronous delete could fail, leaking the entry. Should we also delete the entry when performing MultiKueueGC?
There was a problem hiding this comment.
No, in the reconciler the group will be empty the object deletion skipped and the workload deleted from the cache.
There was a problem hiding this comment.
Hm, but I think there could be a race condition when the group contains a worker, but in the meanwhile the workload is removed by GC. In that case the delete request in the new code would probably fail as the object does not exist, and thus it would not delete the key. Can you double check?
There was a problem hiding this comment.
The NotFound errors are ignored during the object deletion, other errors will trigger a new reconcile that will eventually get to remove the cache entry.
(I have extended a bit the unit tests)
There was a problem hiding this comment.
Thanks for checking and the update to the tests, the extra check is useful
There was a problem hiding this comment.
/lgtm
/approve
@alculquicondor @tenzen-y I think it would be good to include this in 0.7.x line. WDYT?
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 83604b2ac7412b89720fa3ca5896a150db3a7714
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, trasc 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
|
ok with me to cherry-pick. |
SGTM |
|
/cherry-pick release-0.7 |
|
@trasc: new pull request created: #2360 In response to this:
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-sigs/prow repository. |
|
/release-note-edit |
…ernetes-sigs#2347) * [multikueue] Remove remote objects synchronously when reachable. * Extend unit test coverage. * Review remarks
…ernetes-sigs#2347) * [multikueue] Remove remote objects synchronously when reachable. * Extend unit test coverage. * Review remarks
What type of PR is this?
/kind feature
What this PR does / why we need it:
Remove the multikueue remote objects synchronously when reachable based on the filters cache workload content.
Which issue(s) this PR fixes:
Fixes #2320
Special notes for your reviewer:
Does this PR introduce a user-facing change?