remove /var/lib/docker volume from containerized node

[sjenning]

xref https://bugzilla.redhat.com/show_bug.cgi?id=1467824

tl;dr upstream docker does not allow the forced removal of containers anymore. Before, a forced removal, when using devicemapper graph driver, simply orphaned a thin device and those devices would eventually consume the pool. The upstream docker change was backported to docker-1.12.6-40 to fix this issue.

However, the mounting of /var/lib/docker into the containerized node is causing other openshift services from being able to (re)start successfully since the systemd unit files first remove the old container, which fails due to the containerized node holding the mount point, then tries a docker run, which fails because a container with that name already exists.

To avoid this, the PR removes /var/lib/docker from the volumes mounted into the containerized node.

@derekwaynecarr @smarterclayton @eparis

[sjenning]

@rhvgoyal just saw your update on the BZ. Is this not needed anymore?

[rhvgoyal]

@sjenning I have created a PR so that oci-umount plugin will take care of removing multiple mounts inside container. But I think modifying unit file to not mount /var/lib/docker/ to begin with is a good idea.

IOW, while my PR to oci-umount will take care of this, but we should merge this change anyway (irrespective of my PR).

[rhvgoyal]

containers/oci-umount#10

[sjenning]

[test]

[smarterclayton]

@csrwng do we need to change cluster up?

[openshift-bot]

Evaluated for origin test up to 38b2082

[csrwng]

@smarterclayton yes we need to remove it there as well, will create a PR

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3031/) (Base Commit: 18c5730) (PR Branch Commit: 38b2082)

[csrwng]

@sjenning @rhvgoyal under what circumstances should we not mount /var/lib/docker anymore? Is it based on the Docker version? or the version of the kubelet? Will things break, for example, if I try to run origin v1.5.1 images on top of Docker docker-1.12.6-30 and I don't mount /var/lib/docker ?

[rhvgoyal]

@csrwng I am expecting that you don't have to mount /var/lib/docker/ in any circumstance at all. There is one logging case of fluentd which still needs it. Apart from that, if there are other use cases, I want to know.

[csrwng]

@rhvgoyal So for the one fluentd case, is it no longer required?

[rhvgoyal]

@rhvgoyal I think for fluentd case you still require it. As I understand it, for json log based driver, they still go trough container logs under /var/lib/docker.

[rhvgoyal]

@csrwng I myself am trying to figure out who else needs it and what breaks if we don't mount it.

[giuseppe]

would a build still work? I've tried this change in the system container and if there is no /var/lib/docker/containers present, the build fails with:

# oc logs -f bc/hello-openshift-plus
failed to open log file "/var/log/pods/7267b90a-65dc-11e7-9b7c-525400874d80/docker-build_0.log": open /var/log/pods/7267b90a-65dc-11e7-9b7c-525400874d80/docker-build_0.log: no such file or directory[root@69ccc7cff516 /]# oc logs -f bc/hello-openshift-plus

That happens since files under /var/log/pods are symbolic links to /var/lib/docker/containers/, e.g.

ls -l /var/log/pods/bd5e4ab0-65df-11e7-80e7-525400874d80/docker-build_0.log
lrwxrwxrwx. 1 root root 165 Jul 11 02:22 /var/log/pods/bd5e4ab0-65df-11e7-80e7-525400874d80/docker-build_0.log -> /var/lib/docker/containers/37eb94514154a15d542856c03a66cd08fc0d010a1ff9b327e79ceffffe8b4a29/37eb94514154a15d542856c03a66cd08fc0d010a1ff9b327e79ceffffe8b4a29-json.log

I have not tested this with the Docker container, so please ignore my comment if it doesn't happen there. If so though, what is done differently than files in /var/log/pods have the right content?

[rhvgoyal]

@giuseppe These seems to be just accessing json logs of a container. /var/lib/docker/containers/ is just a bind mount onto itself. So even if I unmount it, logs are still there.

So I doubt this breakage is due to unmounting of /var/lib/docker/containers. That directory and logs with-in should still be accessible.

[giuseppe]

@rhvgoyal yes true, my point was that if we just drop the /var/lib/docker mount point (as currently the patch seems to do) we lose completely access to /var/lib/docker/containers from the node container.

Do we need to add a mount point for /var/lib/docker/containers since /var/lib/docker is not mounted anymore or how that will be accessible? I needed this in the system container case to fix the issue I reported above.

[rhvgoyal]

I don't understand openshift. So you will have to simplify it for me. So why do we need access to logs while doing a build?

[giuseppe]

I am not expert either but my guess is that it is needed for oc logs. If I use oc logs I can see the docker build log there.

[rhvgoyal]

Ok, so this is the case where "oc logs" is being run from inside a container and it is trying to access logs of a container? This sounds similar to fluentd accessing container logs.

So why are we not using "docker logs" to access logs?

[rhvgoyal]

Anyway, if some container does need /var/lib/docker, then yes they will have to volume mount it in. I don't know which containers need it. So openshift service/container needs to decide which containers really need it.

oci-umount should take care of unmounting /var/lib/docker/devicemapper and /var/lib/docker/containers from inside the containers.

[smarterclayton]

@derekwaynecarr @sjennings can you help Vivek at all with questions about what the kubelet needs access to in order to service logs?

…

On Tue, Jul 11, 2017 at 9:39 AM, Vivek Goyal ***@***.***> wrote: Anyway, if some container does need /var/lib/docker, then yes they will have to volume mount it in. I don't know which containers need it. So openshift service/container needs to decide which containers really need it. oci-umount should take care of unmounting /var/lib/docker/devicemapper and /var/lib/docker/containers from inside the containers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15105 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_mP3myJM4532122UOJMORSLhQn-ks5sM3ssgaJpZM4ORfFR> .

[sjenning]

@smarterclayton I was trying to prove-by-doing but containerized install is busted #15147. I'll have to look at the code in the meantime.

[sjenning]

This is the only thing I can find

https://github.com/openshift/origin/blob/master/pkg/cmd/server/kubernetes/node/node.go#L61-L63

For that, /var/lib/docker doesn't have to be mounted in, but it does need to exist.

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sjenning
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sjenning
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[deads2k]

/assign sjenning
/unassign

[sjenning]

@rhvgoyal do we still need this?

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[sjenning]

I think this is handled via other means now