add tolerations to eventlistener #597

GwonsooLee · 2020-06-05T18:36:55Z

Changes

Add tolerations to eventlistener. This is needed when I want to make event listener to the tainted node.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

If all nodes are tainted for security and management issue, it is not possible to deploy event listener. (Of course.. it is possible to deploy by getting yaml file and modifying it, but then I have to keep two different files to manage)

If this feature is added, then user could add `tolerations` in EventListener crd, and deployments will be deployed with the same configuration.

linux-foundation-easycla · 2020-06-05T18:36:57Z

The committers are authorized under a signed CLA.

✅ Gwonsoo-Lee (bf2c48c)

tekton-robot · 2020-06-05T18:37:10Z

Hi @GwonsooLee. 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.

GwonsooLee · 2020-06-05T18:43:34Z

/assign @vtereso

dibyom · 2020-06-05T20:43:02Z

Hi, @GwonsooLee Thanks for the PR! This looks related to #505

Could you add a podTemplate field to the EventListener that contains this tolerations field instead of directly adding the tolerations field in the EventListner spec?

GwonsooLee · 2020-06-06T04:44:23Z

@dibyom Thanks for review. I added podTemplate to eventlistener and move the tolerations to it. In #505 , there are lots of things we could add to podTemplate, but this time I would like to add only tolerations feature because I still need time to deep dive into the code for better understanding :). I will try to add new features in podTemplate later. Please let me know if something's wrong wtih this PR.

dibyom · 2020-06-09T15:41:55Z

/ok-to-test

tekton-robot · 2020-06-09T15:43:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

tekton-robot · 2020-06-09T16:09:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

khrm · 2020-06-11T07:02:18Z

examples/eventlisteners/eventlistener.yaml

@@ -5,6 +5,12 @@ metadata:
  name: listener
 spec:
  serviceAccountName: tekton-triggers-example-sa
+# podTemplate:


Add a separate example of eventlistener with tolerations

dibyom

Thanks @GwonsooLee In addition to an example like @khrm mentioned, could we add some docs to this as well :
https://github.com/tektoncd/triggers/blob/master/docs/eventlisteners.md#syntax

Also, before we can merge we'd have to squash the commits!

dibyom · 2020-06-11T20:59:04Z

go.mod

@@ -13,7 +13,9 @@ require (
 	github.com/google/go-github/v31 v31.0.0
 	github.com/gorilla/mux v1.7.3
 	github.com/grpc-ecosystem/grpc-gateway v1.13.0 // indirect
+	github.com/onsi/ginkgo v1.10.2 // indirect


hmm...any idea why this got added 🤔 ?

I think it was added indirectly when I ran dependency script. I ran e2e test without it and it worked so that I removed it.

dibyom · 2020-06-11T21:10:59Z

test/eventlistener_scale_test.go

+		bldr.EventListenerPodTemplate(
+			bldr.EventListenerPodTemplateSpec(
+				bldr.EventListenerPodTemplateTolerations(nil),
+				// If you want to test toleration feature, please use this commented part.


Is the test commented out because it will fail in our CI runs?

No.. it worked even though I uncommented it. I commented out those lines because I think test process cannot verify it really works because I guess there is no tainted node for e2e test. I uncommented every lines about tolerations in test files.

dibyom · 2020-06-11T21:11:53Z

test/eventlistener_test.go

+				bldr.EventListenerPodTemplate(
+					bldr.EventListenerPodTemplateSpec(
+						bldr.EventListenerPodTemplateTolerations(nil),
+						// If you want to test toleration feature, please use this commented part.


Same question as above...is this failing in CI? If it is and it is really hard to test this on our current e2e setup, we can open an issue for it. But I think I'd prefer not having commented out bits of code

I left comment above! 👍

tekton-robot · 2020-06-12T00:39:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

tekton-robot · 2020-06-12T00:40:25Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

GwonsooLee · 2020-06-12T00:41:42Z

/test all

tekton-robot · 2020-06-12T00:43:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

tekton-robot · 2020-06-12T00:59:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

dibyom · 2020-06-15T14:49:00Z

/approve

tekton-robot · 2020-06-15T14:49:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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~~ [dibyom]

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

dibyom · 2020-06-16T17:16:36Z

/lgtm

dibyom · 2020-06-16T20:47:19Z

Just needs a rebase to resolve the go.mod conflicts

tekton-robot · 2020-06-18T13:18:38Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

tekton-robot · 2020-06-18T13:22:43Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

tekton-robot · 2020-06-18T13:23:53Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	69.9%	70.5%	0.5
test/builder/eventlistener.go	92.8%	83.1%	-9.6

GwonsooLee · 2020-06-18T13:47:28Z

/test pull-tekton-triggers-build-tests

GwonsooLee · 2020-06-18T14:02:21Z

I rebase master and I ran though e2e test.. but build test failed.. @dibyom, could you please let me know what I've been doing wrong..? I will try to figure out too! Thanks you in advance..

dibyom · 2020-06-18T14:41:34Z

The problem is :

pkg/resources/create.go:47:8: G601: Implicit memory aliasing in for loop. (gosec)
		r := &apiResource
		     ^

Its strange that this popped up in this PR given that this line was not touched 🤔

dibyom · 2020-06-18T14:53:32Z

Running gosec locally does not fail with this problem either 😕

dibyom · 2020-06-18T14:53:51Z

/test pull-tekton-triggers-build-tests

dibyom · 2020-06-18T17:25:20Z

Should be fixed by https://github.com/tektoncd/triggers/pull/620/files

dibyom · 2020-06-18T17:53:58Z

/test pull-tekton-triggers-build-tests

GwonsooLee · 2020-06-19T00:55:27Z

It seems a bug in gosec. Thank you so much for fixing this problem! 👍

dibyom · 2020-06-19T03:36:28Z

/lgtm

GwonsooLee · 2020-06-19T03:47:59Z

/test pull-tekton-triggers-integration-tests

tekton-robot requested review from dibyom and dlorenc June 5, 2020 18:36

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 5, 2020

tekton-robot assigned vtereso Jun 5, 2020

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 Jun 9, 2020

khrm reviewed Jun 11, 2020

View reviewed changes

dibyom reviewed Jun 11, 2020

View reviewed changes

GwonsooLee force-pushed the tolerations_in_eventlistener branch from 57a3c9d to 64771af Compare June 12, 2020 00:38

GwonsooLee force-pushed the tolerations_in_eventlistener branch from 64771af to 52a4cb6 Compare June 12, 2020 00:57

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2020

tekton-robot assigned dibyom Jun 16, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020

dibyom added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2020

GwonsooLee force-pushed the tolerations_in_eventlistener branch from 52a4cb6 to d94724c Compare June 18, 2020 13:16

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020

GwonsooLee force-pushed the tolerations_in_eventlistener branch from d94724c to 98375b8 Compare June 18, 2020 13:20

add eventlistener with tolerations

41d4fdd

GwonsooLee force-pushed the tolerations_in_eventlistener branch from 98375b8 to 41d4fdd Compare June 18, 2020 13:21

This was referenced Jun 18, 2020

Fix typo in conditional examples tektoncd/pipeline#2782

Merged

fix gosec error #620

Merged

dibyom removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2020

tekton-robot merged commit d2fcfc0 into tektoncd:master Jun 19, 2020

GwonsooLee deleted the tolerations_in_eventlistener branch June 19, 2020 03:57

add tolerations to eventlistener #597

add tolerations to eventlistener #597

Conversation

GwonsooLee commented Jun 5, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

linux-foundation-easycla bot commented Jun 5, 2020 • edited Loading

tekton-robot commented Jun 5, 2020

GwonsooLee commented Jun 5, 2020

dibyom commented Jun 5, 2020

GwonsooLee commented Jun 6, 2020

dibyom commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

khrm Jun 11, 2020

Choose a reason for hiding this comment

dibyom left a comment

Choose a reason for hiding this comment

dibyom Jun 11, 2020

Choose a reason for hiding this comment

GwonsooLee Jun 12, 2020

Choose a reason for hiding this comment

dibyom Jun 11, 2020

Choose a reason for hiding this comment

GwonsooLee Jun 12, 2020

Choose a reason for hiding this comment

dibyom Jun 11, 2020

Choose a reason for hiding this comment

GwonsooLee Jun 12, 2020

Choose a reason for hiding this comment

tekton-robot commented Jun 12, 2020

tekton-robot commented Jun 12, 2020

GwonsooLee commented Jun 12, 2020

tekton-robot commented Jun 12, 2020

tekton-robot commented Jun 12, 2020

dibyom commented Jun 15, 2020

tekton-robot commented Jun 15, 2020

dibyom commented Jun 16, 2020

dibyom commented Jun 16, 2020

tekton-robot commented Jun 18, 2020

tekton-robot commented Jun 18, 2020

tekton-robot commented Jun 18, 2020

GwonsooLee commented Jun 18, 2020

GwonsooLee commented Jun 18, 2020

dibyom commented Jun 18, 2020

dibyom commented Jun 18, 2020

dibyom commented Jun 18, 2020

dibyom commented Jun 18, 2020

dibyom commented Jun 18, 2020

GwonsooLee commented Jun 19, 2020

dibyom commented Jun 19, 2020

GwonsooLee commented Jun 19, 2020

GwonsooLee commented Jun 5, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 5, 2020 •

edited

Loading