-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add namespace and name into the CRI pod log directory #74441
Conversation
1b4ac53
to
1d874aa
Compare
1d874aa
to
3bbad66
Compare
/test pull-kubernetes-integration |
a7de99f
to
3db45d4
Compare
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Just rebased. Reapply LGTM based on #74441 (comment) |
This introduced backwards incompatible change, given the limit on names (253 coming from
The fact this wasn't caught earlier is we're missing a test creating a pod with maximum allowed name. |
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.
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.
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]>
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]>
For #73503.
This PR:
Changed pod log directory from
/var/log/pods/UID
to/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.ContainerStatus
calls. So if people do in-place upgrade, the log stats for existing containers will become empty.RunPodSandbox
instead ofCreateContainerRequest
, the container log will be in the old pod log directory. In this case:/var/log/containers
to/var/log/pods/NAMESPACE_NAME_UID
won't work, because the container log is actually in/var/log/pods/UID
.SandboxConfig
in theCreateContainerRequest
instead of caching and using the one passed inRunPodSandbox.
Dockershim is handling this correctly, and we'll changecontainerd
to 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