From fbfbfeafb100b581e53f6f642d655c337073e627 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Wed, 24 Jul 2019 14:19:18 +0200 Subject: [PATCH] =?UTF-8?q?Migrate/Upgrade=20previous=20version=20with=20a?= =?UTF-8?q?=20default=20timeout=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In case of a controller upgrade, we may still store previous versions of resource that may not have been the default set. This migrates those "in memory" to make sure we do not panic in those cases. This also gives a great example of how to use a mutating webhook to migrate some resources from one version to another. Signed-off-by: Vincent Demeester --- pkg/apis/pipeline/v1alpha1/contexts.go | 24 ++++++ .../pipeline/v1alpha1/pipelinerun_defaults.go | 11 ++- .../v1alpha1/pipelinerun_defaults_test.go | 78 +++++++++++++++++ .../pipeline/v1alpha1/taskrun_defaults.go | 14 ++- .../v1alpha1/taskrun_defaults_test.go | 85 +++++++++++++++++++ .../v1alpha1/pipelinerun/pipelinerun.go | 4 + pkg/reconciler/v1alpha1/taskrun/taskrun.go | 4 + 7 files changed, 217 insertions(+), 3 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/contexts.go b/pkg/apis/pipeline/v1alpha1/contexts.go index 7edc042d5de..1b7daa482b3 100644 --- a/pkg/apis/pipeline/v1alpha1/contexts.go +++ b/pkg/apis/pipeline/v1alpha1/contexts.go @@ -28,3 +28,27 @@ type hdcnKey struct{} func WithDefaultConfigurationName(ctx context.Context) context.Context { return context.WithValue(ctx, hdcnKey{}, struct{}{}) } + +// HasDefaultConfigurationName checks to see whether the given context has +// been marked as having a default configurationName. +func HasDefaultConfigurationName(ctx context.Context) bool { + return ctx.Value(hdcnKey{}) != nil +} + +// lemonadeKey is used as the key for associating information +// with a context.Context. This variable doesn't really matter, so it's +// a total random name (for history purpose, used lemonade as it was written +// in an hot summer day). +type lemonadeKey struct{} + +// WithUpgradeViaDefaulting notes on the context that we want defaulting to rewrite +// from v1alpha1 pre-defaults to v1alpha1 post-defaults. +func WithUpgradeViaDefaulting(ctx context.Context) context.Context { + return context.WithValue(ctx, lemonadeKey{}, struct{}{}) +} + +// IsUpgradeViaDefaulting checks whether we should be "defaulting" from v1alpha1 pre-defaults to +// the v1alpha1 post-defaults subset. +func IsUpgradeViaDefaulting(ctx context.Context) bool { + return ctx.Value(lemonadeKey{}) != nil +} diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go index b3b05ebd975..b24760aebbf 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go @@ -31,6 +31,15 @@ func (pr *PipelineRun) SetDefaults(ctx context.Context) { func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) if prs.Timeout == nil { - prs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute} + var timeout *metav1.Duration + if IsUpgradeViaDefaulting(ctx) { + // This case is for preexisting `TaskRun` before 0.5.0, so let's + // add the old default timeout. + // Most likely those TaskRun passing here are already done and/or already running + timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute} + } else { + timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute} + } + prs.Timeout = timeout } } diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go index d8337125deb..38545291206 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go @@ -22,9 +22,16 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + logtesting "knative.dev/pkg/logging/testing" +) + +var ( + ignoreUnexportedResources = cmpopts.IgnoreUnexported() ) func TestPipelineRunSpec_SetDefaults(t *testing.T) { @@ -60,4 +67,75 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) { } }) } + +} + +func TestPipelineRunDefaulting(t *testing.T) { + tests := []struct { + name string + in *v1alpha1.PipelineRun + want *v1alpha1.PipelineRun + wc func(context.Context) context.Context + }{{ + name: "empty no context", + in: &v1alpha1.PipelineRun{}, + want: &v1alpha1.PipelineRun{ + Spec: v1alpha1.PipelineRunSpec{ + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, + }, { + name: "PipelineRef upgrade context", + in: &v1alpha1.PipelineRun{ + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: v1alpha1.PipelineRef{Name: "foo"}, + }, + }, + want: &v1alpha1.PipelineRun{ + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: v1alpha1.PipelineRef{Name: "foo"}, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + }, { + name: "PipelineRef default config context", + in: &v1alpha1.PipelineRun{ + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: v1alpha1.PipelineRef{Name: "foo"}, + }, + }, + want: &v1alpha1.PipelineRun{ + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: v1alpha1.PipelineRef{Name: "foo"}, + Timeout: &metav1.Duration{Duration: 5 * time.Minute}, + }, + }, + wc: func(ctx context.Context) context.Context { + s := config.NewStore(logtesting.TestLogger(t)) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "default-timeout-minutes": "5", + }, + }) + return s.ToContext(ctx) + }, + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := tc.in + ctx := context.Background() + if tc.wc != nil { + ctx = tc.wc(ctx) + } + got.SetDefaults(ctx) + if !cmp.Equal(got, tc.want, ignoreUnexportedResources) { + t.Errorf("SetDefaults (-want, +got) = %v", + cmp.Diff(got, tc.want, ignoreUnexportedResources)) + } + }) + } } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go index e130966da78..ee92487efb1 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go @@ -29,11 +29,21 @@ func (tr *TaskRun) SetDefaults(ctx context.Context) { } func (trs *TaskRunSpec) SetDefaults(ctx context.Context) { + cfg := config.FromContextOrDefaults(ctx) if trs.TaskRef != nil && trs.TaskRef.Kind == "" { trs.TaskRef.Kind = NamespacedTaskKind } - cfg := config.FromContextOrDefaults(ctx) + if trs.Timeout == nil { - trs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute} + var timeout *metav1.Duration + if IsUpgradeViaDefaulting(ctx) { + // This case is for preexisting `TaskRun` before 0.5.0, so let's + // add the old default timeout. + // Most likely those TaskRun passing here are already done and/or already running + timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute} + } else { + timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute} + } + trs.Timeout = timeout } } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go index 6ef455ac78f..4f82997244a 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go @@ -24,7 +24,9 @@ import ( "github.com/google/go-cmp/cmp" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + logtesting "knative.dev/pkg/logging/testing" ) func TestTaskRunSpec_SetDefaults(t *testing.T) { @@ -77,3 +79,86 @@ func TestTaskRunSpec_SetDefaults(t *testing.T) { }) } } + +func TestTaskRunDefaulting(t *testing.T) { + tests := []struct { + name string + in *v1alpha1.TaskRun + want *v1alpha1.TaskRun + wc func(context.Context) context.Context + }{{ + name: "empty no context", + in: &v1alpha1.TaskRun{}, + want: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, + }, { + name: "TaskRef default to namespace kind", + in: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo"}, + }, + }, + want: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo", Kind: v1alpha1.NamespacedTaskKind}, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, + }, { + name: "TaskRef upgrade context", + in: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo"}, + }, + }, + want: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo", Kind: v1alpha1.NamespacedTaskKind}, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + }, { + name: "TaskRef default config context", + in: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo"}, + }, + }, + want: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{Name: "foo", Kind: v1alpha1.NamespacedTaskKind}, + Timeout: &metav1.Duration{Duration: 5 * time.Minute}, + }, + }, + wc: func(ctx context.Context) context.Context { + s := config.NewStore(logtesting.TestLogger(t)) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "default-timeout-minutes": "5", + }, + }) + return s.ToContext(ctx) + }, + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := tc.in + ctx := context.Background() + if tc.wc != nil { + ctx = tc.wc(ctx) + } + got.SetDefaults(ctx) + if !cmp.Equal(got, tc.want, ignoreUnexportedResources) { + t.Errorf("SetDefaults (-want, +got) = %v", + cmp.Diff(got, tc.want, ignoreUnexportedResources)) + } + }) + } +} diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index 37ee58dac84..cfa42cc10fb 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -195,6 +195,10 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) error { + // We may be reading a version of the object that was stored at an older version + // and may not have had all of the assumed default specified. + pr.SetDefaults(v1alpha1.WithUpgradeViaDefaulting(ctx)) + p, err := c.pipelineLister.Pipelines(pr.Namespace).Get(pr.Spec.PipelineRef.Name) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index 7cee5d53505..e43f09ee721 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -173,6 +173,10 @@ func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) (resources.GetTask, v1alp } func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error { + // We may be reading a version of the object that was stored at an older version + // and may not have had all of the assumed default specified. + tr.SetDefaults(v1alpha1.WithUpgradeViaDefaulting(ctx)) + // If the taskrun is cancelled, kill resources and update status if tr.IsCancelled() { before := tr.Status.GetCondition(apis.ConditionSucceeded)