-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Merge affinity from podtempalte and affinity-assistant #5306
Merge affinity from podtempalte and affinity-assistant #5306
Conversation
Hi @yuzp1996. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Need to add unit test. |
/kind feature |
e267086
to
a7cbbcc
Compare
a7cbbcc
to
e1bcf4b
Compare
/ok-to-test |
The following is the coverage report on the affected files.
|
/retest-required |
I'm not sure whether this is a good idea or not (maybe it is! I'm just not sure). It seems like in this PR, if a user has set affinity from the pod template, both that affinity and the affinity assistant affinity will be applied to the resulting pod. I'm worried this might result in some confusing behavior. What if the user applies a pod affinity that isn't really compatible with the affinity assistant? I'm also wondering if any node affinity specified by the user should be applied to the affinity assistant pod as well since it's scheduled first (again, not sure what the right answer is here). |
I think maybe I understand your concern and here is my thoughts. If I set affinity in pipelinerun's podtemplate I think the affinity priority in podtemplate should be the highest and it should not be overwritten by anything. On the other hand, I think affinity-assistant should only serve as an aid, helping to find better node but not change the way to find a better node. And if the user applies a pod affinity that isn't really compatible with the affinity assistant the result is that pod will not be scheduled and I guess it should be correct. In this case, the user can disable affinity-assistant or change the affinity in podtemplate to ensure a better node can be found.
Yes, I totally agree with you, we should set the affinity of podtemplate to affinity-assistant pod. Sorry, even with the help of google translate I don't know if I describe it clearly! If there is anything I have not described clearly, please point it out to me. Thanks! |
if p.Spec.Affinity == nil { | ||
p.Spec.Affinity = &corev1.Affinity{} | ||
} |
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.
I think this block is duplicated with the similar block in mergeAffinityWithAffinityAssistant
-- we don't need both
} | ||
|
||
// podAffinityTermUsingAffinityAssistant achieves pod Affinity term for taskRun | ||
// pods so that taskRuns is scheduled to the Node were the Affinity Assistant pod |
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.
// pods so that taskRuns is scheduled to the Node were the Affinity Assistant pod | |
// pods so that the taskRun is scheduled to the Node where the Affinity Assistant pod |
}, | ||
}, | ||
}, { | ||
description: "affinity annotation with a different name and pod contains podAffinity", |
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.
it might also be useful to have a test case for pod affinity with preferredduringschedulingignoredduringexecution
35692b2
to
b75b067
Compare
The following is the coverage report on the affected files.
|
b75b067
to
69d8b8a
Compare
The following is the coverage report on the affected files.
|
/retest-required |
69d8b8a
to
366f3ba
Compare
The following is the coverage report on the affected files.
|
/retest-required |
/cc @vdemeester - I'm not familiar enough with this area of the code to lgtm without someone else chiming in. =) |
/kind feature |
/cc @abayer @vdemeester Hello, I would be very grateful if you could help review this PR!Thanks! |
/test check-pr-has-kind-label |
@abayer: The specified target(s) for
The following commands are available to trigger optional jobs:
Use 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/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick, vdemeester 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 |
/lgtm |
/retest |
No matter user provide affinity in podtemplate or not the affinity-assistant will overwrite the affinity in pod. However if user set affinity in podtemplate merge podtemplate's affinity with affinity-assistant's affinity is better way. So we inject the affinity of the affinity-assitant into the affinity of the podtempalte as the affinity of the pod to make sure that all the affinities work properly. Related Issue: tektoncd#5241 Signed-off-by: yuzhipeng <[email protected]>
366f3ba
to
723d147
Compare
The following is the coverage report on the affected files.
|
@vdemeester sorry I force push because of rebasing main branch but cause removing lgtm. Could you please take another look again when you are fine? Thanks! |
@yuzp1996 no worries, both would have remove the lgtm 😉 |
Changes
No matter user provide affinity in podtemplate or not the
affinity-assistant will overwrite the affinity in pod.
However if user set affinity in podtemplate merge
podtemplate's affinity with affinity-assistant's affinity
is better way.
So we inject the affinity of the affinity-assitant into the
affinity of the podtempalte as the affinity of the pod to
make sure that all the affinities work properly.
Related Issue: #5241
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes