node, syscontainer: drop /var/lib/docker mount point

[giuseppe]

Reference: #15105

Signed-off-by: Giuseppe Scrivano [email protected]

[ashcrow]

The change itself looks good.

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

[giuseppe]

It doesn't work if I drop the /var/lib/docker mount point altogether as the build will try to access log files that are under /var/lib/docker/containers, so I've replaced the mount point instead of dropping it completely. I've verified that I am able to build and deploy a POD with this change.

[giuseppe]

@rhvgoyal thanks for the review. I've added another patch that drops any mount point under /var/lib/docker/containers (except /var/lib/docker/containers itself).

I've verified that no shm mount is leaked into the node container.

[rhvgoyal]

oci-umount will do this for you anyway. So you don't have to do this yourself?

Given you need /var/lib/docker/containers/ to access these logs, I would say for your use case volume mount it in. And oci-umount will make sure all mount points under /var/lib/docker/containers don't show up inside containers.

[giuseppe]

does oci-umount work as a runC hook? There are some differences in the way hooks work with runC compared to our version of Docker. We needed to do some changes into oci-systemd-hook and oci-register-machine to get them working with runC. So if it was not already tested, it will probably require some (minor) changes to oci-umount before it can be used within runC.

[rhvgoyal]

oci-umount is supposed to be working same way as oci-umount-systemd hook. I think ultimately they are runc hooks. @dwalsh had done some fixes recently to take care of some of the differences. So I am hoping latest upstream works for you. Please give it a try. If not, let us know, we will fix it.

[rhvgoyal]

I have tested oci-umount only with docker. With system-containers and runc, calling convention might be little different (as you mentioned) and it might require some tweaks.

Having said that @dwlash had done some changes recently so that it works with cri-o and I hope that takes care of system container issues as well.

[rhvgoyal]

cc @rhatdan

[giuseppe]

@rhvgoyal @rhatdan thanks for the update, but unfortunately we are still not able to use it yet as it is not on RHELAH 7.3.6.
That opens a bigger question, how will we distribute OCI hooks if we will ever stop distributing docker as part of the image and consume it exclusively as a system container? Will the runC .rpm depends on them?

[rhvgoyal]

making runc rpm depend on oci hook rpms sounds reasonable to me.

I am sort of surprised that 7.3.6 does not have oci-umount package.

[rhvgoyal]

cc @lsm5

[rhatdan]

oci-umount is currently a subpackage of the docker package. We want to move this out. CRI-O will depend on oci-umount also.
The latest oci-umount should work with the older version of docker-runc as well as the newer runc versions.

[ashcrow]

/assign @sdodson

[sdodson]

/approve no-issue

[ashcrow]

@sdodson

Required Github CI test is not green: ci/openshift-jenkins/cmd

Not sure what that's about ...

[stevekuznetsov]

/test all

@ashcrow that means the required test did not finish successfully

[ashcrow]

@stevekuznetsov thanks! I returned yesterday and didn't realize the new commands.

[giuseppe]

/test all

[ashcrow]

/retest

[ashcrow]

/test extended_conformance_gce

[ashcrow]

Looks totally unrelated but ...

    Play:     Initialize host facts
     Task:     initialize_facts set_fact on openshift_docker_hosted_registry_network
     Message:  the field 'args' has an invalid value, which appears to include a variable that is undefined. The error was: 'dict object' has no attribute 'openshift'
               
               The error appears to have been in '/usr/share/ansible/openshift-ansible/playbooks/common/openshift-cluster/initialize_facts.yml': line 154, column 5, but may
               be elsewhere in the file depending on the exact syntax problem.
               
               The offending line appears to be:
               
               
                 - name: initialize_facts set_fact on openshift_docker_hosted_registry_network
                   ^ here

[giuseppe]

/retest

[ashcrow]

/test cmd

[stevekuznetsov]

@sdodson is this good to go? Missing a LGTM

[ashcrow]

/lgtm

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, giuseppe, sdodson

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

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

• images/node/OWNERS [sdodson]

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]

Automatic merge from submit-queue (batch tested with PRs 17476, 17143, 15115, 17094, 17500).