tail build logs into build object #15181

bparees · 2017-07-13T04:11:49Z

Adds the last 5 lines of the build log to the build object, when the build pod reports failure.

bparees · 2017-07-13T04:38:20Z

[test]

bparees · 2017-07-13T04:38:30Z

[testextended][extended:core(builds)]

bparees · 2017-07-13T04:39:07Z

/cc @smarterclayton

bparees · 2017-07-13T12:34:50Z

@smarterclayton do you want the LogTail output to be part of describe for a build?

bparees · 2017-07-13T13:54:59Z

(added LogTail to describer output)

bparees · 2017-07-13T14:50:53Z

@openshift/devex ptal

bparees · 2017-07-13T14:52:05Z

also @smarterclayton this is going to be something else that will be broken/annoying when we move to init containers....

bparees · 2017-07-13T16:03:54Z

flake #10773
flake #12072
[test]

smarterclayton · 2017-07-13T17:04:07Z

pkg/build/apis/build/types.go

@@ -304,6 +304,9 @@ type BuildStatus struct {
 	// including start time, duration (in milliseconds), and the steps that
 	// occured within each stage.
 	Stages []StageInfo
+
+	// LogTail is the last few lines of the build log.  This value is only set for builds that failed.
+	LogTail *string


discussed with jordan, let's call this excerpt or outputExcerpt. No guarantee there is a log. Also, not a pointer to string, just do omit empty and a normal string. Pointer to string for this sort of field is not necessary.

ok. also @csrwng suggested truncating the beginning instead of the end of the line, thoughts on that? I tend to agree the end might contain the more useful information (vs the timestamp at the beginning).

smarterclayton · 2017-07-13T19:31:58Z

Hrm, the beginning gets ragged too. Would probably middle elide, and we can go to 120 char or something

…

On Thu, Jul 13, 2017 at 3:08 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/testextended Running ( https://ci.openshift.redhat.com/jenkins/job/test_pull_ request_origin_extended/847/) (Base Commit: aa13700 <aa13700>) (PR Branch Commit: ff64afb <ff64afb>) (Extended Tests: core(builds)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxv-dTZBEmUyDxUFLYv7LLvszJRbks5sNmsZgaJpZM4OWevu> .

bparees · 2017-07-13T19:33:54Z

Hrm, the beginning gets ragged too. Would probably middle elide, and we can go to 120 char or something

yeah i considered the ragged beginning problem too. middle it is. (compromise!)

bparees · 2017-07-13T19:43:19Z

pkg/build/controller/build/build_controller.go

+					middle := len(line) / 2
+					excess := (len(line) - 117) / 2
+					excerpt[i] = line[0:middle-excess] + "..." + line[middle+excess:]
+				}


this was fun to write but someone please check my work.

just excerpt[i] = line[:58] + "..." + line[len(line)-59:] ?

bparees · 2017-07-13T23:50:56Z

flake #15024

jim-minter · 2017-07-14T08:38:29Z

pkg/build/controller/build/build_controller.go

@@ -258,15 +259,19 @@ func shouldIgnore(build *buildapi.Build) bool {
 	}

 	// If a build is in a terminal state, ignore it; unless it is in a succeeded or failed
-	// state and its completion time is not set, then we should at least attempt to set its
-	// completion time if possible.
+	// state and its completion time or outputexcerpt is not set, then we should at least attempt to set its


Is it actually the case that we enter here twice, once on becoming aware that the build has failed and a second time when the excerpt text becomes available? (and that the order of those events is not reliable?)

If that's not the case (which I hope), then to avoid complicating matters, if possible I think we should keep the gating condition as simple as possible - e.g. build.Status.CompletionTimestamp == nil only - and the controller should set the OutputExcerpt as part of the update that sets CompletionTimestamp.

I would very much like to see the builder pod no longer updating Status.Phase, so that we can factor out this special case - avoiding complicating it in the meantime is a plus.

jim-minter · 2017-07-14T08:39:41Z

pkg/build/controller/build/build_controller.go

+		msg := pod.Status.ContainerStatuses[0].State.Terminated.Message
+		if len(msg) != 0 {
+			parts := strings.Split(msg, "\n")
+			// 6 will get us 5 lines because the last entry in parts is an empty string.


well, if the message is \n-terminated, that is - could do strings.Split(strings.TrimRight(msg, "\n"), "\n")

jim-minter · 2017-07-14T08:40:21Z

pkg/build/controller/build/build_controller.go

+		if len(msg) != 0 {
+			parts := strings.Split(msg, "\n")
+			// 6 will get us 5 lines because the last entry in parts is an empty string.
+			excerptLength := 6


I think 6 (or whatever) should be a named const.

jim-minter · 2017-07-14T08:44:18Z

pkg/build/controller/build/build_controller.go

+					middle := len(line) / 2
+					excess := (len(line) - 117) / 2
+					excerpt[i] = line[0:middle-excess] + "..." + line[middle+excess:]
+				}


just excerpt[i] = line[:58] + "..." + line[len(line)-59:] ?

jim-minter · 2017-07-14T08:53:27Z

pkg/build/controller/build/build_controller.go

+	if pod != nil && len(pod.Status.ContainerStatuses) != 0 && pod.Status.ContainerStatuses[0].State.Terminated != nil {
+		msg := pod.Status.ContainerStatuses[0].State.Terminated.Message
+		if len(msg) != 0 {
+			parts := strings.Split(msg, "\n")


I think this is a little on the baroque side. In our case Message is already bounded to min(2KB, 80 lines) (see readLastStringFromContainerLogs). Surely doing nothing more, or snipping the last n bytes or lines will do?

Since this is going to end up in the web console, we need it more cleaned up than what is already being done. Ensuring we have full lines (no starting in the middle of a line) is important in terms of confusing users, i think.

(and 80 lines is going to be too much)

bparees · 2017-07-14T16:20:28Z

just excerpt[i] = line[:58] + "..." + line[len(line)-59:] ?

i knew there's a reason we keep you around... :)

updated.

bparees · 2017-07-14T16:21:16Z

(and i forgot to do it in a separate commit, sorry. habits.)

smarterclayton

A few minor things then lgtm and API approved

smarterclayton · 2017-07-15T01:39:36Z

pkg/build/apis/build/types.go

@@ -90,6 +90,9 @@ const (
 	// if the buildconfig does not specify a value.  This only applies to buildconfigs created
 	// via the new group api resource, not the legacy resource.
 	DefaultFailedBuildsHistoryLimit = int32(5)
+
+	// ExcerptLength is the maximum length of the LogSnippet on a build.
+	ExcerptLength = 5


I don't really like this as a constant that people depend on. Would just hide it in the build controller code

smarterclayton · 2017-07-15T01:40:44Z

pkg/build/controller/build/build_controller.go

@@ -834,4 +837,25 @@ func setBuildCompletionTimestampAndDuration(build *buildapi.Build, podStartTime
 		update.setStartTime(*startTime)
 	}
 	update.setDuration(now.Rfc3339Copy().Time.Sub(startTime.Rfc3339Copy().Time))
+
+	if pod != nil && len(pod.Status.ContainerStatuses) != 0 && pod.Status.ContainerStatuses[0].State.Terminated != nil {


Better way for identifying which container status to grab in the future, fine now

bparees · 2017-07-15T03:10:34Z

constant moved, case lowered.

smarterclayton · 2017-07-15T16:25:16Z

Regen open API (use make update-api) squash and I'll do a final pass and tag

bparees · 2017-07-15T22:00:11Z

@smarterclayton done

smarterclayton · 2017-07-15T22:15:19Z

[merge]

bparees · 2017-07-15T22:30:37Z

[merge][severity:blocker] Ben Parees | OpenShift

…

On Jul 15, 2017 18:23, "OpenShift Bot" ***@***.***> wrote: continuous-integration/openshift-jenkins/merge FAILURE ( https://ci.openshift.redhat.com/jenkins/job/merge_pull_ request_origin/1291/) (Base Commit: e1d18a3 <e1d18a3>) (PR Branch Commit: 77d441b <77d441b> ) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3qq3Pnvy8KkVdFpz6Puuec-m9lJnks5sOTvagaJpZM4OWevu> .

smarterclayton · 2017-07-16T00:56:17Z

Verify is still failing

bparees · 2017-07-16T01:34:22Z

weird, i ran a full make update. running hack/update-generated-swagger-spec.sh seems to have picked up the change now though. maybe a generating/compiling order of operations issue?

openshift-bot · 2017-07-16T01:39:31Z

Evaluated for origin testextended up to d2d4121

openshift-bot · 2017-07-16T01:39:42Z

continuous-integration/openshift-jenkins/testextended Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/857/) (Base Commit: 9dd6aa8) (PR Branch Commit: d2d4121) (Extended Tests: core(builds))

openshift-bot · 2017-07-16T01:39:42Z

Evaluated for origin test up to d2d4121

openshift-bot · 2017-07-16T03:32:07Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3187/) (Base Commit: 9dd6aa8) (PR Branch Commit: d2d4121)

openshift-bot · 2017-07-16T03:39:20Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 1

bparees · 2017-07-16T03:58:12Z

flake #13977
[merge]

openshift-bot · 2017-07-16T04:07:19Z

Evaluated for origin merge up to d2d4121

bparees force-pushed the build_logs branch 3 times, most recently from 74bb541 to 7c7096c Compare July 13, 2017 04:37

bparees changed the title ~~tail build logs into build object~~ [DO_NOT_MERGE] tail build logs into build object Jul 13, 2017

bparees added the needs-api-review label Jul 13, 2017

bparees force-pushed the build_logs branch 2 times, most recently from 8e03545 to b94d9f5 Compare July 13, 2017 13:54

bparees changed the title ~~[DO_NOT_MERGE] tail build logs into build object~~ tail build logs into build object Jul 13, 2017

smarterclayton reviewed Jul 13, 2017

View reviewed changes

bparees force-pushed the build_logs branch from 22369d6 to ff64afb Compare July 13, 2017 19:04

openshift deleted a comment from openshift-bot Jul 13, 2017

bparees force-pushed the build_logs branch from ff64afb to 8ca2894 Compare July 13, 2017 19:42

bparees commented Jul 13, 2017

View reviewed changes

bparees force-pushed the build_logs branch from 8ca2894 to 07862c3 Compare July 13, 2017 23:50

openshift deleted a comment from openshift-bot Jul 14, 2017

bparees force-pushed the build_logs branch from 07862c3 to cdda2bb Compare July 14, 2017 04:16

jim-minter suggested changes Jul 14, 2017

View reviewed changes

bparees force-pushed the build_logs branch from cdda2bb to 963102a Compare July 14, 2017 16:20

bparees force-pushed the build_logs branch from 9a886f3 to 0333762 Compare July 14, 2017 21:01

smarterclayton reviewed Jul 15, 2017

View reviewed changes

bparees force-pushed the build_logs branch from 50cabc9 to 77d441b Compare July 15, 2017 21:54

tail build logs into build object

d2d4121

bparees force-pushed the build_logs branch from 77d441b to d2d4121 Compare July 16, 2017 01:33

openshift-bot merged commit 41ac679 into openshift:master Jul 16, 2017

bparees deleted the build_logs branch July 19, 2017 02:16

tail build logs into build object #15181

tail build logs into build object #15181

Conversation

bparees commented Jul 13, 2017 • edited Loading

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

bparees commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 13, 2017 via email

bparees commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 14, 2017

bparees commented Jul 14, 2017

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

bparees commented Jul 15, 2017

smarterclayton commented Jul 15, 2017

bparees commented Jul 15, 2017

smarterclayton commented Jul 15, 2017

bparees commented Jul 15, 2017 via email

smarterclayton commented Jul 16, 2017

bparees commented Jul 16, 2017

openshift-bot commented Jul 16, 2017

openshift-bot commented Jul 16, 2017

openshift-bot commented Jul 16, 2017

openshift-bot commented Jul 16, 2017

openshift-bot commented Jul 16, 2017 • edited Loading

bparees commented Jul 16, 2017

openshift-bot commented Jul 16, 2017

bparees commented Jul 13, 2017 •

edited

Loading

openshift-bot commented Jul 16, 2017 •

edited

Loading