Fix update deployment when there is a change in replicas #715

savitaashture · 2020-08-17T15:40:00Z

Changes

The changes fix update deployment operation when there is a change in replicas number.
With current code whenever user updates replicas as part of eventlistener update success but no change in behavior.

Fix: #714

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

None

tekton-robot · 2020-08-17T15:42:46Z

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.8%	72.5%	1.8

vdemeester

/lgtm

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

dibyom · 2020-08-18T01:14:22Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go

@@ -451,20 +454,20 @@ func Test_reconcileDeployment(t *testing.T) {
 			name: "deployment-replica-update",
 			startResources: test.Resources{
 				Namespaces:     []*corev1.Namespace{namespaceResource},
-				EventListeners: []*v1alpha1.EventListener{eventListener1},
-				Deployments:    []*appsv1.Deployment{deployment3},
+				EventListeners: []*v1alpha1.EventListener{eventListener3},


One issue here is that before we had spec.replicas in the EL itself, users had to manually update the replica on the deployment itself. And we had code and this test to verify that we do not overwrite any changes to the number of replicas on an EL. So, this is a change in our behavior. I am thinking we can either:

Honor the old replica changes i.e. if someone changes the existing replica numbers in the deployment itself and spec.Replicas is not set on the EL, we should not set it back. This keeps the behavior backwards compatible

Or we deprecate the existing behavior (not sure how many people depend on this behavior) and only honor EL's spec.replicas -- we just call this out in the release notes.

One thing I'm curious about (and don't know) is how this will work with Horizontal Pod Autoscalers. Do they change the deployment's spec.Replicas? In that case, if an HPA scales the deployment we do not want to inadvertently scale it back down.

Can we have the behavior where if el.spec.Replicas is set, then we track changes only in el.spec.Replicas and don't care what user set in deployment.
If el.spec.Replicas isn't set, then we don't change deployment replica and let k8s controller take care of it?
HPA uses deployment's number as you surmised. So design 2 will break them.

Ya this is something need to verify with HPA
Thanks for pointing it out i will try and update here

One thing I'm curious about (and don't know) is how this will work with Horizontal Pod Autoscalers. Do they change the deployment's spec.Replicas? In that case, if an HPA scales the deployment we do not want to inadvertently scale it back down.

@dibyom Yes HPA uses deployment so with this PR change if HPA increases replicas on deployment whenever condition met But that will be again overridden by el.Spec.Replicas always

How about below opinion on supporting replicas

Lets keep spec.replicas which can be set by the user and we will add a note in README that if user sets spec.replicas as part of eventlistener yaml then it will override replicas values if user manually set by kubectl edit deployment or by setting HPA
Which means if spec.replicas set by event listener yaml then it won't respects replicas values edited by user manually or through any other mechanism (ex: HPA)

If user Don't set spec.replicas in eventlistener yaml then behavior will be same like earlier how it use to work before adding replicas field to eventlistener yaml

Few points from WG discussion

Don't allow spec.replicas as part of eventlistener yaml instead hardcode replicas to some number instead of 1
-> With this approach if we hardcode replicas to say 3 then if HPA configured with --min=2 --max=5 then again replicas number varies

Some prior art / similar behavior for k8s addons - https://github.com/kubernetes/kubernetes/tree/master/cluster/addons/addon-manager
-> This is bit tricky to implement

/cc @dibyom @khrm @wlynch

Thanks for summarizing the discussion. It sounds good to me!

tekton-robot · 2020-08-24T06:32:56Z

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	72.5%	73.7%	1.2

tekton-robot · 2020-08-24T06:35:30Z

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	72.5%	73.7%	1.2

savitaashture · 2020-08-24T06:38:37Z

/hold

tekton-robot · 2020-08-24T06:48:37Z

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	72.5%	73.7%	1.2

dibyom · 2020-08-25T15:24:51Z

/approve

dibyom

/lgtm

tekton-robot · 2020-08-25T15:25:25Z

[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

savitaashture · 2020-08-25T19:01:10Z

/hold cancel

tekton-robot · 2020-08-25T19:02: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	72.5%	73.7%	1.2

savitaashture · 2020-08-25T19:02:58Z

docs/eventlisteners.md

@@ -166,6 +166,8 @@ check out the guide on [exposing EventListeners](./exposing-eventlisteners.md).
 The `replicas` field is optional. By default, the number of replicas of EventListener is 1.
 If you want to deploy more than one pod, you can specify the number to this field. 

+**Note:** If user sets `replicas` field while creating eventlistener yaml then it won't respects replicas values edited by user manually or through any other mechanism (ex: HPA).
+


@dibyom Added a note as per the discussion here #715 (comment)

dibyom · 2020-08-26T13:59:22Z

/lgtm

tekton-robot requested review from bobcatfish and khrm August 17, 2020 15:40

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 17, 2020

vdemeester reviewed Aug 17, 2020

View reviewed changes

tekton-robot assigned vdemeester Aug 17, 2020

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

khrm reviewed Aug 18, 2020

View reviewed changes

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go Show resolved Hide resolved

dibyom reviewed Aug 18, 2020

View reviewed changes

tekton-robot requested review from dibyom, khrm and wlynch August 18, 2020 18:19

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2020

savitaashture force-pushed the replica_update branch from 1353fed to e1e3588 Compare August 24, 2020 06:30

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

savitaashture force-pushed the replica_update branch from e1e3588 to 2f483a9 Compare August 24, 2020 06:33

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2020

savitaashture force-pushed the replica_update branch from 2f483a9 to 42372d5 Compare August 24, 2020 06:46

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2020

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2020

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

dibyom approved these changes Aug 25, 2020

View reviewed changes

tekton-robot assigned dibyom Aug 25, 2020

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

Fix update deployment when there is a change in replicas

4f94a64

savitaashture force-pushed the replica_update branch from 42372d5 to 4f94a64 Compare August 25, 2020 19:00

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2020

savitaashture commented Aug 25, 2020

View reviewed changes

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

tekton-robot merged commit efdd904 into tektoncd:master Aug 26, 2020

savitaashture mentioned this pull request Sep 10, 2020

Handle validation, defaults and working behavior for Replicas #751

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix update deployment when there is a change in replicas #715

Fix update deployment when there is a change in replicas #715

savitaashture commented Aug 17, 2020 •

edited

tekton-robot commented Aug 17, 2020

vdemeester left a comment

dibyom Aug 18, 2020

khrm Aug 18, 2020

savitaashture Aug 18, 2020

savitaashture Aug 18, 2020 •

edited

savitaashture Aug 18, 2020

dibyom Aug 19, 2020

tekton-robot commented Aug 24, 2020

tekton-robot commented Aug 24, 2020

savitaashture commented Aug 24, 2020

tekton-robot commented Aug 24, 2020

dibyom commented Aug 25, 2020

dibyom left a comment

tekton-robot commented Aug 25, 2020

savitaashture commented Aug 25, 2020

tekton-robot commented Aug 25, 2020

savitaashture Aug 25, 2020

dibyom Aug 26, 2020

dibyom commented Aug 26, 2020

Fix update deployment when there is a change in replicas #715

Fix update deployment when there is a change in replicas #715

Conversation

savitaashture commented Aug 17, 2020 • edited

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 17, 2020

vdemeester left a comment

Choose a reason for hiding this comment

dibyom Aug 18, 2020

Choose a reason for hiding this comment

khrm Aug 18, 2020

Choose a reason for hiding this comment

savitaashture Aug 18, 2020

Choose a reason for hiding this comment

savitaashture Aug 18, 2020 • edited

Choose a reason for hiding this comment

savitaashture Aug 18, 2020

Choose a reason for hiding this comment

dibyom Aug 19, 2020

Choose a reason for hiding this comment

tekton-robot commented Aug 24, 2020

tekton-robot commented Aug 24, 2020

savitaashture commented Aug 24, 2020

tekton-robot commented Aug 24, 2020

dibyom commented Aug 25, 2020

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 25, 2020

savitaashture commented Aug 25, 2020

tekton-robot commented Aug 25, 2020

savitaashture Aug 25, 2020

Choose a reason for hiding this comment

dibyom Aug 26, 2020

Choose a reason for hiding this comment

dibyom commented Aug 26, 2020

savitaashture commented Aug 17, 2020 •

edited

savitaashture Aug 18, 2020 •

edited