CRI: proposal for managing container stdout/stderr streams #34376
Conversation
|
/cc @kubernetes/sig-node |
yujuhong
added
release-note-none
k8s-github-robot
added
kind/design
|
Jenkins Kubemark GCE e2e failed for commit 18e7a0d. Full PR test history. The magic incantation to run this job again is |
|
Jenkins GCI GCE e2e failed for commit 18e7a0d. Full PR test history. The magic incantation to run this job again is |
| and ends with a newline. | ||
|
|
||
| ``` | ||
| 2016-10-06T00:17:09.669794202Z The content of the log entry |
There was a problem hiding this comment.
Do we need to distinguish between stdout and stderr?
There was a problem hiding this comment.
For the kubectl logs feature it doesn't matter, for log aggregators it definitely could
There was a problem hiding this comment.
Yes, kubectl logs looks at combined output, but we can definitely distinguish the two streams for log aggregation. I'll modify the proposal to include that.
There was a problem hiding this comment.
How is this supposed to interact with the container-app being attached to a TTY? It is my understanding that you can't actually get split stdout/stderr and a TTY at the same time, see moby/moby#19696.
There was a problem hiding this comment.
Added the stream type.
@lucab I think that's a limitation of the underlying implementation. For now, maybe we can do what docker does and report only stdout?
|
|
||
|
|
||
| 1. **Allow log collectors to integrate with kubernetes without knowledge of | ||
| the underlying container runtime.** |
There was a problem hiding this comment.
Is this a requirement? It's certainly nice to have, but I would have phrased this much weaker:
"Allow log collectors to integrate with Kubernetes across different container run times while preserving efficient storage and retrieval"
There was a problem hiding this comment.
I am ok with a making it a wekaker statement. I thought about categorizing some of them as nice-haves, but was worried that it'd complicate the proposal. It's an important factor nonetheless.
| 2. **Allow kubelet to manage the lifecycle of the logs to pave the way for | ||
| better disk management in the future.** This implies that the lifecycle | ||
| of containers and their logs need to be decoupled. | ||
| 3. **Provide ways for CRI-compliant runtimes to support all existing logging |
There was a problem hiding this comment.
This would probably be #1 requirement in an ordered list.
There was a problem hiding this comment.
Sure, if we need to rank them. I'd still prefer the solution that addresses all three requirements/nice-to-haves.
| mandate the logging format, assuming runtime already includes plugins for | ||
| various logging formats. Unfortunately, this is not true for existing runtimes | ||
| such as docker, which supports log retrieval only for a very limited number of | ||
| log drivers [2]. On the other hand, the downside is that we would be enforcing |
There was a problem hiding this comment.
Is this really a critical problem? If supporting more log drivers in docker is important, we can always support that.
There was a problem hiding this comment.
This is just to point out that runtime support for log drivers are far from perfect today, and it's not good enough to simply say "runtime already supports log drivers, why can we reuse that?"
| defining complex logging API. | ||
|
|
||
| There are also possibilities to implement the current proposal by imposing the | ||
| log file paths, while leveraging the runtime to access and/or manage logs. This |
There was a problem hiding this comment.
I'm not sure that this proposal makes a really strong argument against this (which I would consider the "default" path because it leverages the existing runtime support).
There was a problem hiding this comment.
There are a few other things to consider:
- Log collection should not require going through a daemon - A process like fluentd should be able to collect logs without having to rely on another daemon being up. This implies that logs must be a set of files at a known location in a format known to log aggregators.
- Rotation policies might vary by containers -- I don't think there are any runtimes now that support log rotation at any reasonable granularity, each runtime would have to independently develop that feature
- Logs persisting and being rotated after the container is removed - If a runtime is specifically only a container runtime, decoupling the lifetime of container+logs while having the runtime manage it will result in either the runtime having to have a firstclass concept of logs separate from containers (I don't think any do), or in the shim having to manage the logs itself... if the shim has to do it, then it might as well be kubelet.
I originally was in favor of having the runtimes manage everything, but if we add all of the above requirements, I think it's a good argument against it. We can question how much we actually need most of those requirements still though, since several of them are quite forward-looking.
There was a problem hiding this comment.
+1 to @euank's comment, which explains more clearly than the statement in the proposal.
TBH, I am not completely against leveraging runtime for various log operations. However,
- We are not sure what we want yet, since disk management is still in its early stages.
- Container runtimes such as docker also does not provide good support of logging yet. Even if we were to improve that (or hack docker internals in the shim), it'd take a longer time.
Instead of committing resources to do this right now, I think the current proposal gives us room to figure things out without having to develop and commit to a premature api.
There was a problem hiding this comment.
We can question how much we actually need most of those requirements still though, since several of them are quite forward-looking.
I think I did this on purpose. I wanted to see if we can try finding a solution that addresses all requirements first. If, instead, sig-node thinks that some requirements can be dropped, it's perfectly reasonable to question the requirements themselves, and convince people who are in favor of the requirements.
| the underlying container runtime.** | ||
|
|
||
| 2. **Allow kubelet to manage the lifecycle of the logs to pave the way for | ||
| better disk management in the future.** This implies that the lifecycle |
There was a problem hiding this comment.
This also implies the kubelet must be able to rotate logs, and thus must be able to work with the given format.
There was a problem hiding this comment.
Another component can be set up to rotate the log if needed. I think the proposal discusses more about the logging formats later. Let me know if you find that insufficient.
There was a problem hiding this comment.
I think this is an orthogonal concern... So long as it's not strict control.
|
Reviewed 1 of 1 files at r1. docs/proposals/kubelet-cri-logging.md, line 35 at r1 (raw file):
small typo in "Interfacej(CRI)", there's a 'j' in there that probably shouldn't be. docs/proposals/kubelet-cri-logging.md, line 116 at r1 (raw file):
|
|
|
||
| Because kubelet determines where the logs are stores and can access them | ||
| directly, this meets requirement (1). As for requirement (2), the log collector | ||
| can easily extracts all necessary metadata (e.g., pod UID, container name) from |
There was a problem hiding this comment.
..all necessary metadata... I would say this is misleading given for the openshift aggregated logging solution we consider the pod labels 'necessary' but are unable to 'access' them without a call to the api server.
There was a problem hiding this comment.
Agreed. Could we have a unique directory per pod (each being symlinked into that known path), where that directory contains a metadata file with the pod's name, id, annotations, and the same for its containers?
There was a problem hiding this comment.
I'll reword it as "basic pod metadata". I think it's reasonable to dump a metadata file in the directory. I was going to add it here, but forgot to do so earlier.
There was a problem hiding this comment.
Changed the wording, and add the future work for writing a metadata file.
|
cc @lucab, references your attach work as a path for rkt to integrate with this, and any insight you have would be appreciated! I wouldn't be surprised if this comes up in sig-node tomorrow :) |
| In the short term, the ongoing docker-CRI integration [3] will support the | ||
| proposed solution only partially by (1) creating symbolic links for kubelet | ||
| to access, but not manage the logs, and (2) add support for json format in | ||
| kubelet. A more sophisticated solution that either involves using a custom plugin |
There was a problem hiding this comment.
Is this saying for the near short term, we tolerate different logging formats than the one proposed above?
There was a problem hiding this comment.
Yes. This helps bridge the transition.
|
I am OK with this proposal. I understand all the arguments, and I think dithering any further is a waste of energy. @yujuhong do you want to make more edits, or do you think it is good? @smarterclayton are your questions advisory or a request to halt a merge? |
|
wrt streaming through the runtime: a) Can we get away with that FOR NOW? |
| as a mid-term solution. | ||
|
|
||
| For rkt, there is a pending PR to enable systemd to redirect stdout/stderr to | ||
| user-provided streams [4]. |
There was a problem hiding this comment.
A bit more context on this:
For rkt, implementation will rely on providing external file-descriptors for stdout/stderr
to applications via systemd [4]. Those streams are currently managed by a journald
sidecar, which collects stream outputs and store them in the journal file of the pod.
This will replaced by a custom sidecar which can produce logs in the format expected
by this specification and can handle clients attaching as well.
There was a problem hiding this comment.
Thanks for the explanation. Included this in the proposal.
|
LGTM. From the rkt side, we can implement this as @lucab commented, or by having some process act as a journald "remote" endpoint and have it transform logs to this format. The benefit of the latter is simply that the existing machined / journald integration would still work, but if you're using Kubernetes it's less important to have that and more important to have a generic on-node tools to work with pod logs (out of scope of this issue). Until the implementation work is done rkt side, it's likely we'll want to have a runtime-specific workaround in kubelet. |
|
|
||
| ## Goals and non-goals | ||
|
|
||
| Container Runtime Interfacej(CRI) is an ongoing project to allow container |
|
Please run hack/update-munge-docs.sh |
thockin
added
the
lgtm
|
|
||
| **Log format** | ||
|
|
||
| The runtime should decorate each log entry with a RFC 3339 timestamp prefix, |
There was a problem hiding this comment.
s/RFC 3339/RFC 3339Nano
Found this during coding.
@yujuhong
| **Log format** | ||
|
|
||
| The runtime should decorate each log entry with a RFC 3339 timestamp prefix, | ||
| the stream type (i.e., "stdout" or "stderr"), and ends with a newline. |
There was a problem hiding this comment.
What about when the container was created with at tty?
In that case, there's no way to distinguish stdout from stderr (per discussion in 32869).
If you look at docker's json-log file for a container started with -t, it will only have stdout items.
Should we specify that here, or assume a tty'd container just shouldn't produce any logs (they'll often have escape sequences and such regardless)?
cc @lucab
There was a problem hiding this comment.
I think we may want to assume that container created with tty should only generate logs with stream type "stdout". (As you mentioned, docker behaves like this)
There was a problem hiding this comment.
If we do, we should document this.
But I also think it could make sense to not support logs for that because a container with a tty is likely to emit terminal control characters or be used for read/writing large chunks of data (per @smarterclayton use-case for attach).
There was a problem hiding this comment.
I think we may want to assume that container created with tty should only generate logs with stream type "stdout". (As you mentioned, docker behaves like this)
+1. only one stream is supported because of tty.
But I also think it could make sense to not support logs for that because a container with a tty is likely to emit terminal control characters or be used for read/writing large chunks of data.
It's better to escape those terminal control characters instead of not supporting logs.
There was a problem hiding this comment.
Alternate option would be a special tty type. This could have value for log collectors being able to decide to ingress it or not, or for knowing when to play back as text to a terminal vs raw for any tool akin to kubectl logs
There was a problem hiding this comment.
FYI, I created a new issue for this: #37247
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
removed
the
lgtm
|
@k8s-bot bazel test this |
yujuhong
added
the
lgtm
|
Jenkins Bazel Build failed for commit 398c82a4142e4ec7a533ed1352ce90f973fc1180. Full PR test history. The magic incantation to run this job again is |
|
Jenkins verification failed for commit 398c82a4142e4ec7a533ed1352ce90f973fc1180. Full PR test history. The magic incantation to run this job again is |
|
Jenkins kops AWS e2e failed for commit 398c82a4142e4ec7a533ed1352ce90f973fc1180. Full PR test history. The magic incantation to run this job again is |
This scope of this proposal is limited to the stdout/stderr logs streams of the containers.
k8s-github-robot
removed
the
lgtm
yujuhong
added
the
lgtm
|
@k8s-bot e2e test this |
|
Automatic merge from submit-queue |
This scope of this proposal is limited to the stdout/stderr logs streams of the
containers.
This addresses #30709
This change is