Repository navigation
TAS: filter irrelevant non-TAS pod updates - #10488
k8s-ci-robot merged 1 commit into
Conversation
k8s-ci-robot
added
release-note-none
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @MatteoFari. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
k8s-ci-robot
added
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| func shouldReconcilePodUpdate(oldPod, newPod *corev1.Pod) bool { | ||
| if !filterPod(newPod) { | ||
| return false | ||
| } | ||
| if oldPod.Spec.NodeName != newPod.Spec.NodeName { | ||
| return true | ||
| } | ||
| if !utilpod.IsTerminated(oldPod) && utilpod.IsTerminated(newPod) { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
I think this works well, so I can merge, but let me ask you an opinion about the alternative. I think the alternative will be easier to follow:
func belongsToNonTASCache(pod *corev1.Pod) bool {
if utiltas.IsTAS(pod) {
return false
} else if len(pod.Spec.NodeName) == 0 {
return false
} else if util.IsTerminated(pod) {
return false
}
return true
}Then:
Create: return belongsToNonTASCache(newPod)
Update: return belongsToNonTASCache(oldPod) != belongsToNonTASCache(newPod)
Delete: return belongsToNonTASCache(oldPod)
So basically in this way the Create and Delete are just special cases for Update, because in case of Create we can assume belongsToNonTASCache(oldPod=nil) = false, and in case of Delete analogously belongsToNonTASCache(newPod=nil) = false, so the other needs to be true.
cc let me also cc @gabesaba who worked on the original impl.
There was a problem hiding this comment.
I think the alternative is easier to follow too.
One thing I am a bit unsure about is the case where the Pod changes from belonging to the non-TAS cache to not belonging anymore, while the new Pod is still not terminated. Right now Reconcile always fetches the Pod and calls TASCache().Update(&pod, log), and that cache update does not enforce belongsToNonTASCache on its own.
So I think it is safe only if we also handle that case by deleting the Pod from the cache instead of updating it. Otherwise a transition like scheduled non-TAS -> scheduled TAS could still reconcile, but then add/update the new Pod in the cache instead of removing it.
There was a problem hiding this comment.
We had this discussion during the original implementation, and decided that we don't need to handle the non-TAS to TAS case: #8484 (comment)
There was a problem hiding this comment.
Indeed, but "don't need to handle" does not imply "don't handle", and my attention back then wasn't focused on the corner case.
Now looking at this it seems support that only requires if belongsToNonTASCache() Update else DeletePodByKey() inside the Reconcile.
And at the expense of the single "if" we also express the intention better, so that the reader of the Reconcile function does not need to worry what is the transitioning.
There was a problem hiding this comment.
This sounds good to me. It mitigates cache corruption risk by explicitly handling all cases (even ones that require unexpected/adversarial user intervention)
There was a problem hiding this comment.
@MatteoFari let's update as per #10488 (comment) + the extra "if" in the Reconcile. Feel free to still keep ShouldReconcilePodUpdate as the wrapper for the transition detection check.
|
/test pull-kueue-test-integration-baseline-main |
Signed-off-by: Matteo Fari <matteofari06@gmail.com>
e921413 to
74a0c89
Compare
mimowo
left a comment
mimowo
left a comment
There was a problem hiding this comment.
Thank you 👍
/lgtm
/approve
/cherrypick release-0.17
/cherrypick release-0.16
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: cf8fb320761cabbbec4826526b66389b67cef319 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatteoFari, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds 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 |
k8s-ci-robot
added
release-note
|
/cherrypick release-0.17 |
|
@mimowo: new pull request created: #10507 DetailsIn 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. |
|
@mimowo: new pull request created: #10508 DetailsIn 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. |
What type of PR is this?
/kind bug
/area tas
What this PR does / why we need it:
Filters Pod updates that do not matter in NonTasUsageReconciler.
Before this change, the controller reconciled every update for scheduled non-TAS Pods. Now it only reconciles when a non-TAS Pod gets scheduled, moves to a different node, or finishes.
Which issue(s) this PR fixes:
Fixes #10269
Special notes for your reviewer:
Does this PR introduce a user-facing change?