Add pod start time by shashwathi · Pull Request #322 · knative/build

shashwathi · 2018-08-29T20:51:04Z

Fixes #321

Proposed Changes

Rename current status.StartTime to status.CreationTime
Add pod start time as status.StartTime

shashwathi · 2018-08-29T20:51:32Z

shashwathi · 2018-08-29T21:08:25Z

I really want to add this build example as part of this PR as it shows startTime set to null and creationTime set to build created time.

Ideally the following example should fail because pods are never created and build times out after 3 minutes. The problem is that as the build times out, pods are terminated and pod termination is unsuccessful (because pod does not exist) and build status is not updated to failed which causes build to remain in Unknown state.

apiVersion: build.knative.dev/v1alpha1
kind: Build
metadata:
  name: test-build-with-badselector
  labels:
    expect: failed
spec:
  timeout: 3m
  steps:
  - name: step1
    image: ubuntu
    command: ["/bin/bash"]
    args: ["-c", "sleep 15"]
  nodeSelector:
    disk: "fake-ssd"

One way to approach this problem is instead of just bubbling of all pod termination errors, it might be better to filter out Not Found errors. Another easy approach would be to update build status before calling Terminate() function(not a fan of this idea).

thoughts ? ideas?
@imjasonh @pivotal-nader-ziada @dprotaso

imjasonh · 2018-08-29T21:10:55Z

+	}
 	if bs.StartTime.IsZero() {
-		t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime)
+		t.Errorf("status.StartTime; want non-zero, got %v", bs.StartTime)


Can you update the other error messages for consistency? Or return this to bs.

imjasonh · 2018-08-29T21:12:10Z


-	build.Status.StartTime.Time = metav1.Now().Time.Add(-addBuffer)
+	// Set creation time to Now()-10min
+	build.Status.CreationTime.Time = metav1.Now().Time.Add(buffer)


Previously this was -addBuffer, now it's buffer, can you explain why this is?

It made more sense to call the variable as buffer. Ideally I want to subtract the buffer time to trigger timeout.

Also just realized that line is not required for test setup. Let me update.

- Set creation time to now()-buffer to trigger timeout when updating build status

imjasonh · 2018-08-29T21:26:11Z

I think we should filter out not found errors from Terminate, which I imagine could happen for other reasons too.

shashwathi · 2018-09-04T13:34:23Z

@imjasonh : I have addressed your comments. Let me know if I missed anything.

imjasonh · 2018-09-04T13:54:39Z

 	"fmt"
 	"sync"

+	v1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"


Can we separate these first-party imports (anything from knative/build) from the vendored imports from other packages?

imjasonh · 2018-09-04T13:58:13Z

 	}
-
-	return time.Since(status.StartTime.Time).Seconds() > timeout.Seconds()
+	return time.Since(status.CreationTime.Time).Seconds() > timeout.Seconds()


Time spent scheduling the pod shouldn't count toward the build's timeout, should this consider status.StartTime instead of status.CreationTime ?

Unless we were thinking this would be a useful way to kill a build that was unschedulable, in which case we might need to introduce two separate timeouts.

In GCB we only enforce timeouts once the build starts, since it's not the user's fault if it takes us a long time to start their build. We've considered enforcing a timeout on queued time, but I don't think any user has asked for it.

If we don't enforce timeout for allocating resources then build will remain in pending state forever. I was planning to follow up with a PR to introduce another timeout for allocating resources(queue timeout). Probably default this timeout to 10min and user doesn't need to worry about using this.

Okay, if that's on your radar for future changes that sounds good to me.

Created: #325 to follow up with changes discussed in this PR.

imjasonh · 2018-09-04T13:59:54Z

@@ -1,2 +1,2 @@
-NAME	BUILDER	TYPE	STATUS	START	END
-.metadata.name	.status.builder	.status.conditions[*].state	.status.conditions[*].status	.status.startTime	.status.completionTime
+NAME	BUILDER	TYPE	STATUS	CREATED  START	END


Is it possible to add columns to show startup duration and build execution duration? I don't know how powerful this template language is, if it's hard then don't worry about it, we can try to add it later.

Also, we can remove the BUILDER columns now that there's only one builder implementation.

If the build has started but pod hasn't been scheduled then is it okay to have negative duration? Might be better off calculating that in status if we need that info.

Also, we can remove the BUILDER columns now that there's only one builder implementation.

Done

We should avoid a negative duration, since that would be pretty hard to interpret. I'm fine putting this off for some future change where we add startup and build duration to the status and make it easy to display here. Don't worry about it for now.

- Re-arrange imports - Remove builder from custom columns txt

knative-metrics-robot · 2018-09-04T15:20:11Z

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/builder/cluster/builder.go	78.6%	78.4%	-0.3
pkg/builder/nop/builder.go	79.2%	80.0%	0.8

imjasonh

/lgtm
/approve

knative-prow-robot · 2018-09-04T15:20:56Z

[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

Details

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

shashwathi · 2018-09-04T15:31:50Z

/test pull-knative-build-integration-tests

shashwathi · 2018-09-04T16:57:14Z

/test pull-knative-build-integration-tests

shashwathi · 2018-09-04T20:57:09Z

/test pull-knative-build-integration-tests

* Rename startTime -> creationTime * add build actual start time to status * Add comments and fix tests * Refactor start time function * Fix error msgs * Update test setup - Set creation time to now()-buffer to trigger timeout when updating build status * Remove unnecessary test setup for timeout * Add example with bad node selector * Address comments - Re-arrange imports - Remove builder from custom columns txt

Shash Reddy added 4 commits August 29, 2018 12:02

Rename startTime -> creationTime

15e19d1

add build actual start time to status

d1755fe

Add comments and fix tests

51ce7a1

Refactor start time function

abee971

knative-prow-robot requested review from imjasonh and srinivashegde86 August 29, 2018 20:51

knative-prow-robot added the size/L label Aug 29, 2018

knative-prow-robot assigned imjasonh Aug 29, 2018

imjasonh suggested changes Aug 29, 2018

View reviewed changes

Shash Reddy added 2 commits August 29, 2018 17:17

Fix error msgs

7acaead

Update test setup

6958950

- Set creation time to now()-buffer to trigger timeout when updating build status

Shash Reddy added 2 commits August 29, 2018 17:26

Remove unnecessary test setup for timeout

4be4746

Add example with bad node selector

8faf9d1

imjasonh reviewed Sep 4, 2018

View reviewed changes

Address comments

23eab08

- Re-arrange imports - Remove builder from custom columns txt

imjasonh approved these changes Sep 4, 2018

View reviewed changes

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

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

knative-prow-robot merged commit 66a754c into knative:master Sep 4, 2018

shashwathi deleted the add-pod-start-time branch September 4, 2018 21:16

Conversation

shashwathi commented Aug 29, 2018

Proposed Changes

Uh oh!

shashwathi commented Aug 29, 2018

Uh oh!

shashwathi commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imjasonh commented Aug 29, 2018

Uh oh!

shashwathi commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shashwathi Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Sep 4, 2018

Uh oh!

imjasonh left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 4, 2018

Uh oh!

shashwathi commented Sep 4, 2018

Uh oh!

shashwathi commented Sep 4, 2018

Uh oh!

shashwathi commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shashwathi commented Sep 4, 2018 •

edited

Loading

shashwathi Sep 4, 2018 •

edited

Loading