CRI: Symlink docker logs to CRI defined log path. #34858

Random-Liu · 2016-10-14T20:52:49Z

This PR symlink docker logs to the CRI defined log path.

I manually test myself, and it works. We still need some more unit test and a node e2e test.

@resouer is working on a node e2e test #34661.
For the unit test, the current MakeFakePodSandbox and MakeFakeContainer is not enough for unit test, because I need to create multiple instances for one container and sandbox to test garbage collection.

I'll send a separate PR to refactor the unit test framework in kuberuntime and finish the unit test.

@yujuhong @feiskyer
/cc @kubernetes/sig-node

This change is

Random-Liu · 2016-10-14T21:10:19Z

@k8s-bot cri test this.

resouer · 2016-10-15T04:52:15Z

node e2e test is #34877

Random-Liu · 2016-10-16T05:43:22Z

@resouer Thanks!

Random-Liu · 2016-10-17T05:15:43Z

Add one more commit to add the legacy container log location support.

feiskyer · 2016-10-17T05:44:02Z

pkg/kubelet/dockershim/docker_container.go

+	if err != nil {
+		return fmt.Errorf("failed to start container %q: %v", containerID, err)
+	}
+	// TODO: Should we stop the container if an error occurs during start.


I think so. If not, dockershim/kubelet doesn't know there is an error happened.

feiskyer · 2016-10-17T05:57:51Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -269,7 +277,8 @@ func makeMounts(opts *kubecontainer.RunContainerOptions, container *api.Containe
 		// here we just add a random id to make the path unique for different instances
 		// of the same container.
 		cid := makeUID()
-		containerLogPath := path.Join(opts.PodContainerDir, cid)
+		containerLogPath := filepath.Join(opts.PodContainerDir, cid)
+		// TODO: We should try to use os interface here.
 		fs, err := os.Create(containerLogPath)


Replace this with m.osInterface.Create?

A small problem is that makeMounts is not a member function of m. We can easily change that, just don't know whether we should make this change just for the unit test.

yujuhong · 2016-10-18T18:58:51Z

Reviewed 8 of 14 files at r1.
Review status: 8 of 15 files reviewed at latest revision, 3 unresolved discussions.

pkg/kubelet/dockershim/docker_container.go, line 240 at r1 (raw file):

      return "", "", fmt.Errorf("failed to inspect container %q: %v", containerID, err)
  }
  path, ok := info.Config.Labels[containerLogPathLabelKey]

If there is no label, maybe we can just return an empty path and don't error out?

Comments from Reviewable

Random-Liu · 2016-10-18T22:06:40Z

pkg/kubelet/dockershim/docker_container.go, line 194 at r2 (raw file):

Previously, feiskyer (Pengfei Ni) wrote…

I think so. If not, dockershim/kubelet doesn't know there is an error happened.

It depends on how important we think the container log file is. For a container which is running but doesn't have corresponding container log file, do we consider it to be usable or not?

@yujuhong WDYT?

Comments from Reviewable

Random-Liu · 2016-10-18T22:06:44Z

pkg/kubelet/dockershim/docker_container.go, line 240 at r1 (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

If there is no label, maybe we can just return an empty path and don't error out?

It should be fine for `StartContainer`, because we just write the label in `CreateContainer`. However, for `RemoveContainer`, we should not error out, because we may want to remove old containers and old containers may not have labels. I'll update the PR to return empty path and deal with the empty path at the caller.

Comments from Reviewable

yujuhong · 2016-10-18T22:37:30Z

pkg/kubelet/dockershim/docker_container.go, line 194 at r2 (raw file):

Previously, Random-Liu (Lantao Liu) wrote…

It depends on how important we think the container log file is.
For a container which is running but doesn't have corresponding container log file, do we consider it to be usable or not?

@yujuhong WDYT?

Let's do the best-effort cleanup and stop the container here.

Comments from Reviewable

Random-Liu · 2016-10-18T22:59:45Z

pkg/kubelet/dockershim/docker_container.go, line 194 at r2 (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

Let's do the best-effort cleanup and stop the container here.

ACK.

Comments from Reviewable

Random-Liu · 2016-10-20T23:32:06Z

@feiskyer @yujuhong Addressed comments. PTAL~

Random-Liu · 2016-10-20T23:42:40Z

@k8s-bot cri test this.

feiskyer · 2016-10-21T04:51:25Z

LGTM

yujuhong · 2016-10-21T17:16:48Z

pkg/kubelet/dockershim/docker_container.go

+	}
+	// Create container log symlink.
+	if err := ds.createContainerLogSymlink(containerID); err != nil {
+		// Stop the container if the symlink creation fails.


Discussed with @Random-Liu offline. We'll not stop the container here as the shim has no context of how to stop a user container properly. Since not being able to create a symlink should be rare, the function will just return an error to let kubelet report an event.

yujuhong · 2016-10-21T22:34:27Z

@Random-Liu let's prioritize this one to unblock the unit test PR :-)

Random-Liu · 2016-10-21T22:49:03Z

@yujuhong ACK. Will address this soon.

Random-Liu · 2016-10-21T23:31:00Z

@yujuhong Addressed comments.

The change I made:

Only gc pod log directories when all sources are ready.
Do not stop container when fail to create container log symlink.

Random-Liu · 2016-10-21T23:37:22Z

@k8s-bot cri test this.

feiskyer · 2016-10-22T15:50:00Z

/lgtm

k8s-ci-robot · 2016-10-22T16:39:55Z

Jenkins verification failed for commit 4de3d4f2d7a83ca9369ec34754385816a1b6bf53. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Random-Liu · 2016-10-24T02:10:40Z

Rebase and sent again, apply LGTM based on #34858 (comment)

k8s-github-robot · 2016-10-24T02:58:51Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2016-10-24T03:22:37Z

Jenkins GCE etcd3 e2e failed for commit fad4672. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-10-24T03:40:44Z

Automatic merge from submit-queue

@feiskyer

Automatic merge from submit-queue CRI: Refactor kuberuntime unit test Based on #34858 This PR: 1) Refactor the fake runtime service and some kuberuntime unit test. 2) Add better garbage collection unit test. 3) Fix init container unit test which isn't testing correctly. Some other unit tests may also need to be fixed. 4) Add pod log directory garbage collection unit test. @feiskyer @yujuhong /cc @kubernetes/sig-node

@yujuhong

Automatic merge from submit-queue CRI: Add kuberuntime container logs Based on #34858. The first 2 commits are from #34858. And the last 2 commits are new. This PR added kuberuntime container logs support and add unit test for it. I've tested all the functions manually, and I'll send another PR to write a node e2e test for container log. **_Notice: current implementation doesn't support log rotation**_, which means that: - It will not retrieve logs in rotated log file. - If log rotation happens when following the log: - If the rotation is using create mode, we'll still follow the old file. - If the rotation is using copytruncate, we'll be reading at the original position and get nothing. To solve these issues, kubelet needs to rotate the log itself, or at least kubelet should be able to control the the behavior of log rotator. These are doable but out of the scope of 1.5 and will be addressed in future release. @yujuhong @feiskyer @yifan-gu /cc @kubernetes/sig-node

cyphar · 2017-04-04T13:51:47Z

This is causing us issues with cri-o/cri-o#162. Has this problem been resolved, and if not where does the issue lie (the Docker or k8s side)?

googlebot added the cla: yes label Oct 14, 2016

Random-Liu added area/kubelet area/kubelet-api sig/node Categorizes an issue or PR as relevant to SIG Node. and removed cla: yes labels Oct 14, 2016

Random-Liu assigned feiskyer and yujuhong Oct 14, 2016

Random-Liu added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 14, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2016

Random-Liu force-pushed the symlink-docker-log branch from 443e931 to 98f4d07 Compare October 14, 2016 21:10

googlebot added the cla: yes label Oct 14, 2016

Random-Liu force-pushed the symlink-docker-log branch from 98f4d07 to d125b3b Compare October 17, 2016 03:54

feiskyer reviewed Oct 17, 2016

View reviewed changes

Random-Liu mentioned this pull request Oct 18, 2016

CRI: Refactor kuberuntime unit test #35018

Merged

Random-Liu force-pushed the symlink-docker-log branch from 2501f1c to 5a4ab5b Compare October 20, 2016 23:31

Random-Liu force-pushed the symlink-docker-log branch from 5a4ab5b to 007dfb7 Compare October 20, 2016 23:41

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2016

yujuhong reviewed Oct 21, 2016

View reviewed changes

yujuhong added this to the v1.5 milestone Oct 21, 2016

Random-Liu force-pushed the symlink-docker-log branch from 007dfb7 to 4de3d4f Compare October 21, 2016 23:29

Random-Liu mentioned this pull request Oct 22, 2016

CRI: Add kuberuntime container logs #35348

Merged

k8s-github-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2016

Random-Liu and others added 2 commits October 23, 2016 19:08

Symlink docker logs to CRI defined log path.

121a91e

Add legacy container log location support.

fad4672

Random-Liu force-pushed the symlink-docker-log branch from 4de3d4f to fad4672 Compare October 24, 2016 02:09

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2016

Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2016

k8s-github-robot merged commit 37dc74f into kubernetes:master Oct 24, 2016

Random-Liu deleted the symlink-docker-log branch October 24, 2016 04:19

yujuhong mentioned this pull request Oct 25, 2016

CRI implemention in kubelet (with in-process docker integration) #29478

Closed

25 tasks

This was referenced Apr 4, 2017

conmon: implement logging cri-o/cri-o#162

Merged

cri: legacy symlink breaks non-Docker CRI implementations #44043

Closed

mlmhl mentioned this pull request Nov 13, 2017

Garbage collection for legacy container log is useless #55607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRI: Symlink docker logs to CRI defined log path. #34858

CRI: Symlink docker logs to CRI defined log path. #34858

Random-Liu commented Oct 14, 2016 •

edited by k8s-oncall

Loading

Random-Liu commented Oct 14, 2016

resouer commented Oct 15, 2016

Random-Liu commented Oct 16, 2016

Random-Liu commented Oct 17, 2016

feiskyer Oct 17, 2016

feiskyer Oct 17, 2016

Random-Liu Oct 18, 2016

yujuhong commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

yujuhong commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

Random-Liu commented Oct 20, 2016

Random-Liu commented Oct 20, 2016

feiskyer commented Oct 21, 2016

yujuhong Oct 21, 2016

yujuhong commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

feiskyer commented Oct 22, 2016

k8s-ci-robot commented Oct 22, 2016

Random-Liu commented Oct 24, 2016

k8s-github-robot commented Oct 24, 2016

k8s-ci-robot commented Oct 24, 2016

k8s-github-robot commented Oct 24, 2016

cyphar commented Apr 4, 2017

CRI: Symlink docker logs to CRI defined log path. #34858

CRI: Symlink docker logs to CRI defined log path. #34858

Conversation

Random-Liu commented Oct 14, 2016 • edited by k8s-oncall Loading

Random-Liu commented Oct 14, 2016

resouer commented Oct 15, 2016

Random-Liu commented Oct 16, 2016

Random-Liu commented Oct 17, 2016

feiskyer Oct 17, 2016

Choose a reason for hiding this comment

feiskyer Oct 17, 2016

Choose a reason for hiding this comment

Random-Liu Oct 18, 2016

Choose a reason for hiding this comment

yujuhong commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

yujuhong commented Oct 18, 2016

Random-Liu commented Oct 18, 2016

Random-Liu commented Oct 20, 2016

Random-Liu commented Oct 20, 2016

feiskyer commented Oct 21, 2016

yujuhong Oct 21, 2016

Choose a reason for hiding this comment

yujuhong commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

Random-Liu commented Oct 21, 2016

feiskyer commented Oct 22, 2016

k8s-ci-robot commented Oct 22, 2016

Random-Liu commented Oct 24, 2016

k8s-github-robot commented Oct 24, 2016

k8s-ci-robot commented Oct 24, 2016

k8s-github-robot commented Oct 24, 2016

cyphar commented Apr 4, 2017

Random-Liu commented Oct 14, 2016 •

edited by k8s-oncall

Loading