Update resolve.conf in containers on file creation.

[ghost]

NetworkManager updates resolve.conf by replacing the current file
with an updated one. This change enables docker to listen for these
events.

I was playing around with @estesp's patch to see if changes made by NetworkManager (when disconnecting from the network or switching to wifi for example) are picked up by the containers after a restart. It turns out that the changes were not picked up because the file is replaced rather than modified.

[estesp]

This looks like a good catch. Thanks!

[LK4D4]

@swagiaal Thank you! can we write test for this? I think similar test exists for writing file.

[ghost]

Cool! I'll work on a test then

[ghost]

Hey guys sorry for the delayed update but I ran into a few hurdles which I will outline here so you can give me some pointers if I have missed something.

The first issue was with writing the test. In order to test replacing resolv.conf with resolv.conf.tmp, resolv.conf must be removed but during the test resolv.conf is a bind mount and therefor must be unmounted, written, then the rest of the test can procede with replacing and checking for updates.

The other issue is to get the CREATE event one must watch the directory '/etc' and not just '/etc/resolv.conf' (I must have missed something the first time, but the test makes this clear) so I have added that to the patch.

The last thing is that, while watching /etc suffices for most cases, when /etc/resolv.conf is a bind mount watching the directory will not notify you of any changes which are made to the file. So I have kept the original watch on /etc/resolv.conf

What do you guys think ?

[jessfraz]

@estesp wdyt?

[estesp]

Thanks for digging into this--obviously the replace/rename is a case that needed coverage and testing. Could you remove the watcher.Add() on the filename itself? After @crosbymichael and I did some local testing, that (original) watcher on the filename is useless because a REMOVE event happens, leaving it dormant after that, which is why you had to add a watcher on the directory to catch CREATE.

After that change, LGTM

[ghost]

@estesp hmmm... that was my first instinct as well. The issue is if I remove the file watcher, your original tests do not pass. The reason for this is that during the test run /etc/resolv.conf is a bind mount and having a watcher on /etc is not enough when /etc/resolv.conf is a bind mount. The test can be changed of course to read resolv.conf, unmount it, write the original file and continue if we choose to remove the file watcher. If we choose to keep the file watcher we would have to handling the dangling watcher situation you mentioned. WDYT?

[estesp]

@crosbymichael do we think we need to support /etc/resolv.conf being a bind-mount in normal scenarios for update? As in, the host/daemon is either a Docker container (like in test mode), or the host/daemon already has some unique setup where /etc/resolv.conf is a bind mount?

[estesp]

@swagiaal let's go ahead and fix the test condition with unmount, like you did for your test. @crosbymichael and I agreed it is a rare case, and therefore the only impact is no support for resolv conf updating when /etc/resolv.conf itself is a bind mount on the host/daemon.

[ghost]

@estesp thanks for following up with @crosbymichael. I have made the changes you suggested: removed the file watcher, and modified the test case to unmount resollv.conf and test using a regular file.

[estesp]

@crosbymichael I think this is ready for LGTM unless we have any other thoughts here

[ghost]

@crosbymichael hey... have you had a chance to look at this ?

[jessfraz]

can we have an integration test?

[estesp]

it was added to the current integration-cli test for resolvconf watcher cases (he added "#6" to test the creation vs. modification path)

[jessfraz]

wow sorry, read wrong LGTM

[estesp]

LGTM

[ghost]

Thanks for reviewing and merging!