Skip to content
This repository has been archived by the owner on Sep 4, 2021. It is now read-only.

Expose hosts' /var/log to the kubelet by default #650

Merged
merged 4 commits into from
Sep 9, 2016
Merged

Expose hosts' /var/log to the kubelet by default #650

merged 4 commits into from
Sep 9, 2016

Conversation

danielfm
Copy link
Contributor

@danielfm danielfm commented Sep 1, 2016

The kubelet creates symlinks that capture the pod name, namespace, container name & Docker container ID to the Docker logs for pods in the /var/log/containers directory on the host.

By mapping the volume for this directory to the kubelet container by default, it becomes easier to use cluster level logging in Kubernetes, i.e., fluentd-elasticsearch, if the user choose to (see my comment in #320).

This PR updates both single-node and multi-node configurations.

(I should probably update some documentation as well, but first I want to know how you guys feel about this.)

@robszumski
Copy link
Member

I'm in favor of this, as it fixes #322 (comment). What do you think @colhom?

You are correct, the docs will need to be updated in all of the manual example manifests, etc.

@danielfm danielfm changed the title kube-aws: expose hosts' /var/log to the kubelet kube-aws: expose hosts' /var/log to the kubelet by default Sep 1, 2016
@AlmogBaku
Copy link

LGTM

@danielfm
Copy link
Contributor Author

danielfm commented Sep 1, 2016

@robszumski I figured there was no documentation about exposing the /var/log directory to the kubelet, but apparently I was wrong.

Should I change anything there?

@chancez
Copy link
Contributor

chancez commented Sep 1, 2016

I am strongly in favor of this change.

@danielfm maybe update the PR title to reflect updating more than AWS

@danielfm danielfm changed the title kube-aws: expose hosts' /var/log to the kubelet by default Expose hosts' /var/log to the kubelet by default Sep 1, 2016
@robszumski
Copy link
Member

@danielfm yes, it is documented there, but if we are going to do this by default we need to remove the example from there and add it to each of the example units in all of the guides.

I think thats these, in addition to what you've already changed:

https://github.com/coreos/coreos-kubernetes/blob/master/Documentation/deploy-master.md#create-the-kubelet-unit
https://github.com/coreos/coreos-kubernetes/blob/master/Documentation/deploy-workers.md#create-the-kubelet-unit

@robszumski
Copy link
Member

Nice, I think you got all of em.

--mount volume=rkt,target=/usr/bin/rkt \
--volume var-lib-rkt,kind=host,source=/var/lib/rkt \
--mount volume=var-lib-rkt,target=/var/lib/rkt \
--volume=stage,kind=host,source=/tmp \
--mount volume=stage,target=/tmp"
--volume stage,kind=host,source=/tmp \
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation for host mounting /tmp?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our setup actually mounts in an entirely separate directory to store cursor positions for our aggregrator (currently fluentd) across pod lifecycle. I suspect maybe that's what this is for?

Copy link
Contributor

@chancez chancez Sep 2, 2016

Choose a reason for hiding this comment

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

The default setup uses a file in /var/log for the cursor, which is why it's /var/log instead of just /var/log/container, so it's not for that I dont think

Copy link
Contributor Author

@danielfm danielfm Sep 2, 2016

Choose a reason for hiding this comment

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

@colhom

What is the motivation for host mounting /tmp?

Our setup actually mounts in an entirely separate directory to store cursor positions for our aggregrator (currently fluentd) across pod lifecycle. I suspect maybe that's what this is for?

The /mnt mounting already existed there; I only removed the = there after --volume to keep its style consistent with the other flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

/tmp is mounted in for rktnetes. It will eventually go away but for now it is needed for some communication between host rkt and containerized kubelet.

@AlmogBaku AlmogBaku mentioned this pull request Sep 2, 2016
18 tasks
@danielfm
Copy link
Contributor Author

danielfm commented Sep 5, 2016

Is there anything else I need to do to get this merged?

Daniel Fernandes Martins added 4 commits September 8, 2016 15:10
The kubelet creates symlinks that capture the pod name, namespace,
container name & Docker container ID to the docker logs for pods in the
/var/log/containers directory on the host.

By mapping the volumes for those directories to the kubelet container,
it becomes easier to use cluster level logging in Kubernetes, i.e.,
fluentd-elasticsearch.
The kubelet creates symlinks that capture the pod name, namespace,
container name & Docker container ID to the docker logs for pods in the
/var/log/containers directory on the host.

By mapping the volumes for those directories to the kubelet container,
it becomes easier to use cluster level logging in Kubernetes, i.e.,
fluentd-elasticsearch.
Since this is now part of the default configuration, there's no need to
document it as an optional step.
@aaronlevy
Copy link
Contributor

Overall this lgtm. @chancez or @colhom can you confirm these changes work for you? I've not ever setup any k8s log aggregation

@danielfm
Copy link
Contributor Author

danielfm commented Sep 8, 2016

I've been running my clusters with this mod for at least three months, but of course, the more people testing this the better!

@chancez
Copy link
Contributor

chancez commented Sep 8, 2016

@aaronlevy these emulate exactly what i've had in a patch file for multiple months, if I find sometime I can validate exactly this PR on vagrant etc (no time for much else), but all the changes look fine to me.

I personally prefer the --flag=value form with the =, but thats not really a big deal and I'd rather get this in.

@colhom
Copy link
Contributor

colhom commented Sep 9, 2016

Thanks @danielfm.

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

Successfully merging this pull request may close these issues.

7 participants