Add namespace and name into the CRI pod log directory#74441
Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom Mar 9, 2019
Merged
Add namespace and name into the CRI pod log directory#74441k8s-ci-robot merged 2 commits intokubernetes:masterfrom
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
1b4ac53 to
1d874aa
Compare
1d874aa to
3bbad66
Compare
Member
Author
|
/test pull-kubernetes-integration |
This was referenced Feb 23, 2019
a7de99f to
3db45d4
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Random-Liu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
New changes are detected. LGTM label has been removed. |
Member
Author
|
Just rebased. Reapply LGTM based on #74441 (comment) |
Contributor
This introduced backwards incompatible change, given the limit on names (253 coming from ) which applies both to namespace and name we end up with filename significantly exceeding 255 which is a fact in majority of file systems (https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits). /cc @smarterclayton @bgrant0607 @thockinThe fact this wasn't caught earlier is we're missing a test creating a pod with maximum allowed name. |
tredman
added a commit
to honeycombio/honeycomb-kubernetes-agent
that referenced
this pull request
Jun 12, 2019
Our agent is broken on K8S cluster versions published after this change: kubernetes/kubernetes#74441 Adding yet another log path bandaid to the list of known patterns. It works, but I'd like to find a way to stop doing this. Unfortunately the current K8S API doesn't provide a way to get the log *path* for a pod, just the log. We could start streaming k8s logs from the API, but that's a significant amount of work and we don't have time to do that right now. Here's hoping the log pattern doesn't change again in the next 6 months.
mollystamos123
pushed a commit
to honeycombio/honeycomb-kubernetes-agent
that referenced
this pull request
Jun 21, 2019
Our agent is broken on K8S cluster versions published after this change: kubernetes/kubernetes#74441 Adding yet another log path bandaid to the list of known patterns. It works, but I'd like to find a way to stop doing this. Unfortunately the current K8S API doesn't provide a way to get the log *path* for a pod, just the log. We could start streaming k8s logs from the API, but that's a significant amount of work and we don't have time to do that right now. Here's hoping the log pattern doesn't change again in the next 6 months.
haircommander
added a commit
to haircommander/kubernetes
that referenced
this pull request
Feb 15, 2022
in kubernetes#74441, the namespace and name were added to the pod log location. However, cAdvisor stats provider wasn't correspondingly updated. since CRI-O uses cAdvisor stats provider by default, despite being a CRI implementation, eviction with ephemeral storage and container logs doesn't work as expected, until now! Signed-off-by: Peter Hunt <[email protected]>
muyangren2
pushed a commit
to muyangren2/kubernetes
that referenced
this pull request
Jul 14, 2022
in kubernetes#74441, the namespace and name were added to the pod log location. However, cAdvisor stats provider wasn't correspondingly updated. since CRI-O uses cAdvisor stats provider by default, despite being a CRI implementation, eviction with ephemeral storage and container logs doesn't work as expected, until now! Signed-off-by: Peter Hunt <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For #73503.
This PR:
Changed pod log directory from
/var/log/pods/UIDto/var/log/pods/NAMESPACE_NAME_UID.ContainerStatus.GetLogPath()is used to get the actual log path from the container runtime. This works for both containers with old log path and new log path.ContainerStatuscalls. So if people do in-place upgrade, the log stats for existing containers will become empty.RunPodSandboxinstead ofCreateContainerRequest, the container log will be in the old pod log directory. In this case:/var/log/containersto/var/log/pods/NAMESPACE_NAME_UIDwon't work, because the container log is actually in/var/log/pods/UID.SandboxConfigin theCreateContainerRequestinstead of caching and using the one passed inRunPodSandbox.Dockershim is handling this correctly, and we'll changecontainerdto do this as well. /cc @mrunalpInclude container log inode usage as part of pod ephemeral storage for CRI stats provider. We were not doing that because cadvisor doesn't return accurate container log inode usage, but we do have that for CRI container runtimes. /cc @dashpole @jingxu97
Include fsstats of log files under the pod log directory. Because we don't have log files under the pod log directory today, so this won't affect any existing functionality. Some sandbox runtimes, e.g. kata and gvisor, may have some pod level logs. This change makes it possible them to fsstats of those logs as part of pod ephemeral storage.
/cc @kubernetes/sig-node-pr-reviews