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.

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

Consolidate cancel and timeout logic #2365

Merged
merged 1 commit into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 28 additions & 0 deletions pkg/apis/pipeline/v1alpha1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package v1alpha1

import (
"fmt"
"time"

apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -77,6 +79,10 @@ const (
// TaskRunSpecStatusCancelled indicates that the user wants to cancel the task,
// if not already cancelled or terminated
TaskRunSpecStatusCancelled = v1beta1.TaskRunSpecStatusCancelled

// TaskRunReasonCancelled indicates that the TaskRun has been cancelled
// because it was requested so by the user
TaskRunReasonCancelled = v1beta1.TaskRunSpecStatusCancelled
)

// TaskRunInputs holds the input values that this task was invoked with.
Expand Down Expand Up @@ -223,6 +229,28 @@ func (tr *TaskRun) IsCancelled() bool {
return tr.Spec.Status == TaskRunSpecStatusCancelled
}

// HasTimedOut returns true if the TaskRun runtime is beyond the allowed timeout
func (tr *TaskRun) HasTimedOut() bool {
if tr.Status.StartTime.IsZero() {
return false
}
timeout := tr.GetTimeout()
// If timeout is set to 0 or defaulted to 0, there is no timeout.
if timeout == apisconfig.NoTimeoutDuration {
return false
}
runtime := time.Since(tr.Status.StartTime.Time)
return runtime > timeout
}

func (tr *TaskRun) GetTimeout() time.Duration {
// Use the platform default is no timeout is set
if tr.Spec.Timeout == nil {
return apisconfig.DefaultTimeoutMinutes * time.Minute
}
return tr.Spec.Timeout.Duration
}

// GetRunKey return the taskrun key for timeout handler map
func (tr *TaskRun) GetRunKey() string {
// The address of the pointer is a threadsafe unique identifier for the taskrun
Expand Down
37 changes: 37 additions & 0 deletions pkg/apis/pipeline/v1alpha1/taskrun_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,40 @@ func TestTaskRunIsOfPipelinerun(t *testing.T) {
})
}
}

func TestHasTimedOut(t *testing.T) {
// IsZero reports whether t represents the zero time instant, January 1, year 1, 00:00:00 UTC
zeroTime := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)
testCases := []struct {
name string
taskRun *v1alpha1.TaskRun
expectedStatus bool
}{{
name: "TaskRun not started",
taskRun: tb.TaskRun("test-taskrun-not-started", "foo", tb.TaskRunSpec(
tb.TaskRunTaskRef("task-name"),
), tb.TaskRunStatus(tb.StatusCondition(apis.Condition{}), tb.TaskRunStartTime(zeroTime))),
expectedStatus: false,
}, {
name: "TaskRun no timeout",
taskRun: tb.TaskRun("test-taskrun-no-timeout", "foo", tb.TaskRunSpec(
tb.TaskRunTaskRef("task-name"), tb.TaskRunTimeout(0),
), tb.TaskRunStatus(tb.StatusCondition(apis.Condition{}), tb.TaskRunStartTime(time.Now().Add(-15*time.Hour)))),
expectedStatus: false,
}, {
name: "TaskRun timed out",
taskRun: tb.TaskRun("test-taskrun-timeout", "foo", tb.TaskRunSpec(
tb.TaskRunTaskRef("task-name"), tb.TaskRunTimeout(10*time.Second),
), tb.TaskRunStatus(tb.StatusCondition(apis.Condition{}), tb.TaskRunStartTime(time.Now().Add(-15*time.Second)))),
expectedStatus: true,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := tc.taskRun.HasTimedOut()
if d := cmp.Diff(result, tc.expectedStatus); d != "" {
t.Fatalf("-want, +got: %v", d)
}
})
}
}
38 changes: 38 additions & 0 deletions pkg/apis/pipeline/v1beta1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"time"

apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -71,6 +72,10 @@ const (
// TaskRunSpecStatusCancelled indicates that the user wants to cancel the task,
// if not already cancelled or terminated
TaskRunSpecStatusCancelled = "TaskRunCancelled"

// TaskRunReasonCancelled indicates that the TaskRun has been cancelled
// because it was requested so by the user
TaskRunReasonCancelled = "TaskRunCancelled"
)

// TaskRunInputs holds the input values that this task was invoked with.
Expand Down Expand Up @@ -120,6 +125,17 @@ func (trs *TaskRunStatus) MarkResourceNotConvertible(err *CannotConvertError) {
})
}

// MarkResourceFailed sets the ConditionSucceeded condition to ConditionFalse
// based on an error that occurred and a reason
func (trs *TaskRunStatus) MarkResourceFailed(reason string, err error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this should be in alpha too...

Copy link
Member

Choose a reason for hiding this comment

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

// TaskRunStatus defines the observed state of TaskRun
type TaskRunStatus = v1beta1.TaskRunStatus

It doesn't as it's just an alias in v1alpha1 😉

taskRunCondSet.Manage(trs).SetCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: reason,
Message: err.Error(),
})
}

// TaskRunStatusFields holds the fields of TaskRun's status. This is defined
// separately and inlined so that other types can readily consume these fields
// via duck typing.
Expand Down Expand Up @@ -333,6 +349,28 @@ func (tr *TaskRun) IsCancelled() bool {
return tr.Spec.Status == TaskRunSpecStatusCancelled
}

// HasTimedOut returns true if the TaskRun runtime is beyond the allowed timeout
func (tr *TaskRun) HasTimedOut() bool {
if tr.Status.StartTime.IsZero() {
return false
}
timeout := tr.GetTimeout()
// If timeout is set to 0 or defaulted to 0, there is no timeout.
if timeout == apisconfig.NoTimeoutDuration {
return false
}
runtime := time.Since(tr.Status.StartTime.Time)
return runtime > timeout
}

func (tr *TaskRun) GetTimeout() time.Duration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @afrittoli are you sure we need this function? looking at the taskrun defaulting logic it looks like we default the value of the timeout:

https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1beta1/taskrun_defaults.go#L47

also we don't have the same function for pipelineruns, and finally finally apisconfig.DefaultTimeoutMinutes is a default value afaik; but the user can actually override it with a config map so we probably want to use that instead (there is some similar logic in the timeout logic that i think is being overly cautious - its always hard to know when you can rely on the defaulting to be called)

// Use the platform default is no timeout is set
if tr.Spec.Timeout == nil {
return apisconfig.DefaultTimeoutMinutes * time.Minute
}
return tr.Spec.Timeout.Duration
}

// GetRunKey return the taskrun key for timeout handler map
func (tr *TaskRun) GetRunKey() string {
// The address of the pointer is a threadsafe unique identifier for the taskrun
Expand Down
77 changes: 77 additions & 0 deletions pkg/apis/pipeline/v1beta1/taskrun_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,80 @@ func TestTaskRunIsOfPipelinerun(t *testing.T) {
})
}
}

func TestHasTimedOut(t *testing.T) {
// IsZero reports whether t represents the zero time instant, January 1, year 1, 00:00:00 UTC
zeroTime := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC)
testCases := []struct {
name string
taskRun *v1beta1.TaskRun
expectedStatus bool
}{{
name: "TaskRun not started",
taskRun: &v1beta1.TaskRun{
Status: v1beta1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
}},
},
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: zeroTime},
},
},
},
expectedStatus: false,
}, {
name: "TaskRun no timeout",
taskRun: &v1beta1.TaskRun{
Spec: v1beta1.TaskRunSpec{
Timeout: &metav1.Duration{
Duration: 0 * time.Minute,
},
},
Status: v1beta1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
}},
},
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: time.Now().Add(-15 * time.Hour)},
},
},
},
expectedStatus: false,
}, {
name: "TaskRun timed out",
taskRun: &v1beta1.TaskRun{
Spec: v1beta1.TaskRunSpec{
Timeout: &metav1.Duration{
Duration: 10 * time.Second,
},
},
Status: v1beta1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
}},
},
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
StartTime: &metav1.Time{Time: time.Now().Add(-15 * time.Second)},
},
},
},
expectedStatus: true,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := tc.taskRun.HasTimedOut()
if d := cmp.Diff(result, tc.expectedStatus); d != "" {
t.Fatalf("-want, +got: %v", d)
}
})
}
}
1 change: 0 additions & 1 deletion pkg/reconciler/pipelinerun/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,5 +168,4 @@ func assertErrIsNil(err error, message string, t *testing.T) {

func unregisterMetrics() {
metricstest.Unregister("pipelinerun_duration_seconds", "pipelinerun_count", "running_pipelineruns_count")

}
1 change: 1 addition & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ func getRunName(pr *v1alpha1.PipelineRun) string {
// getPipelineRunController returns an instance of the PipelineRun controller/reconciler that has been seeded with
// d, where d represents the state of the system (existing resources) needed for the test.
func getPipelineRunController(t *testing.T, d test.Data) (test.Assets, func()) {
//unregisterMetrics()
ctx, _ := ttesting.SetupFakeContext(t)
c, _ := test.SeedTestData(t, ctx, d)
configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace())
Expand Down
53 changes: 0 additions & 53 deletions pkg/reconciler/taskrun/cancel.go

This file was deleted.

Loading