cleanup: add workload Evict and Finish Clock and RoleTracker options#9701
cleanup: add workload Evict and Finish Clock and RoleTracker options#9701mykysha wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
release-note-none
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mykysha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| // finish workload and copy the status to the local one | ||
| return reconcile.Result{}, workload.Finish(ctx, w.client, group.local, remoteFinishedCond.Reason, remoteFinishedCond.Message, w.clock, w.roleTracker) | ||
| return reconcile.Result{}, workload.Finish(ctx, w.client, group.local, remoteFinishedCond.Reason, remoteFinishedCond.Message, | ||
| workload.FinishWithClock(w.clock), workload.FinishWithRoleTracker(w.roleTracker)) |
There was a problem hiding this comment.
the option names have the reduntand part 'Finish'. It was a problem before but with extra Params it looks even more weird. I'm wondering we could move the options and the helper Finish function to a subpackage like workload/finish. Analogously for evict. The common code could be moved to subpackage like workload/patching, WDYT? cc @mbobrovskyi . I think it is better to do now than grow the number of weird option functions.
mykysha
force-pushed
the
feat/options-workload-evict-finish
branch
from
f7d83e9 to
7884acc
Compare
k8s-ci-robot
added
size/XXL
mykysha
force-pushed
the
feat/options-workload-evict-finish
branch
from
7884acc to
9af0a7e
Compare
|
Hm, I don't undrstand, why the PR title suggests feature by the "feat" prefix: "feat: add workload Evict and Finish Clock and RoleTracker options". Otoh, there is no release note. So, I'm not sure if there is any user-observable change, or just refactoring? Or maybe this is a bug, because RoleTracker was not added before? |
mykysha
changed the title
|
Ah, I see this is cleanup. Thank you for updating. I'm now wondering if we need to pump the options via Finish and Evict functions. Maybe we could instead increment the finished metric in the workload_controller Update event, when we notice prevStatus != Finished, status=Finished - transition. wdyt @mbobrovskyi @mykysha ? |
|
I think it might work, let me take a closer look. It would definitely simplify the process if we would not need to pass the config to Evict and Finish |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Add Clock and RoleTracker options to Evict and Finish workload functions. Options can be reused afterwards to extend these functions
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Prerequisite for #9371
Does this PR introduce a user-facing change?