From c6e6ea2c0e5c44791c732d26c24d3f265979b382 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Tue, 21 Jan 2020 19:25:12 -0500 Subject: [PATCH 1/2] fix sink crashloop when upgrading from 0.1 to 0.2 The crashloop is due to 2 reasons: 1. We deprecated and removed the `params` field from the EventListener. So, when the reconciler tries to unmarshal an old EventListener, it fails since it cannot parse the deleted `params` field. 2. The new event listener sink image expects `/etc/config-logging` to be mounted. However the reconciler code does not add the `volumeMount` for any existing sink deployments. So, the new pods go into a crashloop To fix this, this commit does the following: 1. Add back the `params` field. Also, update the mutating webhook to delete any set `params` field in existing eventlisteners. 2. Add volumeMount for existing deployments if not present. - Co-Authored-by: Brandon Walker Fixes #366 Signed-off-by: Dibyo Mukherjee --- .../v1alpha1/event_listener_defaults.go | 7 ++++++ .../v1alpha1/event_listener_defaults_test.go | 25 +++++++++++++++++-- .../triggers/v1alpha1/event_listener_types.go | 4 +++ .../v1alpha1/zz_generated.deepcopy.go | 7 ++++++ .../v1alpha1/eventlistener/eventlistener.go | 8 ++++++ .../eventlistener/eventlistener_test.go | 19 ++++++++++++++ 6 files changed, 68 insertions(+), 2 deletions(-) diff --git a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go index 2e12b5e1d..379bad8b3 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go @@ -12,6 +12,7 @@ func (el *EventListener) SetDefaults(ctx context.Context) { t := &el.Spec.Triggers[i] upgradeBinding(t) upgradeInterceptor(t) + removeParams(t) } } } @@ -41,3 +42,9 @@ func upgradeInterceptor(t *EventListenerTrigger) { t.DeprecatedInterceptor = nil } } + +func removeParams(t *EventListenerTrigger) { + if t.DeprecatedParams != nil { + t.DeprecatedParams = nil + } +} diff --git a/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go b/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go index 8894d71e8..988bf54d2 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" ) @@ -141,8 +142,28 @@ func TestEventListenerSetDefaults(t *testing.T) { }, }, wc: v1alpha1.WithUpgradeViaDefaulting, - }, - } + }, { + name: "with upgrade context - deprecated params", + in: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Triggers: []v1alpha1.EventListenerTrigger{{ + DeprecatedParams: []pipelinev1.Param{{ + Name: "param-name", + Value: pipelinev1.ArrayOrString{ + Type: "string", + StringVal: "static", + }, + }, + }}, + }}, + }, + want: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Triggers: []v1alpha1.EventListenerTrigger{{}}, + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { got := tc.in diff --git a/pkg/apis/triggers/v1alpha1/event_listener_types.go b/pkg/apis/triggers/v1alpha1/event_listener_types.go index cc7856496..63f1c562d 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_types.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_types.go @@ -74,6 +74,10 @@ type EventListenerTrigger struct { // TODO(#248): Remove this before 0.3 release. DeprecatedBinding *EventListenerBinding `json:"binding,omitempty"` + + // TODO(#): Remove this before 0.3 release + // DEPRECATED: Use TriggerBindings with static values instead + DeprecatedParams []pipelinev1.Param `json:"params,omitempty"` } // EventInterceptor provides a hook to intercept and pre-process events diff --git a/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go index afb8856e0..10508d389 100644 --- a/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go @@ -267,6 +267,13 @@ func (in *EventListenerTrigger) DeepCopyInto(out *EventListenerTrigger) { *out = new(EventListenerBinding) **out = **in } + if in.DeprecatedParams != nil { + in, out := &in.DeprecatedParams, &out.DeprecatedParams + *out = make([]pipelinev1alpha1.Param, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go index 08d3a4efa..045b5fed4 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go @@ -354,6 +354,14 @@ func (c *Reconciler) reconcileDeployment(el *v1alpha1.EventListener) error { existingDeployment.Spec.Template.Spec.Containers[0].Command = nil updated = true } + if len(existingDeployment.Spec.Template.Spec.Containers[0].VolumeMounts) == 0 { + existingDeployment.Spec.Template.Spec.Containers[0].VolumeMounts = container.VolumeMounts + updated = true + } + if !reflect.DeepEqual(existingDeployment.Spec.Template.Spec.Volumes, deployment.Spec.Template.Spec.Volumes) { + existingDeployment.Spec.Template.Spec.Volumes = deployment.Spec.Template.Spec.Volumes + updated = true + } } if updated { if _, err := c.KubeClientSet.AppsV1().Deployments(el.Namespace).Update(existingDeployment); err != nil { diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go index 890bd27be..68ac4f315 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go @@ -260,6 +260,7 @@ func Test_reconcileDeployment(t *testing.T) { eventListener4.Spec.ServiceAccountName = updatedSa var replicas int32 = 1 + // deployment1 == initial deployment deployment1 := &appsv1.Deployment{ ObjectMeta: generateObjectMeta(eventListener0), Spec: appsv1.DeploymentSpec{ @@ -329,11 +330,13 @@ func Test_reconcileDeployment(t *testing.T) { }, } + // deployment 2 == initial deployment + labels from eventListener deployment2 := deployment1.DeepCopy() deployment2.Labels = mergeLabels(generatedLabels, updateLabel) deployment2.Spec.Selector.MatchLabels = mergeLabels(generatedLabels, updateLabel) deployment2.Spec.Template.Labels = mergeLabels(generatedLabels, updateLabel) + // deployment 3 == initial deployment + updated replicas deployment3 := deployment1.DeepCopy() var updateReplicas int32 = 5 deployment3.Spec.Replicas = &updateReplicas @@ -341,6 +344,10 @@ func Test_reconcileDeployment(t *testing.T) { deployment4 := deployment1.DeepCopy() deployment4.Spec.Template.Spec.ServiceAccountName = updatedSa + deploymentMissingVolumes := deployment1.DeepCopy() + deploymentMissingVolumes.Spec.Template.Spec.Volumes = nil + deploymentMissingVolumes.Spec.Template.Spec.Containers[0].VolumeMounts = nil + tests := []struct { name string startResources test.Resources @@ -422,6 +429,18 @@ func Test_reconcileDeployment(t *testing.T) { EventListeners: []*v1alpha1.EventListener{eventListener4}, Deployments: []*appsv1.Deployment{deployment4}, }, + }, { + name: "eventlistener-config-volume-mount-update", + startResources: test.Resources{ + Namespaces: []*corev1.Namespace{namespaceResource}, + EventListeners: []*v1alpha1.EventListener{eventListener2}, + Deployments: []*appsv1.Deployment{deploymentMissingVolumes}, + }, + endResources: test.Resources{ + Namespaces: []*corev1.Namespace{namespaceResource}, + EventListeners: []*v1alpha1.EventListener{eventListener2}, + Deployments: []*appsv1.Deployment{deployment2}, + }, }, } for i := range tests { From 5f43915cbe1422c055d7f7a20c53c5ee63cad91b Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Thu, 23 Jan 2020 14:21:44 -0500 Subject: [PATCH 2/2] Set upgrade context on webhook This ensures that the automatic upgrades that we do in `SetDefaults` (e.g. `binding` to `bindings`) are actually persisted in etcd. Previously we only did these upgrades on each reconcile but did not persist them. Signed-off-by: Dibyo Mukherjee --- cmd/webhook/main.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 9d4c6a2b4..6e7253366 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -17,7 +17,6 @@ limitations under the License. package main import ( - "context" "log" "github.com/tektoncd/pipeline/pkg/system" @@ -81,9 +80,7 @@ func main() { } // Decorate contexts with the current state of the config. - ctxFunc := func(ctx context.Context) context.Context { - return ctx - } + ctxFunc := v1alpha1.WithUpgradeViaDefaulting controller, err := webhook.New(kubeClient, options, admissionControllers, logger, ctxFunc) if err != nil {