From 196a945c2dd795b9ef5e516f365e69c69218754e Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 8 Jul 2019 16:07:44 +0200 Subject: [PATCH] Refactoring "config-defaults" to use `Defaultable` (for timeout) Each type of tektoncd/pipeline implements `Defaultable` (from `knative/pkg`), which is meant to be used to set defaults value. This changes uses this mechanisms to set the default timeout for `TaskRun`s, instead of putting this mechanisms in the `timeout_handler` code. Signed-off-by: Vincent Demeester --- cmd/controller/main.go | 7 +- cmd/webhook/main.go | 22 ++++-- pkg/apis/config/default.go | 18 ++--- pkg/apis/config/default_test.go | 19 ++--- pkg/apis/config/store.go | 11 +-- pkg/apis/config/store_test.go | 7 +- pkg/apis/config/zz_generated.deepcopy.go | 8 +-- pkg/apis/pipeline/v1alpha1/contexts.go | 30 ++++++++ .../pipeline/v1alpha1/pipelinerun_defaults.go | 36 ++++++++++ .../pipeline/v1alpha1/pipelinerun_types.go | 8 +-- .../pipeline/v1alpha1/taskrun_defaults.go | 12 +++- pkg/reconciler/timeout_handler.go | 24 +------ pkg/reconciler/timeout_handler_test.go | 69 +++++-------------- .../v1alpha1/pipelinerun/pipelinerun_test.go | 18 +---- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 10 +-- .../v1alpha1/taskrun/taskrun_test.go | 15 +--- test/builder/pipeline.go | 7 +- test/builder/pipeline_test.go | 2 +- test/builder/task.go | 3 + test/builder/task_test.go | 2 + test/timeout_test.go | 2 +- 21 files changed, 166 insertions(+), 164 deletions(-) create mode 100644 pkg/apis/pipeline/v1alpha1/contexts.go create mode 100644 pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index eb5b60f4c75..8f75fb1497c 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -23,7 +23,6 @@ import ( "github.com/knative/pkg/logging" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" tklogging "github.com/tektoncd/pipeline/pkg/logging" corev1 "k8s.io/api/core/v1" kubeinformers "k8s.io/client-go/informers" @@ -118,10 +117,7 @@ func main() { pipelineInformer := pipelineInformerFactory.Tekton().V1alpha1().Pipelines() pipelineRunInformer := pipelineInformerFactory.Tekton().V1alpha1().PipelineRuns() - store := apisconfig.NewStore(logger.Named("config-store")) - store.WatchConfigs(configMapWatcher) - - timeoutHandler := reconciler.NewTimeoutHandler(stopCh, logger, store) + timeoutHandler := reconciler.NewTimeoutHandler(stopCh, logger) trc := taskrun.NewController(opt, taskRunInformer, @@ -131,7 +127,6 @@ func main() { podInformer, nil, //entrypoint cache will be initialized by controller if not provided timeoutHandler, - store, ) prc := pipelinerun.NewController(opt, pipelineRunInformer, diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 0f6bf610d3b..2e1620cd200 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -17,21 +17,20 @@ limitations under the License. package main import ( + "context" "flag" "log" - "github.com/knative/pkg/logging" - tklogging "github.com/tektoncd/pipeline/pkg/logging" - - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" - - "go.uber.org/zap" - "github.com/knative/pkg/configmap" + "github.com/knative/pkg/logging" "github.com/knative/pkg/logging/logkey" "github.com/knative/pkg/signals" "github.com/knative/pkg/webhook" + apiconfig "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + tklogging "github.com/tektoncd/pipeline/pkg/logging" "github.com/tektoncd/pipeline/pkg/system" + "go.uber.org/zap" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -71,6 +70,10 @@ func main() { // Watch the logging config map and dynamically update logging levels. configMapWatcher := configmap.NewInformedWatcher(kubeClient, system.GetNamespace()) configMapWatcher.Watch(tklogging.ConfigName, logging.UpdateLevelFromConfigMap(logger, atomicLevel, WebhookLogKey)) + + store := apiconfig.NewStore(logger.Named("config-store")) + store.WatchConfigs(configMapWatcher) + if err = configMapWatcher.Start(stopCh); err != nil { logger.Fatalf("failed to start configuration manager: %v", err) } @@ -95,6 +98,11 @@ func main() { v1alpha1.SchemeGroupVersion.WithKind("PipelineRun"): &v1alpha1.PipelineRun{}, }, Logger: logger, + + // Decorate contexts with the current state of the config. + WithContext: func(ctx context.Context) context.Context { + return v1alpha1.WithDefaultConfigurationName(store.ToContext(ctx)) + }, } if err := controller.Run(stopCh); err != nil { diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index f31ebff3a3e..ba591d4fee7 100644 --- a/pkg/apis/config/default.go +++ b/pkg/apis/config/default.go @@ -30,20 +30,20 @@ const ( defaultTimeoutMinutesKey = "default-timeout-minutes" ) -// ConfigDefault holds the default configurations +// Defaults holds the default configurations // +k8s:deepcopy-gen=true -type ConfigDefault struct { +type Defaults struct { DefaultTimeoutMinutes int } // Equals returns true if two Configs are identical -func (cfg *ConfigDefault) Equals(other *ConfigDefault) bool { +func (cfg *Defaults) Equals(other *Defaults) bool { return other.DefaultTimeoutMinutes == cfg.DefaultTimeoutMinutes } -// NewConfigDefaultFromMap returns a Config given a map corresponding to a ConfigMap -func NewConfigDefaultFromMap(cfgMap map[string]string) (*ConfigDefault, error) { - tc := ConfigDefault{ +// NewDefaultsFromMap returns a Config given a map corresponding to a ConfigMap +func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) { + tc := Defaults{ DefaultTimeoutMinutes: DefaultTimeoutMinutes, } if defaultTimeoutMin, ok := cfgMap[defaultTimeoutMinutesKey]; ok { @@ -57,7 +57,7 @@ func NewConfigDefaultFromMap(cfgMap map[string]string) (*ConfigDefault, error) { return &tc, nil } -// NewConfigDefaultFromConfigMap returns a Config for the given configmap -func NewConfigDefaultFromConfigMap(config *corev1.ConfigMap) (*ConfigDefault, error) { - return NewConfigDefaultFromMap(config.Data) +// NewDefaultsFromConfigMap returns a Config for the given configmap +func NewDefaultsFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) { + return NewDefaultsFromMap(config.Data) } diff --git a/pkg/apis/config/default_test.go b/pkg/apis/config/default_test.go index a66e4c1b8d2..f67d89f7617 100644 --- a/pkg/apis/config/default_test.go +++ b/pkg/apis/config/default_test.go @@ -17,33 +17,34 @@ limitations under the License. package config import ( + "testing" + "github.com/google/go-cmp/cmp" test "github.com/tektoncd/pipeline/pkg/reconciler/testing" - "testing" ) -func TestNewConfigDefaultFromConfigMap(t *testing.T) { - expectedConfig := &ConfigDefault{ +func TestNewDefaultsFromConfigMap(t *testing.T) { + expectedConfig := &Defaults{ DefaultTimeoutMinutes: 50, } verifyConfigFileWithExpectedConfig(t, DefaultsConfigName, expectedConfig) } -func TestNewConfigDefaultFromEmptyConfigMap(t *testing.T) { +func TestNewDefaultsFromEmptyConfigMap(t *testing.T) { DefaultsConfigEmptyName := "config-defaults-empty" - expectedConfig := &ConfigDefault{ + expectedConfig := &Defaults{ DefaultTimeoutMinutes: 60, } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig) } -func verifyConfigFileWithExpectedConfig(t *testing.T, fileName string, expectedConfig *ConfigDefault) { +func verifyConfigFileWithExpectedConfig(t *testing.T, fileName string, expectedConfig *Defaults) { cm := test.ConfigMapFromTestFile(t, fileName) - if configDefault, err := NewConfigDefaultFromConfigMap(cm); err == nil { - if d := cmp.Diff(configDefault, expectedConfig); d != "" { + if Defaults, err := NewDefaultsFromConfigMap(cm); err == nil { + if d := cmp.Diff(Defaults, expectedConfig); d != "" { t.Errorf("Diff:\n%s", d) } } else { - t.Errorf("NewConfigDefaultFromConfigMap(actual) = %v", err) + t.Errorf("NewDefaultsFromConfigMap(actual) = %v", err) } } diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index 0144c5c8285..d0e0e0c7e82 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -18,6 +18,7 @@ package config import ( "context" + "github.com/knative/pkg/configmap" ) @@ -26,7 +27,7 @@ type cfgKey struct{} // Config holds the collection of configurations that we attach to contexts. // +k8s:deepcopy-gen=false type Config struct { - ConfigDefault *ConfigDefault + Defaults *Defaults } // FromContext extracts a Config from the provided context. @@ -44,9 +45,9 @@ func FromContextOrDefaults(ctx context.Context) *Config { if cfg := FromContext(ctx); cfg != nil { return cfg } - defaults, _ := NewConfigDefaultFromMap(map[string]string{}) + defaults, _ := NewDefaultsFromMap(map[string]string{}) return &Config{ - ConfigDefault: defaults, + Defaults: defaults, } } @@ -69,7 +70,7 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i "defaults", logger, configmap.Constructors{ - DefaultsConfigName: NewConfigDefaultFromConfigMap, + DefaultsConfigName: NewDefaultsFromConfigMap, }, onAfterStore..., ), @@ -86,6 +87,6 @@ func (s *Store) ToContext(ctx context.Context) context.Context { // Load creates a Config from the current config state of the Store. func (s *Store) Load() *Config { return &Config{ - ConfigDefault: s.UntypedLoad(DefaultsConfigName).(*ConfigDefault).DeepCopy(), + Defaults: s.UntypedLoad(DefaultsConfigName).(*Defaults).DeepCopy(), } } diff --git a/pkg/apis/config/store_test.go b/pkg/apis/config/store_test.go index 6eb23809ed0..6ccb2357e97 100644 --- a/pkg/apis/config/store_test.go +++ b/pkg/apis/config/store_test.go @@ -18,10 +18,11 @@ package config import ( "context" + "testing" + "github.com/google/go-cmp/cmp" logtesting "github.com/knative/pkg/logging/testing" test "github.com/tektoncd/pipeline/pkg/reconciler/testing" - "testing" ) func TestStoreLoadWithContext(t *testing.T) { @@ -31,8 +32,8 @@ func TestStoreLoadWithContext(t *testing.T) { config := FromContext(store.ToContext(context.Background())) - expected, _ := NewConfigDefaultFromConfigMap(defaultConfig) - if diff := cmp.Diff(config.ConfigDefault, expected); diff != "" { + expected, _ := NewDefaultsFromConfigMap(defaultConfig) + if diff := cmp.Diff(config.Defaults, expected); diff != "" { t.Errorf("Unexpected default config (-want, +got): %v", diff) } } diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index ac915fdf832..b9be3f53a89 100644 --- a/pkg/apis/config/zz_generated.deepcopy.go +++ b/pkg/apis/config/zz_generated.deepcopy.go @@ -21,17 +21,17 @@ limitations under the License. package config // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ConfigDefault) DeepCopyInto(out *ConfigDefault) { +func (in *Defaults) DeepCopyInto(out *Defaults) { *out = *in return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ConfigDefault. -func (in *ConfigDefault) DeepCopy() *ConfigDefault { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. +func (in *Defaults) DeepCopy() *Defaults { if in == nil { return nil } - out := new(ConfigDefault) + out := new(Defaults) in.DeepCopyInto(out) return out } diff --git a/pkg/apis/pipeline/v1alpha1/contexts.go b/pkg/apis/pipeline/v1alpha1/contexts.go new file mode 100644 index 00000000000..7edc042d5de --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/contexts.go @@ -0,0 +1,30 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import "context" + +// hdcnKey is used as the key for associating information +// with a context.Context. +type hdcnKey struct{} + +// WithDefaultConfigurationName notes on the context for nested validation +// that there is a default configuration name, which affects how an empty +// configurationName is validated. +func WithDefaultConfigurationName(ctx context.Context) context.Context { + return context.WithValue(ctx, hdcnKey{}, struct{}{}) +} diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go new file mode 100644 index 00000000000..5f18fb20ce9 --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go @@ -0,0 +1,36 @@ +/* +Copyright 2019 The Tekton Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "context" + "time" + + "github.com/tektoncd/pipeline/pkg/apis/config" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func (pr *PipelineRun) SetDefaults(ctx context.Context) { + pr.Spec.SetDefaults(ctx) +} + +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} + } +} diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index 32a593fd8cb..d4b93726515 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -17,7 +17,6 @@ limitations under the License. package v1alpha1 import ( - "context" "fmt" "time" @@ -37,6 +36,10 @@ var ( } ) +// Check that TaskRun may be validated and defaulted. +var _ apis.Validatable = (*PipelineRun)(nil) +var _ apis.Defaultable = (*PipelineRun)(nil) + // PipelineRunSpec defines the desired state of PipelineRun type PipelineRunSpec struct { PipelineRef PipelineRef `json:"pipelineRef"` @@ -211,9 +214,6 @@ func (pr *PipelineRun) GetTaskRunRef() corev1.ObjectReference { } } -// SetDefaults for pipelinerun -func (pr *PipelineRun) SetDefaults(ctx context.Context) {} - // GetOwnerReference gets the pipeline run as owner reference for any related objects func (pr *PipelineRun) GetOwnerReference() []metav1.OwnerReference { return []metav1.OwnerReference{ diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go index 753859784ee..e64c06462da 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go @@ -16,7 +16,13 @@ limitations under the License. package v1alpha1 -import "context" +import ( + "context" + "time" + + "github.com/tektoncd/pipeline/pkg/apis/config" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) func (tr *TaskRun) SetDefaults(ctx context.Context) { tr.Spec.SetDefaults(ctx) @@ -26,4 +32,8 @@ func (trs *TaskRunSpec) SetDefaults(ctx context.Context) { 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} + } } diff --git a/pkg/reconciler/timeout_handler.go b/pkg/reconciler/timeout_handler.go index 6b9891281e6..240a219ea43 100644 --- a/pkg/reconciler/timeout_handler.go +++ b/pkg/reconciler/timeout_handler.go @@ -7,7 +7,6 @@ import ( "time" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" "go.uber.org/zap" @@ -52,21 +51,18 @@ type TimeoutSet struct { doneMut sync.Mutex backoffs map[string]backoff backoffsMut sync.Mutex - store *apisconfig.Store } // NewTimeoutHandler returns TimeoutSet filled structure func NewTimeoutHandler( stopCh <-chan struct{}, logger *zap.SugaredLogger, - store *apisconfig.Store, ) *TimeoutSet { return &TimeoutSet{ stopCh: stopCh, done: make(map[string]chan bool), backoffs: make(map[string]backoff), logger: logger, - store: store, } } @@ -110,17 +106,6 @@ func (t *TimeoutSet) getOrCreateFinishedChan(runObj StatusKey) chan bool { return finished } -// GetTimeout takes a kubernetes Duration representing the timeout period for a -// resource and returns it as a time.Duration. If the provided duration is nil -// then fallback behaviour is to return a default timeout period. -func GetTimeout(d *metav1.Duration, defaultTimeout int) time.Duration { - timeout := time.Duration(defaultTimeout) * time.Minute - if d != nil { - timeout = d.Duration - } - return timeout -} - // GetBackoff records the number of times it has seen a TaskRun and calculates an // appropriate backoff deadline based on that count. Only one backoff per TaskRun // may be active at any moment. Requests for a new backoff in the face of an @@ -142,8 +127,7 @@ func (t *TimeoutSet) GetBackoff(tr *v1alpha1.TaskRun) (backoff, bool) { } b.NumAttempts += 1 b.NextAttempt = time.Now().Add(backoffDuration(b.NumAttempts, rand.Intn)) - cfg := t.store.Load() - timeoutDeadline := tr.Status.StartTime.Time.Add(GetTimeout(tr.Spec.Timeout, cfg.ConfigDefault.DefaultTimeoutMinutes)) + timeoutDeadline := tr.Status.StartTime.Time.Add(tr.Spec.Timeout.Duration) if timeoutDeadline.Before(b.NextAttempt) { b.NextAttempt = timeoutDeadline } @@ -220,16 +204,14 @@ func (t *TimeoutSet) checkTaskRunTimeouts(namespace string, pipelineclientset cl // 1. Stop signal, 2. TaskRun to complete or 3. Taskrun to time out, which is // determined by checking if the tr's timeout has occurred since the startTime func (t *TimeoutSet) WaitTaskRun(tr *v1alpha1.TaskRun, startTime *metav1.Time) { - cfg := t.store.Load() - t.waitRun(tr, GetTimeout(tr.Spec.Timeout, cfg.ConfigDefault.DefaultTimeoutMinutes), startTime, t.taskRunCallbackFunc) + t.waitRun(tr, tr.Spec.Timeout.Duration, startTime, t.taskRunCallbackFunc) } // WaitPipelineRun function creates a blocking function for pipelinerun to wait for // 1. Stop signal, 2. pipelinerun to complete or 3. pipelinerun to time out which is // determined by checking if the tr's timeout has occurred since the startTime func (t *TimeoutSet) WaitPipelineRun(pr *v1alpha1.PipelineRun, startTime *metav1.Time) { - cfg := t.store.Load() - t.waitRun(pr, GetTimeout(pr.Spec.Timeout, cfg.ConfigDefault.DefaultTimeoutMinutes), startTime, t.pipelineRunCallbackFunc) + t.waitRun(pr, pr.Spec.Timeout.Duration, startTime, t.pipelineRunCallbackFunc) } func (t *TimeoutSet) waitRun(runObj StatusKey, timeout time.Duration, startTime *metav1.Time, callback func(interface{})) { diff --git a/pkg/reconciler/timeout_handler_test.go b/pkg/reconciler/timeout_handler_test.go index b8f1af0fa6a..7fa3ba03ea8 100644 --- a/pkg/reconciler/timeout_handler_test.go +++ b/pkg/reconciler/timeout_handler_test.go @@ -6,8 +6,7 @@ import ( "time" "github.com/knative/pkg/apis" - logtesting "github.com/knative/pkg/logging/testing" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/test" tb "github.com/tektoncd/pipeline/test/builder" @@ -25,21 +24,6 @@ var ( simpleTask = tb.Task("test-task", testNs, tb.TaskSpec(simpleStep)) ) -func getStore(t *testing.T) *apisconfig.Store { - store := apisconfig.NewStore(logtesting.TestLogger(t)) - defaultConfig := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: "config-defaults", - }, - Data: map[string]string{ - "default-timeout-minutes": "60", - }, - } - store.OnConfigChanged(defaultConfig) - return store -} - func TestTaskRunCheckTimeouts(t *testing.T) { taskRunTimedout := tb.TaskRun("test-taskrun-run-timedout", testNs, tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), @@ -52,6 +36,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { taskRunRunning := tb.TaskRun("test-taskrun-running", testNs, tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), + tb.TaskRunTimeout(config.DefaultTimeoutMinutes*time.Minute), ), tb.TaskRunStatus(tb.Condition(apis.Condition{ Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}), @@ -60,6 +45,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { taskRunDone := tb.TaskRun("test-taskrun-completed", testNs, tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), + tb.TaskRunTimeout(config.DefaultTimeoutMinutes*time.Minute), ), tb.TaskRunStatus(tb.Condition(apis.Condition{ Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue}), @@ -68,6 +54,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { taskRunCancelled := tb.TaskRun("test-taskrun-run-cancelled", testNs, tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name), tb.TaskRunCancelled, + tb.TaskRunTimeout(1*time.Second), ), tb.TaskRunStatus(tb.Condition(apis.Condition{ Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}), @@ -87,7 +74,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { c, _ := test.SeedTestData(t, d) observer, _ := observer.New(zap.InfoLevel) - th := NewTimeoutHandler(stopCh, zap.New(observer).Sugar(), getStore(t)) + th := NewTimeoutHandler(stopCh, zap.New(observer).Sugar()) gotCallback := sync.Map{} f := func(tr interface{}) { trNew := tr.(*v1alpha1.TaskRun) @@ -146,7 +133,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { prTimeout := tb.PipelineRun("test-pipeline-run-with-timeout", testNs, tb.PipelineRunSpec("test-pipeline", tb.PipelineRunServiceAccount("test-sa"), - tb.PipelineRunTimeout(&metav1.Duration{Duration: 1 * time.Second}), + tb.PipelineRunTimeout(1*time.Second), ), tb.PipelineRunStatus( tb.PipelineRunStartTime(time.Now().AddDate(0, 0, -1))), @@ -154,7 +141,9 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { ts := tb.Task("hello-world", testNs) prRunning := tb.PipelineRun("test-pipeline-running", testNs, - tb.PipelineRunSpec("test-pipeline"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute), + ), tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{ Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}), @@ -162,7 +151,9 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { ), ) prDone := tb.PipelineRun("test-pipeline-done", testNs, - tb.PipelineRunSpec("test-pipeline"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute), + ), tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{ Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue}), @@ -171,6 +162,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { prCancelled := tb.PipelineRun("test-pipeline-cancel", testNs, tb.PipelineRunSpec("test-pipeline", tb.PipelineRunServiceAccount("test-sa"), tb.PipelineRunCancelled, + tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute), ), tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{ Type: apis.ConditionSucceeded, @@ -190,7 +182,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { c, _ := test.SeedTestData(t, d) stopCh := make(chan struct{}) observer, _ := observer.New(zap.InfoLevel) - th := NewTimeoutHandler(stopCh, zap.New(observer).Sugar(), getStore(t)) + th := NewTimeoutHandler(stopCh, zap.New(observer).Sugar()) defer close(stopCh) gotCallback := sync.Map{} @@ -265,7 +257,7 @@ func TestWithNoFunc(t *testing.T) { stopCh := make(chan struct{}) c, _ := test.SeedTestData(t, d) observer, _ := observer.New(zap.InfoLevel) - testHandler := NewTimeoutHandler(stopCh, zap.New(observer).Sugar(), getStore(t)) + testHandler := NewTimeoutHandler(stopCh, zap.New(observer).Sugar()) defer func() { // this delay will ensure there is no race condition between stopCh/ timeout channel getting triggered time.Sleep(10 * time.Millisecond) @@ -278,35 +270,6 @@ func TestWithNoFunc(t *testing.T) { } -// TestGetTimeout checks that the timeout calculated for a given taskrun falls -// back to the default taskrun timeout when none is provided. -func TestGetTimeout(t *testing.T) { - testCases := []struct { - description string - inputDuration *metav1.Duration - expectedDuration time.Duration - }{ - { - description: "returns same duration as input when input is not nil", - inputDuration: &metav1.Duration{Duration: 2 * time.Second}, - expectedDuration: 2 * time.Second, - }, - { - description: "returns an end time using the default timeout if a TaskRun's timeout is nil", - inputDuration: nil, - expectedDuration: 60 * time.Minute, - }, - } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - receivedDuration := GetTimeout(tc.inputDuration, 60) - if receivedDuration != tc.expectedDuration { - t.Errorf("expected %q received %q", tc.expectedDuration.String(), receivedDuration.String()) - } - }) - } -} - // TestSetTaskRunTimer checks that the SetTaskRunTimer method correctly calls the TaskRun // callback after a set amount of time. func TestSetTaskRunTimer(t *testing.T) { @@ -321,7 +284,7 @@ func TestSetTaskRunTimer(t *testing.T) { stopCh := make(chan struct{}) observer, _ := observer.New(zap.InfoLevel) - testHandler := NewTimeoutHandler(stopCh, zap.New(observer).Sugar(), getStore(t)) + testHandler := NewTimeoutHandler(stopCh, zap.New(observer).Sugar()) timerDuration := 50 * time.Millisecond timerFailDeadline := 100 * time.Millisecond doneCh := make(chan struct{}) diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index 233e8ed9c80..cbd2983280b 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -25,7 +25,6 @@ import ( "github.com/knative/pkg/apis" duckv1beta1 "github.com/knative/pkg/apis/duck/v1beta1" "github.com/knative/pkg/configmap" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/reconciler" @@ -56,18 +55,7 @@ func getPipelineRunController(t *testing.T, d test.Data, recorder record.EventRe stopCh := make(chan struct{}) configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace()) logger := zap.New(observer).Sugar() - store := apisconfig.NewStore(logger) - defaultConfig := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: "config-defaults", - }, - Data: map[string]string{ - "default-timeout-minutes": "60", - }, - } - store.OnConfigChanged(defaultConfig) - th := reconciler.NewTimeoutHandler(stopCh, logger, store) + th := reconciler.NewTimeoutHandler(stopCh, logger) return test.TestAssets{ Controller: NewController( reconciler.Options{ @@ -643,7 +631,7 @@ func TestReconcileWithTimeout(t *testing.T) { prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-with-timeout", "foo", tb.PipelineRunSpec("test-pipeline", tb.PipelineRunServiceAccount("test-sa"), - tb.PipelineRunTimeout(&metav1.Duration{Duration: 12 * time.Hour}), + tb.PipelineRunTimeout(12*time.Hour), ), tb.PipelineRunStatus( tb.PipelineRunStartTime(time.Now().AddDate(0, 0, -1))), @@ -1001,7 +989,7 @@ func TestReconcileWithTimeoutAndRetry(t *testing.T) { prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-retry-run-with-timeout", "foo", tb.PipelineRunSpec("test-pipeline-retry", tb.PipelineRunServiceAccount("test-sa"), - tb.PipelineRunTimeout(&metav1.Duration{Duration: 12 * time.Hour}), + tb.PipelineRunTimeout(12*time.Hour), ), tb.PipelineRunStatus( tb.PipelineRunStartTime(time.Now().AddDate(0, 0, -1))), diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index b622a59a555..fde57dd889b 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -26,7 +26,6 @@ import ( "github.com/knative/pkg/apis" "github.com/knative/pkg/controller" "github.com/knative/pkg/tracker" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" informers "github.com/tektoncd/pipeline/pkg/client/informers/externalversions/pipeline/v1alpha1" @@ -70,7 +69,6 @@ type Reconciler struct { tracker tracker.Interface cache *entrypoint.Cache timeoutHandler *reconciler.TimeoutSet - store *apisconfig.Store } // Check that our Reconciler implements controller.Reconciler @@ -86,7 +84,6 @@ func NewController( podInformer coreinformers.PodInformer, entrypointCache *entrypoint.Cache, timeoutHandler *reconciler.TimeoutSet, - store *apisconfig.Store, ) *controller.Impl { c := &Reconciler{ @@ -96,10 +93,6 @@ func NewController( clusterTaskLister: clusterTaskInformer.Lister(), resourceLister: resourceInformer.Lister(), timeoutHandler: timeoutHandler, - store: store, - } - if c.store == nil { - c.store = apisconfig.NewStore(c.Logger.Named("config-store")) } impl := controller.NewImpl(c, c.Logger, taskRunControllerName) @@ -540,8 +533,7 @@ func (c *Reconciler) checkTimeout(tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec, d return false, nil } - cfg := c.store.Load() - timeout := reconciler.GetTimeout(tr.Spec.Timeout, cfg.ConfigDefault.DefaultTimeoutMinutes) + timeout := tr.Spec.Timeout.Duration runtime := time.Since(tr.Status.StartTime.Time) c.Logger.Infof("Checking timeout for TaskRun %q (startTime %s, timeout %s, runtime %s)", tr.Name, tr.Status.StartTime, timeout, runtime) diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 192c8edadc9..b955bebba9a 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -25,7 +25,6 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/knative/pkg/apis" "github.com/knative/pkg/configmap" - apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/logging" @@ -224,19 +223,8 @@ func getTaskRunController(t *testing.T, d test.Data) test.TestAssets { configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace()) stopCh := make(chan struct{}) logger := zap.New(observer).Sugar() - store := apisconfig.NewStore(logger.Named("config-store")) - defaultConfig := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tekton-pipelines", - Name: "config-defaults", - }, - Data: map[string]string{ - "default-timeout-minutes": "60", - }, - } - store.OnConfigChanged(defaultConfig) - th := reconciler.NewTimeoutHandler(stopCh, logger, store) + th := reconciler.NewTimeoutHandler(stopCh, logger) return test.TestAssets{ Controller: NewController( reconciler.Options{ @@ -252,7 +240,6 @@ func getTaskRunController(t *testing.T, d test.Data) test.TestAssets { i.Pod, entrypointCache, th, - store, ), Logs: logs, Clients: c, diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go index 1610c11ed6a..09758a1329f 100644 --- a/test/builder/pipeline.go +++ b/test/builder/pipeline.go @@ -17,6 +17,7 @@ import ( "time" "github.com/knative/pkg/apis" + "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" @@ -251,6 +252,8 @@ func PipelineRunSpec(name string, ops ...PipelineRunSpecOp) PipelineRunOp { prs := &pr.Spec prs.PipelineRef.Name = name + // Set a default timeout + prs.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute} for _, op := range ops { op(prs) @@ -331,9 +334,9 @@ func PipelineRunParam(name, value string) PipelineRunSpecOp { } // PipelineRunTimeout sets the timeout to the PipelineSpec. -func PipelineRunTimeout(duration *metav1.Duration) PipelineRunSpecOp { +func PipelineRunTimeout(duration time.Duration) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { - prs.Timeout = duration + prs.Timeout = &metav1.Duration{Duration: duration} } } diff --git a/test/builder/pipeline_test.go b/test/builder/pipeline_test.go index 0de6130654f..c24d1a9a0a0 100644 --- a/test/builder/pipeline_test.go +++ b/test/builder/pipeline_test.go @@ -101,7 +101,7 @@ func TestPipelineRun(t *testing.T) { pipelineRun := tb.PipelineRun("pear", "foo", tb.PipelineRunSpec( "tomatoes", tb.PipelineRunServiceAccount("sa"), tb.PipelineRunParam("first-param", "first-value"), - tb.PipelineRunTimeout(&metav1.Duration{Duration: 1 * time.Hour}), + tb.PipelineRunTimeout(1*time.Hour), tb.PipelineRunResourceBinding("some-resource", tb.PipelineResourceBindingRef("my-special-resource")), tb.PipelineRunServiceAccountTask("foo", "sa-2"), ), tb.PipelineRunStatus(tb.PipelineRunStatusCondition( diff --git a/test/builder/task.go b/test/builder/task.go index 6e7ab9a9957..dd3bcb7d17e 100644 --- a/test/builder/task.go +++ b/test/builder/task.go @@ -17,6 +17,7 @@ import ( "time" "github.com/knative/pkg/apis" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/resources" corev1 "k8s.io/api/core/v1" @@ -423,6 +424,8 @@ func TaskRunAnnotation(key, value string) TaskRunOp { func TaskRunSpec(ops ...TaskRunSpecOp) TaskRunOp { return func(tr *v1alpha1.TaskRun) { spec := &tr.Spec + // Set a default timeout + spec.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute} for _, op := range ops { op(spec) } diff --git a/test/builder/task_test.go b/test/builder/task_test.go index cdaa6260e68..3e6cf8caf2e 100644 --- a/test/builder/task_test.go +++ b/test/builder/task_test.go @@ -20,6 +20,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/knative/pkg/apis" duckv1beta1 "github.com/knative/pkg/apis/duck/v1beta1" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/resources" tb "github.com/tektoncd/pipeline/test/builder" @@ -203,6 +204,7 @@ func TestTaskRunWithTaskRef(t *testing.T) { Paths: []string{"output-folder"}, }}, }, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, TaskRef: &v1alpha1.TaskRef{ Name: "task-output", Kind: v1alpha1.ClusterTaskKind, diff --git a/test/timeout_test.go b/test/timeout_test.go index c56763773c3..7463208ebed 100644 --- a/test/timeout_test.go +++ b/test/timeout_test.go @@ -52,7 +52,7 @@ func TestPipelineRunTimeout(t *testing.T) { tb.PipelineSpec(tb.PipelineTask("foo", "banana")), ) pipelineRun := tb.PipelineRun("pear", namespace, tb.PipelineRunSpec(pipeline.Name, - tb.PipelineRunTimeout(&metav1.Duration{Duration: 5 * time.Second}), + tb.PipelineRunTimeout(5*time.Second), )) if _, err := c.PipelineClient.Create(pipeline); err != nil { t.Fatalf("Failed to create Pipeline `%s`: %s", pipeline.Name, err)