Add capability to unmount submounts of a mount point on host #15

rhvgoyal · 2017-08-08T13:31:58Z

Right now /etc/oci-umount.conf contains path of mount points on host which get unmounted in container. There is a corner case where we don't want top level mount to be unmounted, instead want only submounts to go away.

This patch series adds that capability.

rhvgoyal · 2017-08-08T13:32:33Z

cc @rhatdan @mrunalp @eparis

rhatdan · 2017-08-08T14:16:02Z

src/oci-umount.c

+		if (!ret)
+			pr_pinfo("Unmounted submount: [%s]\n", mnt_table[i].destination);
+		else
+			pr_perror("Failed to unmount submount: [%s]. Skipping.\n", mnt_table[i].destination);


Should we check for the status that the mount point was not mounted and not log anything (EINVAL). Since this is not an error.

In most of the cases mount point should be mounted. We have got mount point information from /proc/self/mouninfo. So this should hit rarely and if it hits, it will give us an idea of a potential problem or wrong assumption etc. So I would rather keep it this way.

rhatdan · 2017-08-08T14:17:03Z

src/oci-umount.c

+			continue;
+		ret = umount2(mnt_table[i].destination, MNT_DETACH);
+		if (!ret)
+			pr_pinfo("Unmounted submount: [%s]\n", mnt_table[i].destination);


I am a little worried about these filling up logs, should this be pr_pdebug?

So debug logs will go in journal as well right. Just with a different priority level? So it does not solve filling log problem?

rhatdan · 2017-08-08T14:18:25Z

src/oci-umount.c

@@ -369,21 +432,34 @@ static int prestart(const char *rootfs,
 	}

 	while ((read = getline(&line, &len, fp)) != -1) {
+		bool submounts_only = false;


You should grab the comments part of my patch to allow comments to be added to /etc/oci-umount.conf file. We need an explanation of this behaviour in the config file.

Comment handling and rewritten oci-umount.conf file in this pr #14

Ok, I will add a patch to be able to handle comments.

rhatdan · 2017-08-08T14:19:53Z

src/oci-umount.c

 		if (nr_mapped < 0) {
-			pr_perror("Error while trying to map mount [%s] from host to conatiner. Skipping.\n", mounts_on_host[i]);
+			pr_perror("Error while trying to map mount [%s] from host to conatiner. Skipping.\n", mounts_on_host[i].path);


If these are expected to fail, should these just be pr_pdebug?

They are not expected to fail. Only if there is an expected error, we print this. If there are no mappings, this function returns 0 and we don't output any message.

I was thinking this was contents in /etc/oci-umount which we were telling the tool to look for mountpoints, and it was reporting that they did not exist.

No, that's different one. This is about that we found a mount point on host and now we are trying to figure out if it is mapped inside the container either directly or as a submount. If there is no mapping present, it will not print anything.

rhatdan · 2017-08-08T14:21:36Z

@imcleod @eparis @mrunalp PTAL

rhvgoyal · 2017-08-08T19:31:39Z

@rhatdan Updated patches to ignore comments and blank lines. Also added few comments to oci-umount.conf. PTAL.

I am not not sure how to reduce number of messages in journal. Most of the messages are failed to canonicalize paths which are present by default in /etc/oci-umount.conf. Ideally if we can't canonicalize any path in /etc/oci-umount.conf and we are skipping it, we need to output it atleast with INFO level and not debug level. So not sure what to do about it.

rhatdan · 2017-08-08T19:34:00Z

Why wouldn't this just be Debug. We know almost every container that gets run will produce 7 or 8 of these messages. If the customer has syslog setup, those messages are going to get into /var/log/messages.

rhvgoyal · 2017-08-08T19:34:53Z

How would a user know that path they specified got silently ignored. If we put it in a debug message.

rhatdan · 2017-08-08T19:36:45Z

Hopefully they would look in the debug log, We could put something in the config to tell them that mount points not found in the container will be logged at the debug level. Then 99.99% of all users would not be bothered by useless messages.

rhvgoyal · 2017-08-08T19:39:43Z

I converted it to pr_pdebug() and still I see following in journal by default.

Aug 08 15:37:49 vm8-f26 oci-umount[23940]: umounthook <debug>: Failed to canonicalize path [/var/lib/docker/overlay]: No such file or directory. Skipping.

So message is very much in journal and seems to show up by default. So converting it to debug level does not seem to help a lot.

rhvgoyal · 2017-08-08T19:41:33Z

Only if I specifically filter out debug messages, then these messages don't appear in output.

journalctl -f -l -p 6

So journalctl default seems to be to output even debug messages.

rhvgoyal · 2017-08-08T19:47:33Z

@rhatdan, I think this PR is about adding submount capability. So lets review this and merge and we should discuss how to reduce the verbosity of messages in a separate issue/PR.

rhatdan · 2017-08-08T20:05:00Z

Well syslog is does info an above.

Ignore any lines in /etc/oci-umount.conf starting with '#' or '\n' Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

This is useful when trying to determine which are direct submounts of a mount point. Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

/etc/oci-umount.conf contains mount points on host which need to be unmounted inside the containers if they leak into it. This does lazy unmount on mount point which makes mount point and all the submounts disappear from container's view. There have been cases where containers don't want top level mount to disappear and want only submounts to disappear (if any). For example, one can volume mount in /var/lib/docker/containers, and fluentd container wants all submounts of /var/lib/docker/containers to be unmounted but leave /var/lib/docker/containers mount point untouched. Hence this patch extends syntax of /etc/oci-umount.conf file and if a path is suffixed by "/*", that means any submounts under that path will be unmounted but not the actual mount. For example, if one specifices "/var/lib/docker/containers/" then it will be lazy unmounted. But if one specifies "/var/lib/docker/containers/*" then all the submounts of it will be lazy unmounted. Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

fluentd mounts /var/lib/docker/containers as volume and they expect only submounts to go away. They don't want /var/lib/docker/containers/ to disappear otherwise they can't access container logs. Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

rhvgoyal · 2017-08-09T19:10:28Z

@rhatdan Now I have taken your comment() function. Did minor cleanups there. (Got rid of break statements). PTAL.

rhvgoyal · 2017-08-11T19:31:51Z

@rhatdan PTAL.

rhatdan · 2017-08-14T14:50:15Z

LGTM

rhatdan reviewed Aug 8, 2017

View reviewed changes

rhvgoyal force-pushed the unmount-submounts branch from 4de0a77 to ea3e155 Compare August 8, 2017 19:29

rhvgoyal force-pushed the unmount-submounts branch from ea3e155 to 953736c Compare August 9, 2017 18:26

rhvgoyal added 4 commits August 9, 2017 15:09

Ignore comments and empty lines while parsing config file

be8de7a

Ignore any lines in /etc/oci-umount.conf starting with '#' or '\n' Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

start storing mount id and parent mount id

b7b1344

This is useful when trying to determine which are direct submounts of a mount point. Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

rhvgoyal force-pushed the unmount-submounts branch from 953736c to 6f0317a Compare August 9, 2017 19:09

rhatdan merged commit 299e781 into containers:master Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capability to unmount submounts of a mount point on host #15

Add capability to unmount submounts of a mount point on host #15

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan Aug 8, 2017

rhvgoyal Aug 8, 2017

rhatdan Aug 8, 2017

rhvgoyal Aug 8, 2017

rhatdan Aug 8, 2017

rhatdan Aug 8, 2017

rhvgoyal Aug 8, 2017

rhatdan Aug 8, 2017

rhvgoyal Aug 8, 2017

rhatdan Aug 8, 2017

rhvgoyal Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 9, 2017

rhvgoyal commented Aug 11, 2017

rhatdan commented Aug 14, 2017

Add capability to unmount submounts of a mount point on host #15

Add capability to unmount submounts of a mount point on host #15

Conversation

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhvgoyal commented Aug 8, 2017

rhatdan commented Aug 8, 2017

rhvgoyal commented Aug 9, 2017

rhvgoyal commented Aug 11, 2017

rhatdan commented Aug 14, 2017