kube: Add support for podman pod logs.
#11427
Conversation
|
/assign @baude since he created original issue. |
flouthoc
force-pushed
the
kube-pod-logs
branch
3 times, most recently
from
06d4389 to
d6ecd06
Compare
cmd/podman/pods/logs.go
Outdated
| flags.BoolVar(&logsOptions.Details, "details", false, "Show extra details provided to the logs") | ||
| flags.BoolVarP(&logsOptions.Follow, "follow", "f", false, "Follow log output. The default is false") | ||
|
|
||
| containerNameFlag := "containername" |
There was a problem hiding this comment.
I think it is better to call it container and also to make it accept container ids.
There was a problem hiding this comment.
@Luap99 Renamed flag to container thanks :) and it already accepts ID or Name
flouthoc
force-pushed
the
kube-pod-logs
branch
from
d6ecd06 to
3d3031c
Compare
flouthoc
force-pushed
the
kube-pod-logs
branch
from
3d3031c to
4009af1
Compare
flouthoc
force-pushed
the
kube-pod-logs
branch
from
4009af1 to
90775f3
Compare
flouthoc
force-pushed
the
kube-pod-logs
branch
from
90775f3 to
9014b3e
Compare
flouthoc
force-pushed
the
kube-pod-logs
branch
3 times, most recently
from
23cd16d to
a39e0d9
Compare
|
|
||
| #### **--container**, **-c** | ||
|
|
||
| By default `podman pod logs` retrives logs for all the containers available within the pod differentiate by field `container`. However there are use-cases where user would want to limit the log stream only to a particular container of a pod for such cases `-c` can be used like `podman pod logs -c ctrNameorID podname`. |
There was a problem hiding this comment.
Why wouldn't I just use podman logs CID?, would podman pod logs give me different data?
There was a problem hiding this comment.
It majorly for mainlining interface parity between kubectl pod logs and podman pod logs , since kubectl would not print logs if there are multiple containers in a pod it rather asks users to provide container using a -c flag.
openshift-ci
bot
added
the
approved
flouthoc
force-pushed
the
kube-pod-logs
branch
from
a39e0d9 to
a0a9292
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan 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 |
Following PR adds support for `kubectl` like `pod logs` to podman. Usage `podman pod logs <podIDorName` gives a stream of logs for all the containers within the pod with **containername** as a field. Just like **`kubectl`** also supports `podman pod logs -c ctrIDorName podIDorName` to limit the log stream to any of the specificied container which belongs to pod. Signed-off-by: Aditya Rajan <[email protected]>
flouthoc
force-pushed
the
kube-pod-logs
branch
from
a0a9292 to
11fc0e5
Compare
|
LGTM |
|
/lgtm |
| logs.WaitWithDefaultTimeout() | ||
| Expect(logs).Should(Exit(0)) | ||
| Expect(logs.OutputToString()).To(ContainSubstring("hello world")) | ||
| }) |
There was a problem hiding this comment.
Apologies for commenting so late. I had it on my to-review list but didn't manage before.
I want to ask for more tests in a follow-up PR. There's a couple of things in the tests that I think need improvement:
- We need more tests and exercise all flags of
podman pod logs(e.g., --follow). The coverage at the moment is low, so the risk of regressions is high. - What are the exact semantics of --follow? Shall we follow over the entire lifecycle of the pod (currently we do that) or shall we follow all containers except the infra one? The difference is that --follow will not return unless the pod is stopped. IMHO we should NOT follow the infra container.
- We need to improve testing the output. Currently, we are only checking for substrings but I don't think that's sufficient to catch future regressions. We need to look closer and match exact output. Since we can get the IDs of containers, we should be able to construct an exact string match, not a substring one.
- Last but not least, we have two log drivers (k8s-file and journald) and I think we should tests both as we do in the container logs tests. I suggest moving the tests away from
play_kubeand into a new filepod_logs_test.go.
Again sorry for not reviewing earlier.
There was a problem hiding this comment.
@vrothberg Yes i think we should address all these point in few more test cases, I'll add different systems tests addressing these points.
github-actions
bot
added
the
locked - please file new issue/PR
Following PR adds support for
kubectllikepod logsto podman.Usage
podman pod logs <podIDorName>gives a stream of logs for allthe containers within the pod with containername as a field.
Just like
kubectlalso supportspodman pod logs -c <ctrIDorName> <podIDorName>to limit the log stream to any of the specificied container which belongs to pod.