Add script to tail logs for pipelinerun and taskruns #336
Conversation
knative-prow-robot
added
size/XXL
|
/test pull-knative-build-pipeline-integration-tests |
test/logs/README.md
Outdated
| There is a "gotcha" about tailing logs for init container. Logs cannot be retrieved from pod that has been shut down for a while. In this case tailing logs will return error `Unable to retrieve container logs`. | ||
|
|
||
| ```shell | ||
| go run test/logs/main.go [-n NAMESPACE] [[-pr PIPELINERUN-NAME] / [-tr TASKRUN_NAME]] [-f FILE-NAME] |
There was a problem hiding this comment.
Either -pr or -tr is required right ? 🤔
There was a problem hiding this comment.
Ah yes. Thanks for catching that
test/logs/helpers/taskrun_logs.go
Outdated
| return fmt.Errorf("waiting for container: %v", err) | ||
| } | ||
|
|
||
| container := pod.Status.InitContainerStatuses[i] |
There was a problem hiding this comment.
hm we'll have to remember to update this once knative/build#470 lands!
since landing knative/build#470 could break this tool, what do you think about adding some kind of super basic sanity check test that will verify the tool keeps working? otherwise we could easily forget about it (if we're not using it regularly, which I bet we will be!)
There was a problem hiding this comment.
i also wonder if the shifting implementation could make it a good idea to abstract the init container based portion of this logic into a function or package that we can swap out when we land knative/build#470
There was a problem hiding this comment.
I think this should work after build PR lands. TaskRun status hold podname and that is used for tracking pod logs.
test/logs/main.go
Outdated
|
|
||
| _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" | ||
|
|
||
| tlogs "github.com/knative/build-pipeline/test/logs/helpers" |
There was a problem hiding this comment.
can we call the package something more specific than helpers? I think helpers is one of those package names that is hard to reason about and makes it difficult what should go there and what shouldn't (see "bad package names" at https://blog.golang.org/package-names)
The fact that you've imported this as tlogs I feel is a hint that maybe we don't even need the helpers package? We could have everything exist directly in test/logs
Or we could break helpers up into more specific packages, e.g. test/logs/pods, test/logs/pipelinerun
There was a problem hiding this comment.
Sure makes sense. I will refactor to include all logs in one package for now.
| @@ -6,6 +6,7 @@ | |||
| - [How do I run a Pipeline?](#running-a-pipeline) | |||
| - [How do I run a Task on its own?](#running-a-task) | |||
| - [How do I troubleshoot a PipelineRun?](#troubleshooting) | |||
| - [How do I follow logs](../test/logs/README.md) | |||
There was a problem hiding this comment.
I guess once we have #107 implemented this will change a bit 🤔
There was a problem hiding this comment.
shashwathi
force-pushed
the
tail-logs
branch
2 times, most recently
from
2df1219
to
cefe0d5
Compare
|
@bobcatfish @vdemeester : I have addressed your comments. Ready for another review |
| out = os.Stdout | ||
|
|
||
| if (*pr == "" && *tr == "") || (*pr != "" && *tr != "") { | ||
| errorMsg(fmt.Sprintf("Usage: %s [-n NAMESPACE] [-pr PIPELINERUN-NAME] / [-tr TASKRUN_NAME] [-f FILE-NAME]", os.Args[0]), nil, out) |
There was a problem hiding this comment.
nit: we may want to print a message "you can't have both -pr/-tr flags or non" maybe ?
test/logs/pipelinerun/logs.go
Outdated
| "k8s.io/client-go/rest" | ||
| ) | ||
|
|
||
| //tailPrLogs tails the logs for pipeline run. |
test/logs/taskrun/logs.go
Outdated
| "k8s.io/client-go/rest" | ||
| ) | ||
|
|
||
| func TailLogs(ctx context.Context, cfg *rest.Config, trName, namespace string, out io.Writer) error { |
There was a problem hiding this comment.
a godoc would be good here (like on pipelineRun.TailLogs)
- supports `-f` flag. Takes name of a file to create to dump logs - supports `-n` flag for namepsace. Default namespace is `default` - supports `-pr` to tail logs for pipelinerun and all sub taskruns - supports `-tr` to tail logs for taskrun only - Add note to help cli user understand about pr/tr option required - Enhance godoc thanks to @vdemeester
shashwathi
force-pushed
the
tail-logs
branch
from
cefe0d5
to
8074f29
Compare
|
@vdemeester I have addressed your comments. Let me know |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shashwathi, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| ```shell | ||
| go run test/logs/main.go -tr my-taskrun | ||
| ``` |
There was a problem hiding this comment.
i'm just seeing this now, v cool! thanks for adding this @shashwathi ! 🎉
Script to follow logs.
-fflag. Takes name of a file to create to dump logs-nflag for namepsace. Default namespace isdefault-prto tail logs for pipelinerun and all sub taskruns-trto tail logs for taskrun onlyI have been using this locally to help my development. It does not have any tests and it uses
Buildlogs script as the base with minor improvements.