Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add completion time field to builds #1326

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Mar 16, 2015

No description provided.

@bparees
Copy link
Contributor Author

bparees commented Mar 16, 2015

fixes #332

@jwforres @smarterclayton ptal. I took an executive action and took pod completion time (actually the time at which the build controller notices the pod is complete) as the build completion timestamp.

@bparees
Copy link
Contributor Author

bparees commented Mar 16, 2015

[test]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1357/)

@jwforres
Copy link
Member

so we know the build object's creationTimestamp and completionTimestamp, but we don't know the creationTimestamp of the pod that was actually running the build, so we don't know the difference between when we asked for the build and when the build actually started running? how big do we expect that diff to be? Just thinking with Jenkins / Travis that builds aren't considered started until they are actually running.

@jwforres
Copy link
Member

You should add this to the Browse -> Builds page while you are at it? Ideally as a diff between creationTimestamp and completionTimestamp, i.e. "Build completed in 5 minutes" or something like that.

@bparees
Copy link
Contributor Author

bparees commented Mar 16, 2015

if @smarterclayton is ok w/ the change, yeah i can add that to the PR.

as for the question of when it started... there shouldn't be a huge difference...the biggest factor is liable to be pulling the builder image, if that is needed, or the pod not getting scheduled at all due to resource constraints.

I could add another field that we set when the pod enters the "running" state, but that leaves us with the possibility of having builds that get a completion timestamp (when they are canceled) and no start timestamp (because when did it "start")? i'm inclined to say this is "good enough" since technically it is tracking when the build object was created.

@bparees
Copy link
Contributor Author

bparees commented Mar 16, 2015

discussed w/ @jwforres and leaning towards adding a "buildStartTime" field that gets set when the pod enters running state, and using the existing creationTimestamp as a "build requested at" value.

@bparees bparees force-pushed the build_completion_time branch 2 times, most recently from 9beea08 to 4f895d1 Compare March 17, 2015 21:25
@bparees
Copy link
Contributor Author

bparees commented Mar 17, 2015

@jwforres ptal. here's what describe outputs now... I went w/ belt+suspenders in outputting start/finish/duration if both start+finish are available:

Name: ruby-sample-build-31e1e8c5-ccec-11e4-bccb-3c970e3bf0b7
Created: 2015-03-17 17:25:54 -0400 EDT
Labels: buildconfig=ruby-sample-build,name=ruby-sample-build,template=application-template-stibuild
Status: Complete
Started: 2015-03-17 17:26:19 -0400 EDT
Finished: 2015-03-17 17:29:09 -0400 EDT
Duration: 2m50s
Build Pod: build-ruby-sample-build-31e1e8c5-ccec-11e4-bccb-3c970e3bf0b7
Strategy: STI
Image: openshift/ruby-20-centos7
Incremental Build: yes
Source Type: Git
URL: git://github.com/openshift/ruby-hello-world.git
Output to: origin-ruby-sample
Output Spec: 172.30.17.158:5000/test/origin-ruby-sample

@jwforres
Copy link
Member

@smarterclayton have you considered doing relative timestamps in calls to describe? like we do in the web console. Something like... 5 minutes ago (2015-03-17 17:26:19 -0400 EDT)

@smarterclayton
Copy link
Contributor

Yes, it's done for pods already so it'd be natural to extend

On Mar 18, 2015, at 9:17 AM, Jessica Forrester notifications@github.com wrote:

@smarterclayton have you considered doing relative timestamps in calls to describe? like we do in the web console. Something like... 5 minutes ago (2015-03-17 17:26:19 -0400 EDT)


Reply to this email directly or view it on GitHub.

@jwforres
Copy link
Member

@bparees discussed with @smarterclayton can you make the timestamps relative, just 5 minutes ago no timestamp

@jwforres
Copy link
Member

Thought about this some more.. make Created relative. For started/finish timestamps if you really want to show them, they probably should just be timestamps since saying started 5 days ago and finished 5 days ago isn't very useful. But all we really need to display is created and duration. It would be nice if Duration was something like...

When waiting to start... (thinking if my build has been stuck in pending for 5 hours i probably care about that and will want to know to bug someone that the build queue is stuck)
Duration: waiting for 5 minutes

When running... (just seeing a started timestamp doesn't immediately tell me anything, seeing how long its been running for immediately tells me if my build is stuck, if say i know it normally runs much faster)
Duration: running for 5 minutes

When completed...
Duration: ran for 5 minutes or maybe just Duration: 5 minutes

@bparees
Copy link
Contributor Author

bparees commented Mar 18, 2015

Pending builds:
Name: ruby-sample-build-829bb313-cd97-11e4-94c6-3c970e3bf0b7
Created: 2015-03-18 13:52:14 -0400 EDT
Status: Pending
Duration: waiting for 7s

Running builds:
Name: ruby-sample-build-829bb313-cd97-11e4-94c6-3c970e3bf0b7
Created: 2015-03-18 13:52:14 -0400 EDT
Status: Running
Started: 2015-03-18 13:52:42 -0400 EDT
Duration: running for 39s

Completed builds:
Name: ruby-sample-build-4ad9a989-cd96-11e4-94c6-3c970e3bf0b7
Created: 2015-03-18 13:43:31 -0400 EDT
Status: Failed
Started: 2015-03-18 13:44:02 -0400 EDT
Finished: 2015-03-18 13:45:22 -0400 EDT
Duration: 1m20s

acceptable, at least for now?

@jwforres
Copy link
Member

Acceptable for now, should look into using something like https://github.com/dustin/go-humanize#times in the future

@bparees
Copy link
Contributor Author

bparees commented Mar 18, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1220/) (Image: devenv-fedora_1077)

@bparees
Copy link
Contributor Author

bparees commented Mar 19, 2015

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 066cb50

openshift-bot pushed a commit that referenced this pull request Mar 19, 2015
@openshift-bot openshift-bot merged commit 69665fc into openshift:master Mar 19, 2015
@bparees bparees deleted the build_completion_time branch March 19, 2015 18:48
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Oct 10, 2017
…service-catalog/' changes from 7011d9e816..a385bb3f67

a385bb3f67 carry: Set external plan name for service-catalog walkthrough
3ec9e5b07a origin build: add origin tooling
8bb631f v0.1.0-rc1 chart updates (openshift#1359)
400e37f Move API to v1beta1 (openshift#1356)
8f55a99 0.0.24 chart updates (openshift#1357)
7ebb8ae Rename Service[Broker|Class|Plan]Status => ClusterService... (openshift#1349)
82a6103 Detect removed catalog entries (openshift#1353)
85125fd Refactor instance spec (openshift#1350)
99c0644 Update to Kubernetes 1.8 (openshift#1334)
a537490 Reformatting printed messages for controller_instance (openshift#1341)
0763ac0 Use field selector for broker delete (openshift#1348)
93fb0e5 remaining renames from ServiceInstanceCredential to ServiceBinding (openshift#1347)
cfdb2ed add text about how updating secrets/params require a 'poke' (openshift#1337)
1d04776 Change PresentInCatalog -> MissingFromBrokerCatalog (openshift#1342)
c95309c Update Service[Broker|Class|Plan] to ClusterService[Broker|Class|Plan] (openshift#1345)
9365039 Fix controller binding queue name, update documents (openshift#1344)
6375eb3 Do not set Ready/False condition in API Server when updating an instance. (openshift#1335)
108f05d Merge branch 'pr/1340'
bd0cb7b gofmt
e0ce4b5 Jenkins modifications
9341cbf unit and integration tests pass
076bfb2 Disambiguate k8s and external names in log messages for broker (openshift#1336)
c59e1e5 types.go changes for review
4ceaaab Rename resources from Service[Class|Plan]  to ClusterService[Class|Plan] (openshift#1327)
556c7d8 Add status.presentInCatalog field to service/plan (openshift#1328)
a6ad96a openshift#1148: Support updates to Instances (openshift#1289)
e36d286 fix RBAC resource names for ClusterServiceBroker (openshift#1330)
78d09ae add timeout (openshift#1326)
92ac8b5 Address comments raised after 1305 was merged (openshift#1321)
aca7543 Rename ServiceBroker to ClusterServiceBroker (openshift#1319)
a2b6ac0 Implementation for binding orphan mitigation (openshift#1241)
bffe4de Chart updates for 0.0.23 release (openshift#1325)
REVERT: 7011d9e816 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: a385bb3f67480a3c49b65b16c1d631b4d5fb5988
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Oct 10, 2017
…service-catalog/' changes from 7011d9e816..3aacfedec6

3aacfedec6 carry: Set external plan name for service-catalog walkthrough
3ec9e5b07a origin build: add origin tooling
8bb631f v0.1.0-rc1 chart updates (openshift#1359)
400e37f Move API to v1beta1 (openshift#1356)
8f55a99 0.0.24 chart updates (openshift#1357)
7ebb8ae Rename Service[Broker|Class|Plan]Status => ClusterService... (openshift#1349)
82a6103 Detect removed catalog entries (openshift#1353)
85125fd Refactor instance spec (openshift#1350)
99c0644 Update to Kubernetes 1.8 (openshift#1334)
a537490 Reformatting printed messages for controller_instance (openshift#1341)
0763ac0 Use field selector for broker delete (openshift#1348)
93fb0e5 remaining renames from ServiceInstanceCredential to ServiceBinding (openshift#1347)
cfdb2ed add text about how updating secrets/params require a 'poke' (openshift#1337)
1d04776 Change PresentInCatalog -> MissingFromBrokerCatalog (openshift#1342)
c95309c Update Service[Broker|Class|Plan] to ClusterService[Broker|Class|Plan] (openshift#1345)
9365039 Fix controller binding queue name, update documents (openshift#1344)
6375eb3 Do not set Ready/False condition in API Server when updating an instance. (openshift#1335)
108f05d Merge branch 'pr/1340'
bd0cb7b gofmt
e0ce4b5 Jenkins modifications
9341cbf unit and integration tests pass
076bfb2 Disambiguate k8s and external names in log messages for broker (openshift#1336)
c59e1e5 types.go changes for review
4ceaaab Rename resources from Service[Class|Plan]  to ClusterService[Class|Plan] (openshift#1327)
556c7d8 Add status.presentInCatalog field to service/plan (openshift#1328)
a6ad96a openshift#1148: Support updates to Instances (openshift#1289)
e36d286 fix RBAC resource names for ClusterServiceBroker (openshift#1330)
78d09ae add timeout (openshift#1326)
92ac8b5 Address comments raised after 1305 was merged (openshift#1321)
aca7543 Rename ServiceBroker to ClusterServiceBroker (openshift#1319)
a2b6ac0 Implementation for binding orphan mitigation (openshift#1241)
bffe4de Chart updates for 0.0.23 release (openshift#1325)
REVERT: 7011d9e816 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 3aacfedec6f8d3d07a5b922e0fcd4a9b28a0e5d2
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants