Merge affinity from podtempalte and affinity-assistant #5306

yuzp1996 · 2022-08-11T03:34:43Z

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:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

If the user provide affinity in podtempalte it will merge with affinity-assistant's affinity

action required: Need to check podtemplate make sure the change will not cause unexpected behaviour

tekton-robot · 2022-08-11T03:34:52Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

yuzp1996 · 2022-08-11T03:35:26Z

Need to add unit test.

yuzp1996 · 2022-08-11T03:36:34Z

/kind feature

dibyom · 2022-08-11T19:07:21Z

/ok-to-test

tekton-robot · 2022-08-11T19:11:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/internal/affinityassistant/transformer.go	100.0%	92.3%	-7.7

yuzp1996 · 2022-08-12T00:37:38Z

/retest-required

lbernick · 2022-08-12T13:46:02Z

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).

yuzp1996 · 2022-08-13T11:48:23Z

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 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.

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).

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!

lbernick · 2022-08-16T17:32:46Z

pkg/internal/affinityassistant/transformer.go

+			if p.Spec.Affinity == nil {
+				p.Spec.Affinity = &corev1.Affinity{}
+			}


I think this block is duplicated with the similar block in mergeAffinityWithAffinityAssistant-- we don't need both

lbernick · 2022-08-16T17:38:23Z

pkg/internal/affinityassistant/transformer.go

+}
+
+// podAffinityTermUsingAffinityAssistant achieves pod Affinity term for taskRun
+// pods so that taskRuns is scheduled to the Node were the Affinity Assistant pod


Suggested change

// 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

lbernick · 2022-08-16T17:39:29Z

pkg/internal/affinityassistant/transformer_test.go

+			},
+		},
+	}, {
+		description: "affinity annotation with a different name and pod contains podAffinity",


it might also be useful to have a test case for pod affinity with preferredduringschedulingignoredduringexecution

tekton-robot · 2022-08-16T22:21:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	92.0%	93.0%	1.0

tekton-robot · 2022-08-16T22:35:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	92.0%	93.0%	1.0

yuzp1996 · 2022-08-16T23:01:53Z

/retest-required

tekton-robot · 2022-08-16T23:07:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	92.0%	93.0%	1.0

yuzp1996 · 2022-08-16T23:38:58Z

/retest-required

abayer · 2022-08-24T20:48:56Z

/cc @vdemeester - I'm not familiar enough with this area of the code to lgtm without someone else chiming in. =)

yuzp1996 · 2022-08-27T13:59:14Z

/kind feature

yuzp1996 · 2022-08-29T02:08:06Z

/cc @abayer @vdemeester Hello, I would be very grateful if you could help review this PR！Thanks!

abayer · 2022-09-06T16:27:55Z

/test check-pr-has-kind-label

tekton-robot · 2022-09-06T16:27:57Z

@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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.

tekton-robot · 2022-09-08T08:17:18Z

[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:

~~OWNERS~~ [lbernick,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vdemeester · 2022-09-08T08:41:18Z

/lgtm

vdemeester · 2022-09-08T09:29:10Z

/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 <yuzp1996@qq.com>

tekton-robot · 2022-09-08T14:42:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	92.0%	93.0%	1.0

yuzp1996 · 2022-09-08T15:46:11Z

@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!

vdemeester · 2022-09-08T15:57:02Z

@yuzp1996 no worries, both would have remove the lgtm 😉
/lgtm

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 11, 2022

tekton-robot requested review from lbernick and pritidesai August 11, 2022 03:34

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2022

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 11, 2022

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch 4 times, most recently from e267086 to a7cbbcc Compare August 11, 2022 04:07

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 11, 2022

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch from a7cbbcc to e1bcf4b Compare August 11, 2022 05:35

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 11, 2022

yuzp1996 changed the title ~~[WIP]Merge affinity from podtempalte and affinity-assistant~~ Merge affinity from podtempalte and affinity-assistant Aug 11, 2022

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2022

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2022

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 12, 2022

lbernick approved these changes Aug 16, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2022

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch from 35692b2 to b75b067 Compare August 16, 2022 22:16

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch from b75b067 to 69d8b8a Compare August 16, 2022 22:30

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch from 69d8b8a to 366f3ba Compare August 16, 2022 23:03

vdemeester approved these changes Sep 8, 2022

View reviewed changes

tekton-robot assigned vdemeester Sep 8, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

yuzp1996 force-pushed the feat/merge-affinity-with-podtemplate branch from 366f3ba to 723d147 Compare September 8, 2022 14:37

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

tekton-robot merged commit 5d7cf8d into tektoncd:main Sep 8, 2022

yuzp1996 mentioned this pull request Sep 9, 2022

PipelineRun 's PodTemplate support affinity #5241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge affinity from podtempalte and affinity-assistant #5306

Merge affinity from podtempalte and affinity-assistant #5306

yuzp1996 commented Aug 11, 2022 •

edited

Loading

tekton-robot commented Aug 11, 2022

yuzp1996 commented Aug 11, 2022

yuzp1996 commented Aug 11, 2022

dibyom commented Aug 11, 2022

tekton-robot commented Aug 11, 2022

yuzp1996 commented Aug 12, 2022

lbernick commented Aug 12, 2022

yuzp1996 commented Aug 13, 2022

lbernick Aug 16, 2022

lbernick Aug 16, 2022

lbernick Aug 16, 2022

tekton-robot commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

yuzp1996 commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

yuzp1996 commented Aug 16, 2022

abayer commented Aug 24, 2022

yuzp1996 commented Aug 27, 2022

yuzp1996 commented Aug 29, 2022

abayer commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

tekton-robot commented Sep 8, 2022

vdemeester commented Sep 8, 2022

vdemeester commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

yuzp1996 commented Sep 8, 2022

vdemeester commented Sep 8, 2022

	// 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

Merge affinity from podtempalte and affinity-assistant #5306

Merge affinity from podtempalte and affinity-assistant #5306

Conversation

yuzp1996 commented Aug 11, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 11, 2022

yuzp1996 commented Aug 11, 2022

yuzp1996 commented Aug 11, 2022

dibyom commented Aug 11, 2022

tekton-robot commented Aug 11, 2022

yuzp1996 commented Aug 12, 2022

lbernick commented Aug 12, 2022

yuzp1996 commented Aug 13, 2022

lbernick Aug 16, 2022

Choose a reason for hiding this comment

lbernick Aug 16, 2022

Choose a reason for hiding this comment

lbernick Aug 16, 2022

Choose a reason for hiding this comment

tekton-robot commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

yuzp1996 commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

yuzp1996 commented Aug 16, 2022

abayer commented Aug 24, 2022

yuzp1996 commented Aug 27, 2022

yuzp1996 commented Aug 29, 2022

abayer commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

tekton-robot commented Sep 8, 2022

vdemeester commented Sep 8, 2022

vdemeester commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

yuzp1996 commented Sep 8, 2022

vdemeester commented Sep 8, 2022

yuzp1996 commented Aug 11, 2022 •

edited

Loading