Skip to content
This repository has been archived by the owner on Apr 22, 2020. It is now read-only.

[kubelet.service] Add the mounting of /var/log. #258

Merged
merged 1 commit into from Oct 5, 2016

Conversation

jordanyaker
Copy link
Contributor

The mounting of /var/log enables necessary log sharing when Fluentd is in play.

@colemickens
Copy link
Contributor

@jordanyaker I had fixed this previously: #164

I noticed today that apparently this change got lost when we moved to ignition.

Do we need all of /var/log or just /var/log/containers?

@colemickens
Copy link
Contributor

Additionally, kube-deploy only mounts in /var/log/containers/: https://github.com/kubernetes/kube-deploy/blob/master/docker-multinode/common.sh#L87

@jordanyaker
Copy link
Contributor Author

@colemickens I'm not 100% about /var/log vs /var/log/containers. I was so far down the rabbit hole by the time I got to this fix that I didn't try to experiment. I'm betting you could make a case for either.

@jordanyaker
Copy link
Contributor Author

I was really doing this to try and get Deis working on Azure. deis/logger#50 specifically references /var/log so I went with that first.

@@ -6,6 +6,7 @@ Documentation=https://github.com/kubernetes/kubernetes
ExecStartPre=/bin/mkdir -p /var/lib/kubelet
ExecStartPre=/bin/mount --bind /var/lib/kubelet /var/lib/kubelet
ExecStartPre=/bin/mount --make-shared /var/lib/kubelet
ExecStartPre=/bin/mkdir -p /var/log/containers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed, docker will create dirs ahead of time for you:

$ docker run -it -v /home/cole/path/does/not/exist:/blah busybox
/ # exit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I was unaware of that. I added and removed that line probably about half a dozen times during my troubleshooting.

@@ -15,6 +16,7 @@ ExecStart=/usr/bin/docker run \
-v /var/run:/var/run:rw \
-v /var/lib/docker/:/var/lib/docker:rw \
-v /var/lib/kubelet/:/var/lib/kubelet:shared \
-v /var/log:/var/log:shared \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you go ahead and change this to /var/log/containers:/var/log/containers:rw so that we're consistent with kube-deploy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw your additional comment... And this comment from @chancez deis/logger#50 (comment)

@mikedanese What do you think? Sounds like we want to mount /var/log rather than /var/log/containers possibly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI referring to this: https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/fluentd-elasticsearch/fluentd-es-image/td-agent.conf#L113

Those are the position files, which keep track of how far into a particular log file fluentd has ingested. If the container was deleted (ie for an upgrade) these files would be lost if they're not being stored on the host via a volume mount, resulting in fluentd re-ingesting logs that have already been ingested previously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chancez for elaborating (it made me completely convinced). Can you confirm if is it okay to leave it mounted as shared?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea on that regard, sorry!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, @chancez convinced me. If you remove the superfluous line above (and my current test deployment looks okay) then I'll merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my question about that: does anything else create the /var/log/containers folder? If we're only sharing /var/log, doesn't there stand a chance that we will need to first create the containers folder? Or, is there another component that will check and create that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think kubelet is smart enough to create that directory but I can't be sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, something is smart enough to make it. I merged this PR locally (but without the ExecStartPre line) and deployed a cluster. Looks like logs are flowing into ES/Kibana now as expected and /var/log/containers was created on the nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for me. I've pushed the changes with the removal of that line.

@colemickens colemickens merged commit 7f31f44 into kubernetes-retired:master Oct 5, 2016
@colemickens
Copy link
Contributor

I've pushed up the k8s-ignition container and tested it again. Looks good. Thanks @jordanyaker!

@jordanyaker
Copy link
Contributor Author

Glad to be of assistance!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants