Update build execution time

pkg/apis/build/v1alpha1/build_types.go

@@ -73,7 +73,8 @@ type BuildSpec struct {
 	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
 
 	// Time after which the build times out. Defaults to 10 minutes.
-	//  Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration
+	// 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"`
 }
 

pkg/builder/interface.go

@@ -75,24 +75,32 @@ func IsDone(status *v1alpha1.BuildStatus) bool {
 	return false
 }
 
-// IsTimedOut returns true if the build's status indicates that build is timedout.
+// IsTimeout returns true if the build's execution time is greater than
+// specified build spec timeout.
 func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout string) bool {
 	var timeout time.Duration
+	var defaultTimeout = 10 * time.Minute
 	var err error
 
 	if status == nil {
 		return false
 	}
 
 	if buildTimeout == "" {
-		timeout = 10 * time.Minute
+		// 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
 		}
 	}
-	return time.Since(status.CreationTime.Time).Seconds() > timeout.Seconds()
+
+	// If build has not started timeout, startTime should be zero.
+	if status.StartTime.Time.IsZero() {
+		return false
+	}
+	return time.Since(status.StartTime.Time).Seconds() > timeout.Seconds()
 }
 
 // ErrorMessage returns the error message from the status.

pkg/controller/build/controller.go

@@ -294,7 +294,8 @@ func (c *Controller) syncHandler(key string) error {
 					Reason:  "BuildTimeout",
 					Message: timeoutMsg,
 				})
-
+				// update build completed time
+				build.Status.CompletionTime = metav1.Now()
 				c.recorder.Eventf(build, corev1.EventTypeWarning, "BuildTimeout", timeoutMsg)
 
 				if _, err := c.updateStatus(build); err != nil {
@@ -303,7 +304,7 @@ func (c *Controller) syncHandler(key string) error {
 				}
 
 				c.logger.Errorf("Timeout: %v", timeoutMsg)
-				return fmt.Errorf("Build %q timed out after %s", build.Name, build.Spec.Timeout)
+				return nil
 			} else {
 				// if not timed out then wait async
 				if err := c.waitForOperationAsync(build, op); err != nil {

pkg/controller/build/controller_test.go

@@ -145,7 +145,7 @@ func TestBasicFlows(t *testing.T) {
 			t.Errorf("error fetching build: %v", err)
 		}
 		// Update status to current time
-		first.Status.CreationTime = metav1.Now()
+		first.Status.StartTime = metav1.Now()
 
 		if builder.IsDone(&first.Status) {
 			t.Errorf("First IsDone(%d); wanted not done, got done.", idx)
@@ -282,8 +282,8 @@ func TestTimeoutFlows(t *testing.T) {
 	f.updateIndex(i, []*v1alpha1.Build{first})
 
 	// Run a second iteration of the syncHandler.
-	if err := c.syncHandler(getKey(build, t)); err == nil {
-		t.Errorf("Expect syncing build to error with timeout")
+	if err := c.syncHandler(getKey(build, t)); err != nil {
+		t.Errorf("Unexpected error while syncing build: %v", err)
 	}
 
 	expectedTimeoutMsg := "Warning BuildTimeout Build \"test\" failed to finish within \"1s\""

pkg/webhook/build.go

@@ -243,13 +243,11 @@ func validateTimeout(timeout string) error {
 	if timeout == "" {
 		return nil
 	} else {
-		timeout, err := time.ParseDuration(timeout)
+		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)
 		}
-
-		// time out should not greater than 24 hours
-		if timeout.Hours() > maxTimeout.Hours() {
+		if timeoutDuration > maxTimeout {
 			return validationError("InvalidTimeout", "build timeout exceeded 24h")
 		}
 	}

test/build-with-timeout/build-with-high-timeout.yaml

@@ -18,7 +18,7 @@ metadata:
   labels:
     expect: succeeded
 spec:
-  timeout: 500s
+  timeout: "500s"
   steps:
   - name: step1
     image: ubuntu

test/build-with-timeout/build-with-low-timeout.yaml

@@ -18,7 +18,7 @@ metadata:
   labels:
     expect: failed
 spec:
-  timeout: 50s
+  timeout: "50s"
   steps:
   - name: step1
     image: ubuntu

test/e2e/e2e.go

@@ -86,7 +86,14 @@ type buildClient struct {
 }
 
 func (c *buildClient) watchBuild(name string) (*v1alpha1.Build, error) {
-	w, err := c.builds.Watch(metav1.SingleObject(metav1.ObjectMeta{Name: name}))
+	ls := metav1.SingleObject(metav1.ObjectMeta{Name: name})
+	// TODO: Update watchBuild function to take this as parameter depending on test requirements
+
+	// Set build timeout to 120 seconds. This will trigger watch timeout error
+	var timeout int64 = 120
+	ls.TimeoutSeconds = &timeout
+
+	w, err := c.builds.Watch(ls)
 	if err != nil {
 		return nil, err
 	}

test/e2e/simple_test.go

@@ -23,6 +23,7 @@ import (
 	"log"
 	"os"
 	"testing"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	kuberrors "k8s.io/apimachinery/pkg/api/errors"
@@ -82,6 +83,7 @@ func TestSimpleBuild(t *testing.T) {
 			Name:      buildName,
 		},
 		Spec: v1alpha1.BuildSpec{
+			Timeout: "40s",
 			Steps: []corev1.Container{{
 				Image: "busybox",
 				Args:  []string{"echo", "simple"},
@@ -120,3 +122,90 @@ func TestFailingBuild(t *testing.T) {
 		t.Fatalf("watchBuild did not return expected error: %v", err)
 	}
 }
+
+func TestBuildLowTimeout(t *testing.T) {
+	clients := setup(t)
+
+	buildName := "build-low-timeout"
+	buildTimeout := "50s"
+	if _, err := clients.buildClient.builds.Create(&v1alpha1.Build{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: buildTestNamespace,
+			Name:      buildName,
+		},
+		Spec: v1alpha1.BuildSpec{
+			Timeout: buildTimeout,
+			Steps: []corev1.Container{{
+				Name:    "lowtimeoutstep",
+				Image:   "ubuntu",
+				Command: []string{"/bin/bash"},
+				Args:    []string{"-c", "sleep 2000"},
+			}},
+		},
+	}); err != nil {
+		t.Fatalf("Error creating build: %v", err)
+	}
+
+	b, err := clients.buildClient.watchBuild(buildName)
+	if err == nil {
+		t.Error("watchBuild did not return expected BuildTimeout error")
+	}
+
+	if &b.Status == nil {
+		t.Fatalf("wanted build status to be set; got nil")
+	}
+
+	successCondition := b.Status.GetCondition(v1alpha1.BuildSucceeded)
+
+	if successCondition == nil {
+		t.Fatalf("wanted build status to be set; got %q", b.Status)
+	}
+
+	// verify reason for build failure is timeout
+	if successCondition.Reason != "BuildTimeout" {
+		t.Fatalf("wanted BuildTimeout; got %q", successCondition.Reason)
+	}
+	buildDuration := b.Status.CompletionTime.Time.Sub(b.Status.StartTime.Time).Seconds()
+	lowerEnd, err := time.ParseDuration(buildTimeout)
+	if err != nil {
+		t.Errorf("Error parsing build duration: %v", err)
+	}
+	higherEnd := 90 * time.Second // build timeout + 30 sec poll time + 10 sec
+
+	if !(buildDuration >= lowerEnd.Seconds() && buildDuration < higherEnd.Seconds()) {
+		t.Fatalf("Expected the build duration to be within range %.2fs to %.2fs; but got build duration: %f, start time: %q and completed time: %q \n",
+			lowerEnd.Seconds(),
+			higherEnd.Seconds(),
+			buildDuration,
+			b.Status.StartTime.Time,
+			b.Status.CompletionTime.Time,
+		)
+	}
+}
+
+// TestPendingBuild tests that a build with non existent node selector will remain in pending
+// state until watch timeout.
+func TestPendingBuild(t *testing.T) {
+	clients := setup(t)
+
+	buildName := "pending-build"
+	if _, err := clients.buildClient.builds.Create(&v1alpha1.Build{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: buildTestNamespace,
+			Name:      buildName,
+		},
+		Spec: v1alpha1.BuildSpec{
+			NodeSelector: map[string]string{"disk": "fake-ssd"},
+			Steps: []corev1.Container{{
+				Image: "busybox",
+				Args:  []string{"false"}, // fails.
+			}},
+		},
+	}); err != nil {
+		t.Fatalf("Error creating build: %v", err)
+	}
+
+	if _, err := clients.buildClient.watchBuild(buildName); err == nil {
+		t.Fatalf("watchBuild did not return watch timeout error")
+	}
+}