oci-umount: do not error out if the config file is missing

[giuseppe]

When the configuration file is missing, just raise a warning instead of
giving an error.

Closes: https://github.com/rhatdan/oci-umount/issues/2

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[rhvgoyal]

How did you run into this? Somebody removed the config file? It should be shipped with the package.

[rhvgoyal]

Having said that, warning when config file is missing sounds reasonable.

LGTM

[giuseppe]

we encountered this issue while running Docker in a system container and /etc coming from the host. We fixed it in the container as we copy the file to the host now, but I thought it would still be good to not error out when the conf file is missing.

/cc @ashcrow

[rhatdan]

LGTM

[rhvgoyal]

Now thinking more about it. Very fact that it was error you noticed it and fixed it by copying file to host. If it was just a warning, nobody would have noticed it in the flood of messages. And much later somebody would have complained that hey mounts points are leaking and oci-umount is not working. So may be making it error again makes more sense?

[ashcrow]

@rhvgoyal Interesting. If docker should indeed fail if the file doesn't exist I agree, though I'd suggest a more verbose error message. When I noticed the message it seemed more like "I can't find this file so I can't start" rather than "I must have this file else docker won't work correctly". If that's the case I don't mind updating our container Dockerfiles as well submitting a PR with an updated error message.

[rhvgoyal]

@ashcrow Whether docker works correctly without this file or not depends on how docker is being used. If people are using volume mounts like "-v /:/host" for containers, then there is a chance that docker will not work correctly for container removal.

I am not sure about using "must" keyword. I think its more a sanity check for the oci-umount plugin. And if a sanity check improves the probability of user having right configuration, I am up for it.

OTOH, what's the use case for making this a warning (instead of error). Where is that useful? If we don't have a good use case, I would say, just revert this PR and let the code be as it is.

[ashcrow]

@rhvgoyal I'm OK with a revert but I do feel pretty strongly on having a better error message. I'd say, with this new information, the use case for a warning changes and would be that oci-umount is not required, but may be used for sanity. Letting the administrator know that the file is missing is helpful. However, failing on a non required file is confusing.

I think the alternate solution is reverting and updating the error message to note that the file must exist to run docker (though nothing is done to make sure the file has content).

[rhvgoyal]

@ashcrow change error message and generate a PR if you like. But I am not sure how it is more helpful if I say File foo is required for sanity and its missing instead of Failed to open file foo, its missing.

In both the cases admin comes to know that docker is looking for a file which is missing.

[ashcrow]

@rhvgoyal Agreed. I'd just like to speed up the time 😄 .

👍 on revert. Will sent a PR with an updated message.

[rhvgoyal]

@ashcrow hey, did you generate a PR reverting this?

[rhvgoyal]

@giuseppe this PR changes pr_pwarning() to LOG_INFO, instead of LOG_WARNING. That's just wrong.

[ashcrow]

@rhvgoyal This is where I reverted some of this: #4

[rhvgoyal]

Ok, I see that pr_pwarning() is logging as LOG_INFO. I will fix it. This should have been reverted too.