generate build state events #13660

bparees · 2017-04-06T18:43:13Z

No description provided.

bparees · 2017-04-06T19:50:11Z

[test]

bparees · 2017-04-07T14:08:06Z

(5) Create events for started/completed/failed builds [builds]

bparees · 2017-04-07T14:23:50Z

@jwforres fyi, these are the new build events we're going to be adding.
@openshift/api-review ptal at the events we're introducing.

bparees · 2017-04-07T18:31:22Z

[testextended][extended:core(builds)]

bparees · 2017-04-07T22:04:25Z

[test]

…

On Fri, Apr 7, 2017 at 3:53 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test FAILURE ( https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/645/) (Base Commit: b4b7083 <b4b7083> ) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3iKXQ6oIApZL1zra9BVBdMMv-kv4ks5rtpRJgaJpZM4M2Atp> .

-- Ben Parees | OpenShift

smarterclayton · 2017-04-07T22:12:22Z

pkg/build/api/types.go

@@ -63,6 +63,31 @@ const (
 	// forces the build to be processed by the build controller queue without waiting
 	// for a resync.
 	BuildAcceptedAnnotation = "build.openshift.io/accepted"
+
+	// BuildCreatedEvent is the reason associated with the event registered when a build is created.
+	BuildCreatedEventReason = "BuildCreated"


There's a lot of duplication here. Why do I care about created and started and running? I'd prefer just one, probably started.

we don't have to display them all in the console by default, but is that a good reason not to create them?

I viewed all 3 as interesting primarily in an unhealthy system.
Created - a build was kicked off for some reason (manual, image trigger, git push, etc) - i think i want to know this.

Started - The system is attempting to start working on my build.. particularly relevant if you are doing serial builds, there could be a long time between creating a build and starting it.

Running - In an overscheduled system, there could be a long time between starting a build(pod is created) and the pod getting scheduled. I like to know when my build is actually running, not when it was eligible to run.

I can maybe see the argument for leaving out "Running" because in a healthy system started and running will happen very close together, but i definitely thing created and started are distinct and important due to the serial build policy.

They're interesting in an unhealthy system and completely useless / waste space / clog up event views in a normal system. If the goal is to debug unhealthy, dispatch events for specific unhealthy. I don't think the serial build thing is a good reason, especially if you have conditions on builds that convey that info, because that's the system working as designed.

We already have an event for created which is the watch. We don't fire events when anything else is created, so no reason to start here. We have a long term strategy for user creation stuff which is roughly analogous to an event log for object changes (not just events), and that would subsume this anyway.

In short - I'm really against sending events just because we can. We should send events that materially improve users understanding of the system. I'd rather have 2 events that tell people something really important vs 10 that convey the same total amount of info. Every event is a write to etcd - that cost has to be worth it.

I'd like @jwforres to weigh in, the only reason we're even creating these events is because the web console wanted to be able to convey this information to the user.

All of these are reconstructable from the new status input - what am I missing?

not sure what the "new status input" is, but the idea was for these to feed toaster notifications, i believe.

smarterclayton · 2017-04-07T23:10:06Z

pkg/build/api/types.go

+	// BuildFailedEvent is the message associated with the event registered when build fails.
+	BuildFailedEventMessage = "Build %s/%s failed"
+	// BuildCancelledEvent is the reason associated with the event registered when build is cancelled.
+	BuildCancelledEventReason = "BuildCancelled"


I'd be ok with terminal state build events, in that they are useful for bringing together reason and status together. I'm not really happy with more than a single initial state. I'd encourage brainstorming whether there are events that aren't lifecycle but that actually help resolve questions users have about the system.

I'm not going to cry about making this simpler, but I do worry that if we provide one kind of event, people are just going to ask why we don't provide the other kind... ie if we provide build created events, people will say they actually just want to know when the build started, and vice versa.

so again, let's see what @jwforres wants to display in the webconsole and go from there.

jwforres · 2017-04-10T13:54:54Z

we definitely want success/cancelled/failed

as far as created vs started, we do have a use for both, but if i have to choose one, then I want started. Ideally we were only going to have events feeding our toast notifications and notification drawer, so after someone creates a build we were hoping to just get the event back immediately for creation, and then to @bparees 's point the start may not happen till much later. That said, if we need to then we can send the toast notification to our drawer ourselves when you launch a build and not require an event for it, but knowing when the build actually started definitely needs an event.

bparees · 2017-04-10T14:46:41Z

@jwforres @smarterclayton ok. Do we want the "build started" event to be created when the pod is created (meaning the build has reached the top of the queue for serial builds), or when the pod starts running (meaning the pod was scheduled to a node)?

jwforres · 2017-04-10T15:30:51Z

at what point do you generate startTimestamp? thats what i would want

…

On Mon, Apr 10, 2017 at 10:46 AM, Ben Parees ***@***.***> wrote: @jwforres <https://github.com/jwforres> @smarterclayton <https://github.com/smarterclayton> ok. Do we want the "build started" event to be created when the pod is created (meaning the build has reached the top of the queue for serial builds), or when the pod starts running (meaning the pod was scheduled to a node)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7cAdXakidHrPzeBt8G3MJ5tEkWxgks5rukDagaJpZM4M2Atp> .

bparees · 2017-04-10T15:38:10Z

we set the startimestamp when the pod starts running. So what is currently in the code as the "build running" event would become the build started event (and the existing build created and build started events currently in this PR will go away)

bparees · 2017-04-10T19:41:00Z

@smarterclayton @jwforres ok i've stripped this down to the started event (when the pod starts running) and the terminal state events. needs @openshift/api-review signoff.

@openshift/devex ptal

coreydaley · 2017-04-10T19:44:57Z

pkg/build/api/types.go

@@ -63,6 +63,23 @@ const (
 	// forces the build to be processed by the build controller queue without waiting
 	// for a resync.
 	BuildAcceptedAnnotation = "build.openshift.io/accepted"
+
+	// BuildStartedEvent is the reason associated with the event registered when a build is started (pod is created).


Should be BuildStartedEventReason in comment (also should be fixed in the rest of the comments to match constant names)

csrwng · 2017-04-10T19:43:36Z

pkg/build/api/types.go

+	// BuildStartedEvent is the reason associated with the event registered when a build is started (pod is created).
+	BuildStartedEventReason = "BuildStarted"
+	// BuildStartedEvent is the message associated with the event registered when a build is started (pod is created).
+	BuildStartedEventMessage = "Pod has been created to run build %s/%s"


Could the event ever apply to a pipeline build? If so, maybe a more generic message would be better.

will update

smarterclayton

API is acceptable.

smarterclayton · 2017-04-10T20:22:54Z

test/extended/util/framework.go

@@ -1400,3 +1400,20 @@ func CreateExecPodOnNode(client kcoreclient.CoreInterface, ns, nodeName, name st
 	o.Expect(err).NotTo(o.HaveOccurred())
 	return created.Name
 }
+
+func CheckForEvent(client kcoreclient.CoreInterface, build *buildapi.Build, reason, message string) {


Give this a name that indicates it's checking for build events, or move it to a build specific package.

done (renamed)

csrwng · 2017-04-10T20:32:24Z

lgtm

openshift-bot · 2017-04-10T21:29:32Z

Evaluated for origin testextended up to 1462117

openshift-bot · 2017-04-10T23:05:26Z

Evaluated for origin test up to 1462117

openshift-bot · 2017-04-10T23:13:29Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/148/) (Base Commit: 5e305ac) (Extended Tests: core(builds))

openshift-bot · 2017-04-11T00:36:09Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/683/) (Base Commit: 445ae00)

bparees · 2017-04-11T01:57:40Z

[merge]

bparees · 2017-04-11T14:28:16Z

flake #13712
[merge]

bparees · 2017-04-11T23:00:30Z

dnf flake [merge]

openshift-bot · 2017-04-11T23:05:23Z

Evaluated for origin merge up to 1462117

openshift-bot · 2017-04-11T23:21:43Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/320/) (Base Commit: 15941b9) (Image: devenv-rhel7_6134)

bparees force-pushed the build_events branch from d5ffae6 to be1c042 Compare April 6, 2017 19:50

bparees force-pushed the build_events branch 2 times, most recently from 7cc6f3f to 9983952 Compare April 6, 2017 21:57

bparees added the needs-api-review label Apr 7, 2017

bparees force-pushed the build_events branch from 38133a8 to dd2ee3d Compare April 7, 2017 18:30

smarterclayton reviewed Apr 7, 2017

View reviewed changes

bparees force-pushed the build_events branch 4 times, most recently from 1254da7 to 63a3183 Compare April 10, 2017 02:08

bparees force-pushed the build_events branch 3 times, most recently from da6d87c to 3ce38a3 Compare April 10, 2017 19:29

coreydaley reviewed Apr 10, 2017

View reviewed changes

csrwng reviewed Apr 10, 2017

View reviewed changes

bparees force-pushed the build_events branch 2 times, most recently from d1ec407 to 3f9c4b0 Compare April 10, 2017 19:48

smarterclayton suggested changes Apr 10, 2017

View reviewed changes

smarterclayton added the api-approved label Apr 10, 2017

smarterclayton removed the needs-api-review label Apr 10, 2017

bparees force-pushed the build_events branch from 3f9c4b0 to c353381 Compare April 10, 2017 20:31

generate build state events

1462117

bparees force-pushed the build_events branch from c353381 to 1462117 Compare April 10, 2017 21:22

bparees changed the title ~~[DO_NOT_MERGE] generate build state events~~ generate build state events Apr 10, 2017

openshift-bot merged commit b01ec19 into openshift:master Apr 12, 2017

bparees deleted the build_events branch April 17, 2017 21:09

generate build state events #13660

generate build state events #13660

Conversation

bparees commented Apr 6, 2017

bparees commented Apr 6, 2017

bparees commented Apr 7, 2017

bparees commented Apr 7, 2017

bparees commented Apr 7, 2017

bparees commented Apr 7, 2017 via email

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

jwforres commented Apr 10, 2017

bparees commented Apr 10, 2017

jwforres commented Apr 10, 2017 via email

bparees commented Apr 10, 2017

bparees commented Apr 10, 2017

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

smarterclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

openshift-bot commented Apr 11, 2017

bparees commented Apr 11, 2017

bparees commented Apr 11, 2017

bparees commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017 • edited Loading

openshift-bot commented Apr 11, 2017 •

edited

Loading