diff --git a/pkg/apis/build/v1alpha1/build_types.go b/pkg/apis/build/v1alpha1/build_types.go index cc595e07..5d0a1a2e 100644 --- a/pkg/apis/build/v1alpha1/build_types.go +++ b/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"` } diff --git a/pkg/builder/interface.go b/pkg/builder/interface.go index 6a6fb671..b5f14fcf 100644 --- a/pkg/builder/interface.go +++ b/pkg/builder/interface.go @@ -75,9 +75,11 @@ 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 { @@ -85,14 +87,20 @@ func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout string) bool { } 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. diff --git a/pkg/controller/build/controller.go b/pkg/controller/build/controller.go index b2fcc4c0..5479d001 100644 --- a/pkg/controller/build/controller.go +++ b/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 { diff --git a/pkg/controller/build/controller_test.go b/pkg/controller/build/controller_test.go index f7d76035..fa3ee6a0 100644 --- a/pkg/controller/build/controller_test.go +++ b/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\"" diff --git a/pkg/webhook/build.go b/pkg/webhook/build.go index 1eaa29c3..bcb13e67 100644 --- a/pkg/webhook/build.go +++ b/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") } } diff --git a/test/bad-nodeselector/build-with-bad-selector.yaml b/test/bad-nodeselector/build-with-bad-selector.yaml deleted file mode 100644 index 51f9aafc..00000000 --- a/test/bad-nodeselector/build-with-bad-selector.yaml +++ /dev/null @@ -1,15 +0,0 @@ -apiVersion: build.knative.dev/v1alpha1 -kind: Build -metadata: - name: test-build-with-badselector - labels: - expect: failed -spec: - timeout: 2m - steps: - - name: step1 - image: ubuntu - command: ["/bin/bash"] - args: ["-c", "sleep 15"] - nodeSelector: - disk: "fake-ssd" \ No newline at end of file diff --git a/test/build-with-timeout/build-with-high-timeout.yaml b/test/build-with-timeout/build-with-high-timeout.yaml index 7f2c0c1e..1698051b 100644 --- a/test/build-with-timeout/build-with-high-timeout.yaml +++ b/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 diff --git a/test/build-with-timeout/build-with-low-timeout.yaml b/test/build-with-timeout/build-with-low-timeout.yaml index 76a30176..9936d763 100644 --- a/test/build-with-timeout/build-with-low-timeout.yaml +++ b/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 diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index 671e6e98..8a79c029 100644 --- a/test/e2e/e2e.go +++ b/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 } diff --git a/test/e2e/simple_test.go b/test/e2e/simple_test.go index 1e25da23..8e7176cd 100644 --- a/test/e2e/simple_test.go +++ b/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") + } +}