add nodeSelector and replicas to eventlistener #625

GwonsooLee · 2020-06-19T16:45:39Z

Changes

Add nodeSelector and replicas feature to eventListener. With this, user could schedule eventListener pod to the node with specific label. Also, if needed, user could specify the number of replicas in yaml file. Previously I scaled pod by editing after deployment ( If other possible way to scale which I might not know..., please let me know!! 😄 )

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

- add replicas field for event listener to be able to run more than one pod.
- add nodeSelector field for event listener to schedule event listener pod to the specific node.

tekton-robot · 2020-06-19T16:45:54Z

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.

dibyom · 2020-06-22T16:08:19Z

/ok-to-test

tekton-robot · 2020-06-22T16:10:09Z

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	70.5%	70.8%	0.3
test/builder/eventlistener.go	83.1%	79.0%	-4.1

GwonsooLee · 2020-06-22T16:25:16Z

@dibyom Is the reason of failure related to the nodeSelector..? I added nodeSelector to test file with beta.kubernetes.io/arch": "amd64. If not then, I will figure out what's wrong more. Thanks in advance

dibyom · 2020-06-23T01:43:26Z

@dibyom Is the reason of failure related to the nodeSelector..? I added nodeSelector to test file with beta.kubernetes.io/arch": "amd64. If not then, I will figure out what's wrong more. Thanks in advance

Doesn't look like...looks like some test has a data race

khrm · 2020-06-23T11:00:19Z

pkg/apis/triggers/v1alpha1/event_listener_types.go

@@ -57,11 +57,20 @@ type EventListenerSpec struct {
 	ServiceAccountName string                 `json:"serviceAccountName"`
 	Triggers           []EventListenerTrigger `json:"triggers"`
 	ServiceType        corev1.ServiceType     `json:"serviceType,omitempty"`
+	Replicas           int32                  `json:"replicas"`


Should Replica be part of PodTemplate? Maybe in future have a ducktype like PodSpecable mentioned here:
#505 (comment)
Of course, we want to give only limited set of capability even if we do ducktype.
@dibyom

So, my understanding is that for PodTemplate or PodSpecable, the object can contain fields from a PodSpec: https://godoc.org/k8s.io/api/core/v1#PodSpec

replicas are part of the DeploymentSpec -> looking through the fields it doesn't seems like we'd want to support customizing any fields besides replicas. So, maybe we just leave replicas as part of the EL spec (maybe we can call it deploymentReplicas ?) What do you think @khrm ?

I'll add this to tomorrow's WG agenda. Unless there are objections, I feel inclined to keep the current field naming!

Thanks :) @dibyom

GwonsooLee · 2020-06-24T00:15:23Z

/test pull-tekton-triggers-integration-tests

tekton-robot · 2020-06-24T06:23: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	70.5%	70.8%	0.3
test/builder/eventlistener.go	83.1%	79.0%	-4.1

khrm

I think syncing with Master would resolve that test failure. We solve that Data Race here: #631

GwonsooLee · 2020-06-24T16:42:22Z

I think syncing with Master would resolve that test failure. We solve that Data Race here: #631

I rebase the master branch so that the error message was changed. I think there is another problem... I will try to figure it out.. Thank you so much @khrm :)

GwonsooLee · 2020-06-24T16:44:23Z

/test pull-tekton-triggers-integration-tests

dibyom · 2020-06-26T15:18:49Z

/test pull-tekton-triggers-integration-tests

dibyom · 2020-06-26T15:19:23Z

/kind feature

dibyom · 2020-06-26T15:20:00Z

third_party/github.com/hashicorp/golang-lru/simplelru/lru_interface.go

-  // Resizes cache, returning number evicted
-  Resize(int) int
+	// Resizes cache, returning number evicted
+	Resize(int) int


I think this is because I ran one of the scripts in ./hack directory...

dibyom · 2020-06-26T15:33:13Z

Thanks for adding this @GwonsooLee
Looks like the integration test failure is:

The EventListener "listener-podTemplate" is invalid: metadata.name: Invalid value: "listener-podTemplate": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

dibyom

Could you add a Release Notes section to this PR description ? Very helpful when we are doing a release!
for an example, see: #591

dibyom · 2020-06-26T15:33:42Z

examples/eventlisteners/eventlistener-podTemplate.yaml

+metadata:
+  name: listener-podTemplate
+spec:
+  serviceAccountName: tekton-triggers-example-sa


Thanks for adding examples, can you also update the docs to mention the new fields?

I added replicas and podTemplate.nodeSelector to docs. :)

tekton-robot · 2020-06-26T15:59:50Z

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	70.5%	70.8%	0.3
test/builder/eventlistener.go	83.1%	79.0%	-4.1

GwonsooLee · 2020-06-26T16:09:23Z

Thanks for adding this @GwonsooLee
Looks like the integration test failure is:

The EventListener "listener-podTemplate" is invalid: metadata.name: Invalid value: "listener-podTemplate": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

Thanks for finding the error! I fixed it :)
Is there any other way to run integration test locally??

GwonsooLee · 2020-06-26T16:11:03Z

Could you add a Release Notes section to this PR description ? Very helpful when we are doing a release!
for an example, see: #591

Oh.. I am sorry that I missed this one till now.. I will update this section from now on whenever I open PR. Thanks @dibyom

dibyom · 2020-06-26T16:23:11Z

Is there any other way to run integration test locally??

In general, for go e2e tests, you can run something like go test -count=1 -v -tags=e2e ./...

In addition, we have some tests, that apply our YAML examples to make sure they are valid: https://github.com/tektoncd/triggers/blob/master/test/e2e-tests-yaml.sh

I think you should be able to run the e2e-tests-yaml.sh script locally as well too!

dibyom

/cc @savitaashture @vdemeester @khrm

tekton-robot · 2020-07-09T15:46:49Z

[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

vdemeester

Looks good to me :)

dibyom · 2020-07-10T16:52:48Z

/lgtm

tekton-robot requested review from dlorenc and ncskier June 19, 2020 16:45

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 19, 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 22, 2020

dibyom mentioned this pull request Jun 23, 2020

E2e test has a data race #629

Closed

khrm reviewed Jun 23, 2020

View reviewed changes

GwonsooLee force-pushed the nodeSelector_and_replicas branch from d9177ca to e129b28 Compare June 24, 2020 06:21

khrm reviewed Jun 24, 2020

View reviewed changes

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 26, 2020

dibyom reviewed Jun 26, 2020

View reviewed changes

add nodeSelector and replicas to eventlistener

205376f

GwonsooLee force-pushed the nodeSelector_and_replicas branch from e129b28 to 205376f Compare June 26, 2020 15:57

dibyom approved these changes Jul 9, 2020

View reviewed changes

tekton-robot requested review from khrm, savitaashture and vdemeester July 9, 2020 15:46

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

vdemeester reviewed Jul 10, 2020

View reviewed changes

tekton-robot assigned dibyom Jul 10, 2020

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

tekton-robot merged commit 0bd7671 into tektoncd:master Jul 10, 2020

GwonsooLee deleted the nodeSelector_and_replicas branch July 10, 2020 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add nodeSelector and replicas to eventlistener #625

add nodeSelector and replicas to eventlistener #625

GwonsooLee commented Jun 19, 2020 •

edited

Loading

tekton-robot commented Jun 19, 2020

dibyom commented Jun 22, 2020

tekton-robot commented Jun 22, 2020

GwonsooLee commented Jun 22, 2020

dibyom commented Jun 23, 2020

khrm Jun 23, 2020

dibyom Jun 26, 2020

dibyom Jul 6, 2020

GwonsooLee Jul 7, 2020

GwonsooLee commented Jun 24, 2020

tekton-robot commented Jun 24, 2020

khrm left a comment

GwonsooLee commented Jun 24, 2020

GwonsooLee commented Jun 24, 2020

dibyom commented Jun 26, 2020

dibyom commented Jun 26, 2020

dibyom Jun 26, 2020

GwonsooLee Jun 26, 2020

dibyom commented Jun 26, 2020

dibyom left a comment

dibyom Jun 26, 2020

GwonsooLee Jun 26, 2020

tekton-robot commented Jun 26, 2020

GwonsooLee commented Jun 26, 2020

GwonsooLee commented Jun 26, 2020

dibyom commented Jun 26, 2020

dibyom left a comment

tekton-robot commented Jul 9, 2020

vdemeester left a comment

dibyom commented Jul 10, 2020

add nodeSelector and replicas to eventlistener #625

add nodeSelector and replicas to eventlistener #625

Conversation

GwonsooLee commented Jun 19, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 19, 2020

dibyom commented Jun 22, 2020

tekton-robot commented Jun 22, 2020

GwonsooLee commented Jun 22, 2020

dibyom commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GwonsooLee commented Jun 24, 2020

tekton-robot commented Jun 24, 2020

khrm left a comment

Choose a reason for hiding this comment

GwonsooLee commented Jun 24, 2020

GwonsooLee commented Jun 24, 2020

dibyom commented Jun 26, 2020

dibyom commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Jun 26, 2020

dibyom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 26, 2020

GwonsooLee commented Jun 26, 2020

GwonsooLee commented Jun 26, 2020

dibyom commented Jun 26, 2020

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 9, 2020

vdemeester left a comment

Choose a reason for hiding this comment

dibyom commented Jul 10, 2020

GwonsooLee commented Jun 19, 2020 •

edited

Loading