Adding per container stats for CRI runtimes #59906

abhi · 2018-02-15T06:02:18Z

What this PR does / why we need it

This commit aims to collect per container log stats. The change was proposed as a part of #55905. The change includes change the log path from /var/pod//containername_attempt.log to /var/pod//containername/containername_attempt.log. The logs are collected by reusing volume package to collect metrics from the log path.
Fixes #55905

Special notes for your reviewer:
cc @Random-Liu

Release note:

Adding container log stats for CRI runtimes.

k8s-ci-robot · 2018-02-15T06:02:19Z

@abhi: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Random-Liu · 2018-02-15T19:03:39Z

pkg/kubelet/kuberuntime/helpers.go

@@ -207,14 +207,19 @@ func getStableKey(pod *v1.Pod, container *v1.Container) string {

 // buildContainerLogsPath builds log path for container relative to pod logs directory.
 func buildContainerLogsPath(containerName string, restartCount int) string {
-	return fmt.Sprintf("%s_%d.log", containerName, restartCount)
+	return fmt.Sprintf("%s/%s_%d.log", containerName, containerName, restartCount)


I feel like we can use restartCount as log file name for now until we have more metadata to add. Just to make the path shorter.
@mrunalp @feiskyer WDYT?

Random-Liu · 2018-02-15T19:04:48Z

pkg/kubelet/kuberuntime/helpers.go

@@ -207,14 +207,19 @@ func getStableKey(pod *v1.Pod, container *v1.Container) string {

 // buildContainerLogsPath builds log path for container relative to pod logs directory.
 func buildContainerLogsPath(containerName string, restartCount int) string {
-	return fmt.Sprintf("%s_%d.log", containerName, restartCount)
+	return fmt.Sprintf("%s/%s_%d.log", containerName, containerName, restartCount)


keep consistent filepath.Join

Random-Liu · 2018-02-15T19:05:06Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -190,6 +190,11 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai
 	}

 	command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
+	logDir := fmt.Sprintf("%s/%s", buildPodLogsDirectory(pod.UID), container.Name)


Use your helper function. BuildContainerLogsDirectory

Remove this directory when remove container.

good point. Will do

Random-Liu · 2018-02-15T19:24:45Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

@@ -61,7 +61,7 @@ func TestRemoveContainer(t *testing.T) {
 	err = m.removeContainer(containerId)
 	assert.NoError(t, err)
 	// Verify container log is removed
-	expectedContainerLogPath := filepath.Join(podLogsRootDirectory, "12345678", "foo_0.log")
+	expectedContainerLogPath := filepath.Join(podLogsRootDirectory, "12345678", "/foo/foo_0.log")


Use Join instead of /.

"foo", "0.log"

Random-Liu · 2018-02-15T19:27:52Z

pkg/kubelet/stats/cri_stats_provider.go

+	m := volume.NewMetricsDu(path)
+	logMetrics, err := m.GetMetrics()
+	if err != nil {
+		return nil


log the error.

Random-Liu · 2018-02-15T19:28:12Z

pkg/kubelet/stats/cri_stats_provider.go

@@ -423,3 +415,26 @@ func getCRICadvisorStats(ca cadvisor.Interface) (map[string]cadvisorapiv2.Contai
 	}
 	return stats, nil
 }
+
+func getContainerLogStats(path string) *statsapi.FsStats {


Add TODO, cache this in container log manager.

Random-Liu · 2018-02-15T19:40:19Z

pkg/kubelet/stats/cri_stats_provider_test.go

+
+func makeFakeLogs(sandbox *critest.FakePodSandbox, containers ...string) {
+	for _, name := range containers {
+		os.MkdirAll(fmt.Sprintf("/var/log/pods/%s/%s/", types.UID(sandbox.GetMetadata().GetUid()), name), 755)


Use your helper function.

Random-Liu · 2018-02-15T19:40:30Z

pkg/kubelet/stats/cri_stats_provider_test.go

+}
+
+func deleteFakeLogs(sandbox *critest.FakePodSandbox) {
+	os.RemoveAll(fmt.Sprintf("/var/log/pods/%s", types.UID(sandbox.GetMetadata().GetUid())))


Random-Liu · 2018-02-15T19:43:04Z

pkg/kubelet/stats/cri_stats_provider_test.go

+
+func makeFakeLogs(sandbox *critest.FakePodSandbox, containers ...string) {
+	for _, name := range containers {
+		os.MkdirAll(fmt.Sprintf("/var/log/pods/%s/%s/", types.UID(sandbox.GetMetadata().GetUid()), name), 755)


/var/log/pods is actual pod/container log directory. We should not create files there in unit test... We may ruin people's local environment...

yep went mock approach

Random-Liu · 2018-02-15T19:44:43Z

pkg/kubelet/stats/cri_stats_provider.go

@@ -423,3 +415,26 @@ func getCRICadvisorStats(ca cadvisor.Interface) (map[string]cadvisorapiv2.Contai
 	}
 	return stats, nil
 }
+
+func getContainerLogStats(path string) *statsapi.FsStats {
+	m := volume.NewMetricsDu(path)


NewMetricsDu returns an interface MetricsProvider. It should be easy to mock.
Let's use a mock instead of using actual log stats.

abhi · 2018-02-16T19:25:08Z

@Random-Liu will update the PR and address the review comments. Thanks for reviewing

k8s-ci-robot · 2018-02-17T21:32:15Z

@abhi: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Random-Liu · 2018-02-21T01:31:24Z

pkg/kubelet/stats/cri_stats_provider_test.go

-	assert.Equal(*rootFsInfo.Inodes, *actual.Logs.Inodes)
-	assert.Nil(actual.Logs.UsedBytes)
-	assert.Nil(actual.Logs.InodesUsed)
+func checkCRILogsStats(assert *assert.Assertions, actual statsapi.ContainerStats) {


Compare the value for rootFsInfo and log stats. That's the point of the fake, right? :)

Random-Liu · 2018-02-21T01:33:31Z

pkg/kubelet/stats/cri_stats_provider_test.go

+
+func makeFakeLogStats(seed int) *volume.Metrics {
+	m := &volume.Metrics{}
+	m.Used = resource.NewQuantity(int64(0), resource.BinarySI)       //seed + offsetUsage),resource.BinarySI)


? comment not removed?

Random-Liu · 2018-02-21T01:36:22Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -190,6 +190,11 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai
 	}

 	command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
+	logDir := BuildContainerLogsDirectory(kubetypes.UID(pod.UID), container.Name)


Remove the directory in removeContainerLog

This commit aims to collect per container log stats. The change was proposed as a part of kubernetes#55905. The change includes change of the log path from /var/pod/<pod uid>/containername_attempt.log to /var/pod/<pod uid>/containername/containername_attempt.log. The logs are collected by reusing volume package to collect metrics from the log path. Signed-off-by: abhi <abhi@docker.com>

Random-Liu · 2018-02-21T20:52:30Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -840,8 +845,7 @@ func (m *kubeGenericRuntimeManager) removeContainerLog(containerID string) error
 		return fmt.Errorf("failed to get container status %q: %v", containerID, err)
 	}
 	labeledInfo := getContainerInfoFromLabels(status.Labels)
-	annotatedInfo := getContainerInfoFromAnnotations(status.Annotations)
-	path := buildFullContainerLogsPath(labeledInfo.PodUID, labeledInfo.ContainerName, annotatedInfo.RestartCount)
+	path := BuildContainerLogsDirectory(labeledInfo.PodUID, labeledInfo.ContainerName)


Sorry... This is my fault... We can't remove the whole directory... There might be newer instance of container log with different restart count in this directory. Let's leave the container log directory to be deleted when pod deletion.

yep :) Cool will update

The commit contains test case modifications to test and verify changes for container log stats feature. Signed-off-by: abhi <abhi@docker.com>

Random-Liu · 2018-02-21T21:29:54Z

/lgtm
/approve

k8s-ci-robot · 2018-02-21T21:30:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhi, 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:

~~pkg/kubelet/OWNERS~~ [Random-Liu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

feiskyer

While the change is not tied to any platform, volume.MetricsProvider doesn't support windows yet. Will open a new issue for windows.

LGTM

k8s-github-robot · 2018-02-22T03:40:41Z

Automatic merge from submit-queue (batch tested with PRs 54191, 59374, 59824, 55032, 59906). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-02-27T03:07:03Z

@abhi: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Automatic merge from submit-queue (batch tested with PRs 63603, 63557, 62015). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. CRI: update documents for container logpath **What this PR does / why we need it**: The container log path has been changed from `containername_attempt#.log` to `containername/attempt#.log` in #59906. This PR updates CRI documents for it. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes # **Special notes for your reviewer**: **Release note**: ```release-note CRI: update documents for container logpath. The container log path has been changed from containername_attempt#.log to containername/attempt#.log ```

andor44 · 2018-07-11T12:23:04Z

This change broke retrieval of logs for pods that were started on a <1.10 cluster that got upgraded to 1.10, even though this has nothing to do with CRI:

k logs -f <foo>
failed to open log file "/var/log/pods/<UUID>/<container name>/<restart count>.log": open /var/log/pods/<UUID>/<container name>/<restart count>.log: no such file or directory

I'm sorry for the bluntness but how the hell can such a change get merged without any regard for backwards compatibility?

Random-Liu · 2018-07-11T19:04:01Z

@andor44 Are you using cri-o?

andor44 · 2018-07-11T19:44:09Z

No, Docker 1.12. I worded my original comment wrongly, by "has nothing to do with CRI" I meant we don't use a CRI runtime.

Random-Liu · 2018-07-11T20:12:40Z

@andor44
We made a change to avoid the backward compatibility issue, but that was only in 1.11 #63268.

When upgrading from 1.9 to 1.10, you will have this issue. I'll say this is expected, because:

We never officially support in-place Kubelet update, it is always recommended to drain the node first before upgrade.
CRI had a breaking change, and the log path change is one of the breaking changes. The CRI version is also bumped up to v1alpha2 in 1.10. https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG-1.10.md#before-upgrading

We can cherry-pick #63268 to 1.10 for this, because the fix is small. @yujuhong Are you ok with that?

But please note that in-place kubelet may break again in the future, because it is never supported. :)

yujuhong · 2018-07-11T21:36:47Z

We can cherry-pick #63268 to 1.10 for this, because the fix is small. @yujuhong Are you ok with that?

Sounds good.

feiskyer · 2018-07-12T01:45:18Z

We can cherry-pick #63268 to 1.10 for this, because the fix is small.

LGTM. Created #66096 for this.

andor44 · 2018-07-12T07:30:27Z

@Random-Liu understandable, but it seems that point 1 is undocumented, or at least I can't find anything in the docs that would indicate that the kubelet does not support in-place upgrades. I know that a lack of warning does not indicate support, but lots of online guides and how-tos tell you to do just that. Given that other components deal with in-place upgrades just fine, it is not a big mental leap to assume kubelet does too. If the kubelet really doesn't support it and such breaks are to be expected it should be well documented IMO.

FWIW, the cluster in question has been upgraded from 1.5 all the way to 1.10 over the past 2 years or so, and this is the first time we ran into some serious incompatibility/issue.

Issue asking this very question, no answer: Is it necessary to drain pods when upgrading kubelet on woker node #60556
Stackoverflow question asking about this quotes a doc page, but I can't find said quote on the page.
CLI reference page, no mention: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/
CoreOS Tectonic docs, implies this should work: https://coreos.com/tectonic/docs/latest/reference/kubelet-upgrade.html
Kubeadm docs, nothing: https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-upgrade/
Random blogpost that's GKE specific, this is the only place that mentions draining: https://medium.com/retailmenot-engineering/zero-downtime-kubernetes-cluster-upgrades-aab4cac943d2

lucaim · 2018-07-12T08:34:04Z

I have to say that I agree with @andor44 here. We probably made assumption based, at least partially, on the lack of documentation or the presence of the contrary and our experience related to that. I knew that on GKE for example the node is drained but this could have been for a cloud specific issue/best practice. For us, on bare metal, it worked all the times. I kindly suggest to explicitly put this in the documentation. Thanks for the effort done in cherry picking the fix.

Random-Liu · 2018-07-13T18:28:03Z

@andor44 @lucaim For this particular issue, we should mention it in the BeforeUpgrade, which we should start doing for CRI changes. @yujuhong @feiskyer

yujuhong · 2018-07-13T18:47:34Z

@andor44 @lucaim I agree that the release note should call out issues that may lead to container restarts (or other problems among kubelet upgrade) if possible. This is our oversight and sorry for the trouble.

AFAIK, the kubelet in-place upgrade path is not tested, so these issues may not be caught and well-documented before release. Just like the stackoverflow question @andor44 mentioned, container restarts also happened in 1.7 to 1.8 upgrade as well. If you use pod manifest files on the node, they'd encounter upgrade issues even more frequently (arguably by design). Given the current situation, I'd recommend draining the node to avoid unexpected issues.

feiskyer · 2018-07-16T02:50:37Z

Agreed. Since this particular issue has been mitigated, let's address those user-facing issues clearly in the future on both PR's changelog and BeforeUpgrade part.

k8s-ci-robot requested review from dchen1107 and resouer February 15, 2018 06:02

abhi force-pushed the log_stats branch 2 times, most recently from 4674aa7 to 344721a Compare February 15, 2018 06:10

Random-Liu added this to the v1.10 milestone Feb 15, 2018

Random-Liu self-assigned this Feb 15, 2018

Random-Liu reviewed Feb 15, 2018

View reviewed changes

Random-Liu mentioned this pull request Feb 16, 2018

Add CRI container log rotation support #59898

Merged

abhi force-pushed the log_stats branch from 344721a to d0abcf3 Compare February 17, 2018 21:26

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 17, 2018

abhi force-pushed the log_stats branch 2 times, most recently from 820c3ed to 24a2d33 Compare February 17, 2018 22:45

Random-Liu added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 21, 2018

Random-Liu reviewed Feb 21, 2018

View reviewed changes

abhi force-pushed the log_stats branch 3 times, most recently from 7f861d4 to 42b9205 Compare February 21, 2018 06:12

Random-Liu reviewed Feb 21, 2018

View reviewed changes

Test cases to verify container log stats

ad6bf35

The commit contains test case modifications to test and verify changes for container log stats feature. Signed-off-by: abhi <abhi@docker.com>

abhi force-pushed the log_stats branch from 42b9205 to ad6bf35 Compare February 21, 2018 21:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2018

feiskyer approved these changes Feb 22, 2018

View reviewed changes

feiskyer mentioned this pull request Feb 22, 2018

Adding per container log stats for windows containers #60180

Closed

k8s-github-robot merged commit 742c9b1 into kubernetes:master Feb 22, 2018

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 27, 2018

feiskyer mentioned this pull request Apr 2, 2018

CRI: update documents for container logpath #62015

Merged

Adding per container stats for CRI runtimes #59906

Adding per container stats for CRI runtimes #59906

Conversation

abhi commented Feb 15, 2018 • edited by Random-Liu Loading

k8s-ci-robot commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi commented Feb 16, 2018

k8s-ci-robot commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

feiskyer left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Feb 22, 2018

k8s-ci-robot commented Feb 27, 2018

andor44 commented Jul 11, 2018

Random-Liu commented Jul 11, 2018

andor44 commented Jul 11, 2018

Random-Liu commented Jul 11, 2018 • edited Loading

yujuhong commented Jul 11, 2018

feiskyer commented Jul 12, 2018

andor44 commented Jul 12, 2018

lucaim commented Jul 12, 2018 • edited Loading

Random-Liu commented Jul 13, 2018

yujuhong commented Jul 13, 2018

feiskyer commented Jul 16, 2018

abhi commented Feb 15, 2018 •

edited by Random-Liu

Loading

Random-Liu Feb 15, 2018 •

edited

Loading

Random-Liu Feb 15, 2018 •

edited

Loading

Random-Liu commented Jul 11, 2018 •

edited

Loading

lucaim commented Jul 12, 2018 •

edited

Loading