Stabilize & simplify build log stream

[mgoltzsche]

Summary

I propose to refactor the pkg/logs package because its current interface and implementation comes with some problems:

• it requires the caller to implement a LogWriter interface with methods that accept two channels (one for the log stream and one for an error). This is cumbersome and error-prone for callers to implement (can cause concurrency issues + deadlocks potentially).
• LogWriter methods are called from within another goroutine (in case of JXUI this seems to be the reason for a randomly occurring panic when flushing the log stream - currently simply recovered to avoid a complete crash)
• when loading logs from buckets they are loaded into memory before they are actually streamed - causing unnecessarily high memory consumption and, when streamed concurrently, potential crashes due to memory exhaustion.
• the LogWriter interface mixes multiple concerns: a) writing logs, b) obfuscating secrets in logs.

Steps to reproduce the behavior

Look at

• the logs package: https://github.com/jenkins-x/jx/blob/v2.1.95/pkg/logs/tekton_logging.go
• the jx get build logs cmd impl (caller): https://github.com/jenkins-x/jx/blob/v2.1.95/pkg/cmd/get/get_build_logs.go#L189

The JXUI log stream impl is similar to the latter and does not crash in such a case simply because the panic gets recovered now.

Expected behavior

The logging interface should be simple, reliable and not involve the caller in the selection of the underlying log storage which depends on jx' internal state and should not be the caller's concern.

Therefore I propose to simplify the logging interface as follows:

type PipelineActivitySelector struct {
  Owner string
  Repository string
  Branch string
  Context string
  Build string
}
type BuildLogs interface {
  Stream(ctx context.Context, selector PipelineActivitySelector) <-chan LogLine
  Err() error
}

(Secret obfuscation could be implemented separately and/or as a decorator.)
The new interface could be used as follows:

for line := range buildLogs.Stream(ctx, activitySelector) {
  // obfuscate / write log line to destination
}
if err := buildLogs.Err(); err != nil {
  return err
}

Though this is a breaking change which needs to be aligned with all callers/users of the package.

Actual behavior

Randomly occuring concurrency issues like a panic when flushing a shared writer within a LogWriter impl and potential deadlocks, lack of separation of concerns, redundant and cumbersome code in callers - for instance https://github.com/jenkins-x/jx/blob/v2.1.95/pkg/cmd/get/get_build_logs.go#L189

Jx version

The output of jx version is:

Version        2.1.79
Commit         9a5cb37
Build date     2020-06-22T16:31:09Z
Go version     1.13.8
Git tree state clean

Diagnostic information

The output of jx diagnose version is:

Running in namespace: jx
Version        2.1.79
Commit         9a5cb37
Build date     2020-06-22T16:31:09Z
Go version     1.13.8
Git tree state clean
NAME                          VERSION
Kubernetes cluster            v1.14.10-gke.36
kubectl (installed in JX_BIN) v1.13.2
helm client                   2.12.2
git                           2.17.1
Operating System              Ubuntu 18.04.4 LTS

Kubernetes cluster

v1.14.10-gke.36

Though when we reproduced the panic in JXUI previously it was reliably/often happening on an EKS cluster using the same jx version.

Kubectl version

The output of kubectl version --client is:

Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.8", GitCommit:"ec6eb119b81be488b030e849b9e64fda4caaf33c", GitTreeState:"clean", BuildDate:"2020-03-12T21:00:06Z", GoVersion:"go1.13.8", Compiler:"gc", Platform:"linux/amd64"}

Operating system / Environment

Ubuntu 18.04

[mgoltzsche]

I removed two problems from the list above because this is not required now and should be considered in a build log stream redesign (see #6759):

• It lets the caller decide where to obtain the logs from (bucket or pod; all callers (jx get build logs, JXUI) implement the decision in the same way) - this logic should be moved into jx' logs package.
• it does not support Go's context and therefore does not support interrupting the log stream before it finished.

Therefore the interface looks as follows now (error handling changed + log destination handled in caller's goroutine):

type TektonLogs interface {
  GetRunningBuildLogs(pa *v1.PipelineActivity, buildName string, noWaitForRuns bool) <-chan LogLine
  StreamPipelinePersistentLogs(logsURL string, authSvc auth.ConfigService) <-chan LogLine
  Err() error
}