Skip to content
This repository has been archived by the owner on Sep 5, 2019. It is now read-only.

Use k8s Duration instead of string for build timeout #365

Merged
merged 4 commits into from
Sep 19, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion pkg/apis/build/v1alpha1/build_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ type BuildSpec struct {
// Time after which the build times out. Defaults to 10 minutes.
// Specified build timeout should be less than 24h.
// Refer Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration
Timeout string `json:"timeout,omitempty"`
Timeout metav1.Duration `json:"timeout,omitempty"`

// If specified, the pod's scheduling constraints
// +optional
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/build/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 4 additions & 7 deletions pkg/builder/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"time"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

v1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
)
Expand Down Expand Up @@ -79,23 +80,19 @@ func IsDone(status *v1alpha1.BuildStatus) bool {

// IsTimeout returns true if the build's execution time is greater than
// specified build spec timeout.
func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout string) bool {
func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout metav1.Duration) bool {
var timeout time.Duration
var defaultTimeout = 10 * time.Minute
var err error

if status == nil {
return false
}

if buildTimeout == "" {
if buildTimeout.Duration == 0 {
// Set default timeout to 10 minute if build timeout is not set
timeout = defaultTimeout
} else {
timeout, err = time.ParseDuration(buildTimeout)
if err != nil {
return false
}
timeout = buildTimeout.Duration
}

// If build has not started timeout, startTime should be zero.
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/build/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ func (c *Controller) syncHandler(key string) error {
// Check if build has timed out
if builder.IsTimeout(&build.Status, build.Spec.Timeout) {
//cleanup operation and update status
timeoutMsg := fmt.Sprintf("Build %q failed to finish within %q", build.Name, build.Spec.Timeout)
timeoutMsg := fmt.Sprintf("Build %q failed to finish within %q", build.Name, build.Spec.Timeout.Duration.String())

if err := op.Terminate(); err != nil {
c.logger.Errorf("Failed to terminate pod: %v", err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/build/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func newBuild(name string) *v1alpha1.Build {
Namespace: metav1.NamespaceDefault,
},
Spec: v1alpha1.BuildSpec{
Timeout: "20m",
Timeout: metav1.Duration{Duration: 20 * time.Minute},
},
}
}
Expand Down Expand Up @@ -237,7 +237,7 @@ func TestTimeoutFlows(t *testing.T) {
t.Errorf("Error parsing duration")
}

build.Spec.Timeout = "1s"
build.Spec.Timeout = metav1.Duration{Duration: 1 * time.Second}

f := &fixture{
t: t,
Expand Down
16 changes: 5 additions & 11 deletions pkg/webhook/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,19 +237,13 @@ func validationError(reason, format string, fmtArgs ...interface{}) error {
}
}

func validateTimeout(timeout string) error {
func validateTimeout(timeout metav1.Duration) error {
maxTimeout := time.Duration(24 * time.Hour)

if timeout == "" {
return nil
} else {
timeoutDuration, err := time.ParseDuration(timeout)
if err != nil {
return validationError("InvalidTimeFormat", "invalid build timeout %q err %#v. Refer https://golang.org/pkg/time/#ParseDuration for time format documentation", timeout, err)
}
if timeoutDuration > maxTimeout {
return validationError("InvalidTimeout", "build timeout exceeded 24h")
}
if timeout.Duration > maxTimeout {
return validationError("InvalidTimeout", "build timeout exceeded 24h")
} else if timeout.Duration < 0 {
return validationError("InvalidTimeFormat", "build timeout should be greater than 0")
Copy link
Member

Choose a reason for hiding this comment

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

The reason should be InvalidFormat just like a timeout >24h, otherwise everything LGTM

}
return nil
}
11 changes: 6 additions & 5 deletions pkg/webhook/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"strings"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/mattbaird/jsonpatch"
Expand Down Expand Up @@ -207,21 +208,21 @@ func TestValidateBuild(t *testing.T) {
},
},
}, {
reason: "invalid build timeout",
reason: "negative build timeout",
build: &v1alpha1.Build{
Spec: v1alpha1.BuildSpec{
Timeout: "garbagetimeout",
Timeout: metav1.Duration{Duration: -48 * time.Hour},
Copy link
Member

Choose a reason for hiding this comment

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

This test case should be "negative timeout" now

Steps: []corev1.Container{{
Name: "foo",
Image: "gcr.io/foo-bar/baz:latest",
}},
},
},
},
}, {
reason: "maximum timeout",
build: &v1alpha1.Build{
Spec: v1alpha1.BuildSpec{
Timeout: "48h",
Timeout: metav1.Duration{Duration: 48 * time.Hour},
Steps: []corev1.Container{{
Name: "foo",
Image: "gcr.io/foo-bar/baz:latest",
Expand All @@ -231,7 +232,7 @@ func TestValidateBuild(t *testing.T) {
}, {
build: &v1alpha1.Build{
Spec: v1alpha1.BuildSpec{
Timeout: "1m",
Timeout: metav1.Duration{Duration: 5 * time.Minute},
Steps: []corev1.Container{{
Name: "foo",
Image: "gcr.io/foo-bar/baz:latest",
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/simple_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestSimpleBuild(t *testing.T) {
Name: buildName,
},
Spec: v1alpha1.BuildSpec{
Timeout: "40s",
Timeout: metav1.Duration{Duration: 40 * time.Second},
Steps: []corev1.Container{{
Image: "busybox",
Args: []string{"echo", "simple"},
Expand Down Expand Up @@ -132,7 +132,7 @@ func TestBuildLowTimeout(t *testing.T) {
Name: buildName,
},
Spec: v1alpha1.BuildSpec{
Timeout: buildTimeout.String(),
Timeout: metav1.Duration{Duration: buildTimeout},
Steps: []corev1.Container{{
Name: "lowtimeoutstep",
Image: "ubuntu",
Expand Down