Update build execution time #335

shashwathi · 2018-09-05T21:34:15Z

Proposed Changes

Update build execution time to be (build finish time - pod start time)
Add integration test to check build status

shashwathi · 2018-09-07T15:20:46Z

/assign @imjasonh
/assign @pivotal-nader-ziada

nader-ziada · 2018-09-07T15:36:21Z

test/e2e/simple_test.go

+	}
+}
+
+func TestBuildWithHighTimeout(t *testing.T) {


Not sure you need this test here, since the build is actually successful

+1, our test that a build under the timeout passes is TestSimpleBuild

I agree. I wanted to test a build with higher timeout set. As an alternate idea I could alter the SimpleBuild test to have a timeout as well.

- Add comments

imjasonh · 2018-09-07T15:42:23Z

test/e2e/simple_test.go

+		t.Fatalf("watchBuild did not return expected BuildTimeout error")
+	}
+
+	// verify reason for build failure is timeout


Can we add a check that the build executed for 20s, +/- some wiggle room? That wiggle room will be pretty high today, since we only poll every 30s.

Okay how about we check if build duration is between buildTimeout and buildTimeout + 40s(30s poll + 10s tolerance)?

Yeah that sounds good. Add a TODO to reduce the wiggle room once we're more strict, and maybe run the test a few times to make sure it's not flaky.

I ran into couple of edge cases while re-running the test. So I fixed couple of bugs in this PR.

imjasonh · 2018-09-07T15:43:13Z

test/e2e/simple_test.go

+
+	b, err := clients.buildClient.watchBuild(buildName)
+	if err == nil {
+		t.Fatalf("watchBuild did not return expected BuildTimeout error")


This could probably be t.Error, and we can keep checking other things about the returned build.

imjasonh · 2018-09-07T15:43:41Z

test/e2e/simple_test.go

+	}
+}
+
+func TestBuildWithHighTimeout(t *testing.T) {


+1, our test that a build under the timeout passes is TestSimpleBuild

- Fix build completed time bug for cancelle builds

- If build node selector is not present build never timesout, then e2e never fail/succeed. - Moved bad node selector test into e2e so its easier to test that build is in pending state until build watch exits.

shashwathi · 2018-09-07T20:00:11Z

@imjasonh @pivotal-nader-ziada : Addressed all comments and fixed e2e tests. Please take a look at this and provide feedback

nader-ziada · 2018-09-07T20:51:37Z

/lgtm

mattmoor · 2018-09-08T01:53:32Z

pkg/builder/interface.go

@@ -75,7 +75,8 @@ 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


nit: IsTimeout (lowercase o)

mattmoor · 2018-09-08T01:53:58Z

test/e2e/simple_test.go

@@ -82,6 +83,7 @@ func TestSimpleBuild(t *testing.T) {
 			Name:      buildName,
 		},
 		Spec: v1alpha1.BuildSpec{
+			Timeout: "40s",


Can we use an actual time.Duration for this field?

Thats a great idea. I have updated the PR with this change.

shashwathi · 2018-09-10T16:50:39Z

If timeout is not specified then it will be equal to 0 and thats valid input. so I will update the logic to below

if timeout < 0 {
  // bad
} else if timeout > maxTimeout {
  // bad
}
// good

WDYT? @imjasonh

shashwathi · 2018-09-10T17:00:59Z

Go serializes Duration in nanoseconds. User needs to be aware of this conversion. I have added more comments in spec and test as well. I will update the docs after this PR is merged.

I am not sure of this change anymore. Strings made it simple and readable format for developer. Thoughts @imjasonh @pivotal-nader-ziada @mattmoor

nader-ziada · 2018-09-10T17:55:23Z

I would prefer as a user to enter the build timeout in seconds, not nanoseconds.

shashwathi · 2018-09-10T17:55:57Z

/test pull-knative-build-integration-tests

shashwathi · 2018-09-10T19:12:42Z

/test pull-knative-build-integration-tests

imjasonh · 2018-09-10T20:23:38Z

I would prefer as a user to enter the build timeout in seconds, not nanoseconds.

Agreed. I would have thought Go would be smart enough to accept the string form of a time.Duration while JSON-unmarshalling, but apparently it's not, and that's just gross.

Sorry for the back-and-forth, but if Go's not going to let us say timeout: 3m18s and instead will make me do math (ugh), then I think we should have the field be a string and to the duration parsing ourselves. 😞

shashwathi · 2018-09-10T20:33:02Z

I will update the PR with build duration as string. 👍

knative-metrics-robot · 2018-09-10T21:42:28Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/build/controller.go	50.0%	50.4%	0.4

shashwathi · 2018-09-10T22:01:00Z

@imjasonh : Addressed all comments. Let me know if you have any comments on this PR.

imjasonh

/lgtm
/approve

imjasonh · 2018-09-11T14:14:38Z

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

@@ -18,7 +18,7 @@ metadata:
  labels:
    expect: failed
 spec:
-  timeout: 50s
+  timeout: "50s"


These quotes aren't necessary right? I don't feel strongly about removing them, I just want to make sure this all works if a user doesn't explicitly quote the string.

Quotes are not necessary. When I changed the type into string I might have gone on a spree to add quotes everywhere :D I will follow up with a PR to address all your comments.

imjasonh · 2018-09-11T14:15:46Z

test/e2e/simple_test.go

+	clients := setup(t)
+
+	buildName := "build-low-timeout"
+	buildTimeout := "50s"


Optional: buildTimeout = 50*time.Second then Timeout: buildTimeout.String() means you don't have to parse and handle the error down on line 169. Up to you, it's also fine like this.

imjasonh · 2018-09-11T14:17:04Z

test/e2e/simple_test.go

+	if err != nil {
+		t.Errorf("Error parsing build duration: %v", err)
+	}
+	higherEnd := 90 * time.Second // build timeout + 30 sec poll time + 10 sec


If this value is intended to be based on buildTimeout, maybe it should explicitly be based on it?

higherEnd := buildTimeout + 30*time.Second + 10*time.Second

That way if we change buildTimeout above we won't also have to remember to update this value.

knative-prow-robot · 2018-09-11T14:17:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ImJasonH]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tcnghia · 2018-09-17T23:59:11Z

@shashwathi @imjasonh can we use https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/duration.go instead of strings here?

shashwathi · 2018-09-18T13:40:41Z

Thats a great idea. We rejected the idea of using time.Duration because native Golang time Duration does not support strings unmarshal. I can follow up with a PR to use k8s duration type. Thanks for finding that @tcnghia

* build execution time=(build finish time - pod start time) * Add e2e tests for build with low and high timeout * Remove redundant test - Add comments * Add assertion for build duration - Fix build completed time bug for cancelle builds * Reduce timeout for faster feedback * Fix unit test * Move bad node selector into e2e from YAML test - If build node selector is not present build never timesout, then e2e never fail/succeed. - Moved bad node selector test into e2e so its easier to test that build is in pending state until build watch exits. * Update timeout from string to duration * update build duration for timeout * Address comments * Fix timeout logic * Refactor and address comments * Update timeout in test * Revert to using build duration as string instead of Duration * go fmt update * Update test build with string timeout

build execution time=(build finish time - pod start time)

dae3fed

knative-prow-robot added the do-not-merge/work-in-progress label Sep 5, 2018

knative-prow-robot requested review from imjasonh and mattmoor September 5, 2018 21:34

knative-prow-robot added the size/XS label Sep 5, 2018

Add e2e tests for build with low and high timeout

3f17b56

knative-prow-robot added size/M and removed size/XS labels Sep 7, 2018

shashwathi changed the title ~~[WIP] Update build execution time~~ Update build execution time Sep 7, 2018

knative-prow-robot removed the do-not-merge/work-in-progress label Sep 7, 2018

knative-prow-robot assigned imjasonh and nader-ziada Sep 7, 2018

nader-ziada reviewed Sep 7, 2018

View reviewed changes

Remove redundant test

b01d6af

- Add comments

imjasonh reviewed Sep 7, 2018

View reviewed changes

Shash Reddy added 4 commits September 7, 2018 14:23

Add assertion for build duration

e028b2f

- Fix build completed time bug for cancelle builds

Reduce timeout for faster feedback

9d5e48b

Fix unit test

958b00b

Move bad node selector into e2e from YAML test

b0809d6

- If build node selector is not present build never timesout, then e2e never fail/succeed. - Moved bad node selector test into e2e so its easier to test that build is in pending state until build watch exits.

knative-prow-robot added size/L and removed size/M labels Sep 7, 2018

knative-prow-robot added the lgtm label Sep 7, 2018

mattmoor reviewed Sep 8, 2018

View reviewed changes

Update timeout from string to duration

c09dbdc

knative-prow-robot removed the lgtm label Sep 10, 2018

update build duration for timeout

73f1478

Fix timeout logic

45bd5c0

Shash Reddy added 2 commits September 10, 2018 12:50

Refactor and address comments

fbd2d41

Update timeout in test

eb0ea68

adrcunha mentioned this pull request Sep 10, 2018

e2e tests are trying to create an unsupported k8s cluster knative/test-infra#108

Closed

Shash Reddy added 3 commits September 10, 2018 17:15

Revert to using build duration as string instead of Duration

7286782

go fmt update

61b4568

Update test build with string timeout

6fc208c

shashwathi mentioned this pull request Sep 11, 2018

Add Pod affinity to builds #344

Merged

imjasonh approved these changes Sep 11, 2018

View reviewed changes

knative-prow-robot added the lgtm label Sep 11, 2018

knative-prow-robot added the approved label Sep 11, 2018

knative-prow-robot merged commit 0ad3c32 into knative:master Sep 11, 2018

shashwathi mentioned this pull request Sep 11, 2018

Test cleanup #346

Merged

adrcunha mentioned this pull request Sep 14, 2018

Emit metric to track null build latency #337

Merged

shashwathi deleted the update-execution-time branch September 18, 2018 13:40

shashwathi mentioned this pull request Sep 19, 2018

Use k8s Duration instead of string for build timeout #365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update build execution time #335

Update build execution time #335

shashwathi commented Sep 5, 2018 •

edited

Loading

shashwathi commented Sep 7, 2018

nader-ziada Sep 7, 2018

imjasonh Sep 7, 2018

shashwathi Sep 7, 2018

imjasonh Sep 7, 2018

shashwathi Sep 7, 2018

imjasonh Sep 7, 2018

shashwathi Sep 7, 2018

imjasonh Sep 7, 2018

imjasonh Sep 7, 2018

shashwathi commented Sep 7, 2018

nader-ziada commented Sep 7, 2018

mattmoor Sep 8, 2018

mattmoor Sep 8, 2018

shashwathi Sep 10, 2018 •

edited

Loading

shashwathi commented Sep 10, 2018 •

edited

Loading

shashwathi commented Sep 10, 2018 •

edited

Loading

nader-ziada commented Sep 10, 2018

shashwathi commented Sep 10, 2018

shashwathi commented Sep 10, 2018

imjasonh commented Sep 10, 2018

shashwathi commented Sep 10, 2018

knative-metrics-robot commented Sep 10, 2018

shashwathi commented Sep 10, 2018

imjasonh left a comment

imjasonh Sep 11, 2018

shashwathi Sep 11, 2018

imjasonh Sep 11, 2018

imjasonh Sep 11, 2018

knative-prow-robot commented Sep 11, 2018

tcnghia commented Sep 17, 2018

shashwathi commented Sep 18, 2018

Update build execution time #335

Update build execution time #335

Conversation

shashwathi commented Sep 5, 2018 • edited Loading

Proposed Changes

shashwathi commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi commented Sep 7, 2018

nader-ziada commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

shashwathi commented Sep 10, 2018 • edited Loading

shashwathi commented Sep 10, 2018 • edited Loading

nader-ziada commented Sep 10, 2018

shashwathi commented Sep 10, 2018

shashwathi commented Sep 10, 2018

imjasonh commented Sep 10, 2018

shashwathi commented Sep 10, 2018

knative-metrics-robot commented Sep 10, 2018

shashwathi commented Sep 10, 2018

imjasonh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 11, 2018

tcnghia commented Sep 17, 2018

shashwathi commented Sep 18, 2018

shashwathi commented Sep 5, 2018 •

edited

Loading

shashwathi Sep 10, 2018 •

edited

Loading

shashwathi commented Sep 10, 2018 •

edited

Loading

shashwathi commented Sep 10, 2018 •

edited

Loading