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

Jump to bottom

Refactor some taskrun fields into helper functions #1110

Merged
merged 2 commits into from
Jul 24, 2019
Merged

Refactor some taskrun fields into helper functions #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8218f4f
Refactor some taskrun fields into helper functions
dibyom Jul 22, 2019
4bc47d3
Add unit tests for getTaskRunTimeout
dibyom Jul 23, 2019
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
131 changes: 72 additions & 59 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,14 @@ import (
"github.com/knative/pkg/configmap"
"github.com/knative/pkg/controller"
"github.com/knative/pkg/tracker"
"go.uber.org/zap"
"golang.org/x/xerrors"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/cache"

"github.com/tektoncd/pipeline/pkg/apis/config"
apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -36,13 +44,6 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/pipeline/dag"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/pipelinerun/resources"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun"
"go.uber.org/zap"
"golang.org/x/xerrors"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/cache"
)

const (
Expand Down Expand Up @@ -396,54 +397,6 @@ 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 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.
taskRunTimeout = &metav1.Duration{Duration: time.Until(pTimeoutTime)}
if taskRunTimeout.Duration < 0 {
taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second}
}
} else {
taskRunTimeout = &metav1.Duration{Duration: timeout}
}
}

// If service account is configured for a given PipelineTask, override PipelineRun's seviceAccount
serviceAccount := pr.Spec.ServiceAccount
for _, sa := range pr.Spec.ServiceAccounts {
if sa.TaskName == rprt.PipelineTask.Name {
serviceAccount = sa.ServiceAccount
}
}

// Propagate labels from PipelineRun to TaskRun.
labels := make(map[string]string, len(pr.ObjectMeta.Labels)+1)
for key, val := range pr.ObjectMeta.Labels {
labels[key] = val
}
labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] = pr.Name
if rprt.PipelineTask.Name != "" {
labels[pipeline.GroupName+pipeline.PipelineTaskLabelKey] = rprt.PipelineTask.Name
}

// Propagate annotations from PipelineRun to TaskRun.
annotations := make(map[string]string, len(pr.ObjectMeta.Annotations)+1)
for key, val := range pr.ObjectMeta.Annotations {
annotations[key] = val
}

tr, _ := c.taskRunLister.TaskRuns(pr.Namespace).Get(rprt.TaskRunName)

if tr != nil {
Expand All @@ -463,8 +416,8 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re
Name: rprt.TaskRunName,
Namespace: pr.Namespace,
OwnerReferences: pr.GetOwnerReference(),
Labels: labels,
Annotations: annotations,
Labels: getTaskrunLabels(pr, rprt.PipelineTask.Name),
Annotations: getTaskrunAnnotations(pr),
},
Spec: v1alpha1.TaskRunSpec{
TaskRef: &v1alpha1.TaskRef{
Expand All @@ -474,8 +427,8 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re
Inputs: v1alpha1.TaskRunInputs{
Params: rprt.PipelineTask.Params,
},
ServiceAccount: serviceAccount,
Timeout: taskRunTimeout,
ServiceAccount: getServiceAccount(pr, rprt.PipelineTask.Name),
Timeout: getTaskRunTimeout(pr),
PodTemplate: podTemplate,
}}

Expand All @@ -497,6 +450,66 @@ func clearStatus(tr *v1alpha1.TaskRun) {
tr.Status.PodName = ""
}

func getTaskrunAnnotations(pr *v1alpha1.PipelineRun) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these are still covered by the mega-tests in taskrun_test/pipelinerun_test, but do you think it might be worthwhile to simplify those tests and add coverage here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, will add some!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

// Propagate annotations from PipelineRun to TaskRun.
annotations := make(map[string]string, len(pr.ObjectMeta.Annotations)+1)
for key, val := range pr.ObjectMeta.Annotations {
annotations[key] = val
}
return annotations
}

func getTaskrunLabels(pr *v1alpha1.PipelineRun, pipelineTaskName string) map[string]string {
// Propagate labels from PipelineRun to TaskRun.
labels := make(map[string]string, len(pr.ObjectMeta.Labels)+1)
for key, val := range pr.ObjectMeta.Labels {
labels[key] = val
}
labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] = pr.Name
if pipelineTaskName != "" {
labels[pipeline.GroupName+pipeline.PipelineTaskLabelKey] = pipelineTaskName
}
return labels
}

func getTaskRunTimeout(pr *v1alpha1.PipelineRun) *metav1.Duration {
var taskRunTimeout = &metav1.Duration{Duration: apisconfig.NoTimeoutDuration}

var timeout time.Duration
if pr.Spec.Timeout == nil {
timeout = config.DefaultTimeoutMinutes * time.Minute
} 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 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.
taskRunTimeout = &metav1.Duration{Duration: time.Until(pTimeoutTime)}
if taskRunTimeout.Duration < 0 {
taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second}
}
} else {
taskRunTimeout = &metav1.Duration{Duration: timeout}
}
}
return taskRunTimeout
}

func getServiceAccount(pr *v1alpha1.PipelineRun, pipelineTaskName string) string {
// If service account is configured for a given PipelineTask, override PipelineRun's seviceAccount
serviceAccount := pr.Spec.ServiceAccount
for _, sa := range pr.Spec.ServiceAccounts {
if sa.TaskName == pipelineTaskName {
serviceAccount = sa.ServiceAccount
}
}
return serviceAccount
}

func (c *Reconciler) updateStatus(pr *v1alpha1.PipelineRun) (*v1alpha1.PipelineRun, error) {
newPr, err := c.pipelineRunLister.PipelineRuns(pr.Namespace).Get(pr.Name)
if err != nil {
Expand Down
55 changes: 52 additions & 3 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import (
duckv1beta1 "github.com/knative/pkg/apis/duck/v1beta1"
"github.com/knative/pkg/configmap"
rtesting "github.com/knative/pkg/reconciler/testing"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ktesting "k8s.io/client-go/testing"

"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/pipelinerun/resources"
Expand All @@ -34,9 +39,6 @@ import (
"github.com/tektoncd/pipeline/test"
tb "github.com/tektoncd/pipeline/test/builder"
"github.com/tektoncd/pipeline/test/names"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ktesting "k8s.io/client-go/testing"
)

func getRunName(pr *v1alpha1.PipelineRun) string {
Expand Down Expand Up @@ -1005,3 +1007,50 @@ func TestReconcilePropagateAnnotations(t *testing.T) {
t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, d)
}
}

func TestGetTaskRunTimeout(t *testing.T) {
prName := "pipelinerun-timeouts"
ns := "foo"
p := "pipeline"

tcs := []struct {
name string
pr *v1alpha1.PipelineRun
expected *metav1.Duration
}{{
name: "nil timeout duration",
pr: tb.PipelineRun(prName, ns,
tb.PipelineRunSpec(p, tb.PipelineRunNilTimeout),
tb.PipelineRunStatus(tb.PipelineRunStartTime(time.Now())),
),
expected: &metav1.Duration{Duration: 60 * time.Minute},
}, {
name: "timeout specified in pr",
pr: tb.PipelineRun(prName, ns,
tb.PipelineRunSpec(p, tb.PipelineRunTimeout(20*time.Minute)),
tb.PipelineRunStatus(tb.PipelineRunStartTime(time.Now())),
),
expected: &metav1.Duration{Duration: 20 * time.Minute},
}, {
name: "0 timeout duration",
pr: tb.PipelineRun(prName, ns,
tb.PipelineRunSpec(p, tb.PipelineRunTimeout(0*time.Minute)),
tb.PipelineRunStatus(tb.PipelineRunStartTime(time.Now())),
),
expected: &metav1.Duration{Duration: 0 * time.Minute},
}, {
name: "taskrun being created after timeout expired",
pr: tb.PipelineRun(prName, ns,
tb.PipelineRunSpec(p, tb.PipelineRunTimeout(1*time.Minute)),
tb.PipelineRunStatus(tb.PipelineRunStartTime(time.Now().Add(-2*time.Minute)))),
expected: &metav1.Duration{Duration: 1 * time.Second},
}}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
if d := cmp.Diff(getTaskRunTimeout(tc.pr), tc.expected); d != "" {
t.Errorf("Unexpected task run timeout. Diff %s", d)
}
})
}
}