Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not panic if Timeout is nil #1085

Merged
merged 1 commit into from
Jul 16, 2019
Merged

Do not panic if Timeout is nil #1085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions pkg/reconciler/timeout_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"time"

"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"
Expand Down Expand Up @@ -204,14 +205,26 @@ 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) {
t.waitRun(tr, tr.Spec.Timeout.Duration, startTime, t.taskRunCallbackFunc)
var timeout time.Duration
if tr.Spec.Timeout == nil {
timeout = config.DefaultTimeoutMinutes * time.Minute
} else {
timeout = tr.Spec.Timeout.Duration
}
t.waitRun(tr, timeout, 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) {
t.waitRun(pr, pr.Spec.Timeout.Duration, startTime, t.pipelineRunCallbackFunc)
var timeout time.Duration
if pr.Spec.Timeout == nil {
timeout = config.DefaultTimeoutMinutes * time.Minute
} else {
timeout = pr.Spec.Timeout.Duration
}
t.waitRun(pr, timeout, startTime, t.pipelineRunCallbackFunc)
}

func (t *TimeoutSet) waitRun(runObj StatusKey, timeout time.Duration, startTime *metav1.Time, callback func(interface{})) {
Expand Down
31 changes: 29 additions & 2 deletions pkg/reconciler/timeout_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
tb.TaskRunStartTime(time.Now()),
))

taskRunRunningNilTimeout := tb.TaskRun("test-taskrun-running-nil-timeout", testNs, tb.TaskRunSpec(
tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunNilTimeout,
), tb.TaskRunStatus(tb.Condition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown}),
tb.TaskRunStartTime(time.Now()),
))

taskRunDone := tb.TaskRun("test-taskrun-completed", testNs, tb.TaskRunSpec(
tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunTimeout(config.DefaultTimeoutMinutes*time.Minute),
Expand All @@ -62,7 +71,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
))

d := test.Data{
TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled},
TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled, taskRunRunningNilTimeout},
Tasks: []*v1alpha1.Task{simpleTask},
Namespaces: []*corev1.Namespace{{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -98,6 +107,10 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
name: "running",
taskRun: taskRunRunning,
expectCallback: false,
}, {
name: "running-with-nil-timeout",
taskRun: taskRunRunningNilTimeout,
expectCallback: false,
}, {
name: "completed",
taskRun: taskRunDone,
Expand Down Expand Up @@ -152,6 +165,16 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
tb.PipelineRunStartTime(time.Now()),
),
)
prRunningNilTimeout := tb.PipelineRun("test-pipeline-running-nil-timeout", testNs,
tb.PipelineRunSpec("test-pipeline",
tb.PipelineRunNilTimeout,
),
tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown}),
tb.PipelineRunStartTime(time.Now()),
),
)
prDone := tb.PipelineRun("test-pipeline-done", testNs,
tb.PipelineRunSpec("test-pipeline",
tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute),
Expand All @@ -172,7 +195,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
),
)
d := test.Data{
PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled},
PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled, prRunningNilTimeout},
Pipelines: []*v1alpha1.Pipeline{simplePipeline},
Tasks: []*v1alpha1.Task{ts},
Namespaces: []*corev1.Namespace{{
Expand Down Expand Up @@ -205,6 +228,10 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
name: "pr-running",
pr: prRunning,
expectCallback: false,
}, {
name: "pr-running-nil-timeout",
pr: prRunningNilTimeout,
expectCallback: false,
}, {
name: "pr-timedout",
pr: prTimeout,
Expand Down
13 changes: 10 additions & 3 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/knative/pkg/configmap"
"github.com/knative/pkg/controller"
"github.com/knative/pkg/tracker"
"github.com/tektoncd/pipeline/pkg/apis/config"
apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
Expand Down Expand Up @@ -397,10 +398,16 @@ func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1alpha1.PipelineRun) erro
func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.ResolvedPipelineRunTask, pr *v1alpha1.PipelineRun, storageBasePath string) (*v1alpha1.TaskRun, error) {
var taskRunTimeout = &metav1.Duration{Duration: apisconfig.NoTimeoutDuration}

var timeout time.Duration
if pr.Spec.Timeout == nil {
timeout = config.DefaultTimeoutMinutes
} else {
timeout = pr.Spec.Timeout.Duration
}
// If the value of the timeout is 0 for any resource, there is no timeout.
// It is impossible for pr.Spec.Timeout to be nil, since SetDefault always assigns it with a value.
if pr.Spec.Timeout.Duration != apisconfig.NoTimeoutDuration {
pTimeoutTime := pr.Status.StartTime.Add(pr.Spec.Timeout.Duration)
if timeout != apisconfig.NoTimeoutDuration {
pTimeoutTime := pr.Status.StartTime.Add(timeout)
if time.Now().After(pTimeoutTime) {
// Just in case something goes awry and we're creating the TaskRun after it should have already timed out,
// set the timeout to 1 second.
Expand All @@ -409,7 +416,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re
taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second}
}
} else {
taskRunTimeout = pr.Spec.Timeout
taskRunTimeout = &metav1.Duration{Duration: timeout}
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/reconciler/v1alpha1/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/knative/pkg/apis"
"github.com/knative/pkg/controller"
"github.com/knative/pkg/tracker"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1alpha1"
Expand Down Expand Up @@ -213,6 +214,9 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
tr.ObjectMeta.Annotations[key] = value
}

if tr.Spec.Timeout == nil {
tr.Spec.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}
}
// Check if the TaskRun has timed out; if it is, this will set its status
// accordingly.
if CheckTimeout(tr) {
Expand Down Expand Up @@ -483,7 +487,6 @@ func createRedirectedTaskSpec(kubeclient kubernetes.Interface, ts *v1alpha1.Task
type DeletePod func(podName string, options *metav1.DeleteOptions) error

func (c *Reconciler) updateTaskRunStatusForTimeout(tr *v1alpha1.TaskRun, dp DeletePod) error {
timeout := tr.Spec.Timeout.Duration
c.Logger.Infof("TaskRun %q has timed out, deleting pod", tr.Name)
// tr.Status.PodName will be empty if the pod was never successfully created. This condition
// can be reached, for example, by the pod never being schedulable due to limits imposed by
Expand All @@ -495,6 +498,7 @@ func (c *Reconciler) updateTaskRunStatusForTimeout(tr *v1alpha1.TaskRun, dp Dele
}
}

timeout := tr.Spec.Timeout.Duration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm this is interesting cuz it means if the order of the call to this function were to change, this could still be nil 🤔

timeoutMsg := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, timeout.String())
tr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Expand Down
22 changes: 20 additions & 2 deletions pkg/reconciler/v1alpha1/taskrun/taskrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,8 +1364,8 @@ func TestReconcilePodUpdateStatus(t *testing.T) {
t.Fatalf("Unexpected error fetching taskrun: %v", err)
}
if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionTrue,
Type: apis.ConditionSucceeded,
Status: corev1.ConditionTrue,
Reason: status.ReasonSucceeded,
Message: "All Steps have completed executing",
}, ignoreLastTransitionTime); d != "" {
Expand Down Expand Up @@ -1514,6 +1514,24 @@ func TestReconcileTimeouts(t *testing.T) {
Message: `TaskRun "test-taskrun-default-timeout-60-minutes" failed to finish within "1h0m0s"`,
},
},
{
taskRun: tb.TaskRun("test-taskrun-nil-timeout-default-60-minutes", "foo",
tb.TaskRunSpec(
tb.TaskRunTaskRef(simpleTask.Name),
tb.TaskRunNilTimeout,
),
tb.TaskRunStatus(tb.Condition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown}),
tb.TaskRunStartTime(time.Now().Add(-61*time.Minute)))),

expectedStatus: &apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: "TaskRunTimeout",
Message: `TaskRun "test-taskrun-nil-timeout-default-60-minutes" failed to finish within "1h0m0s"`,
},
},
}

for _, tc := range testcases {
Expand Down
7 changes: 6 additions & 1 deletion test/builder/pipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,13 +333,18 @@ func PipelineRunParam(name, value string) PipelineRunSpecOp {
}
}

// PipelineRunTimeout sets the timeout to the PipelineSpec.
// PipelineRunTimeout sets the timeout to the PipelineRunSpec.
func PipelineRunTimeout(duration time.Duration) PipelineRunSpecOp {
return func(prs *v1alpha1.PipelineRunSpec) {
prs.Timeout = &metav1.Duration{Duration: duration}
}
}

// PipelineRunNilTimeout sets the timeout to nil on the PipelineRunSpec
func PipelineRunNilTimeout(prs *v1alpha1.PipelineRunSpec) {
prs.Timeout = nil
}

// PipelineRunNodeSelector sets the Node selector to the PipelineSpec.
func PipelineRunNodeSelector(values map[string]string) PipelineRunSpecOp {
return func(prs *v1alpha1.PipelineRunSpec) {
Expand Down
5 changes: 5 additions & 0 deletions test/builder/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,11 @@ func TaskRunTimeout(d time.Duration) TaskRunSpecOp {
}
}

// TaskRunNilTimeout sets the timeout duration to nil on the TaskRunSpec.
func TaskRunNilTimeout(spec *v1alpha1.TaskRunSpec) {
spec.Timeout = nil
}

// TaskRunNodeSelector sets the NodeSelector to the PipelineSpec.
func TaskRunNodeSelector(values map[string]string) TaskRunSpecOp {
return func(spec *v1alpha1.TaskRunSpec) {
Expand Down