Thank you for figuring this out! It is a subtle problem. More commentary below.
On Mon, Apr 29, 2013 at 08:16:36AM -0700, Gurucharan Shetty wrote: > Currently we are creating the worker after creation of the pidfile. > This means two things for ovs-vswitchd. One, the pidfile's file descriptor > is open in both main process and the worker process and closing of the file > descriptor in either of the process means we will loose the lock on the > pidfile. I don't think that that is the case, because file locks are not inherited across fork. (See fcntl(2) and http://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html.) I believe that it is only the main process closing the lockfile that will cause us to lose the lock. > Two, the responsibility of deleting the pidfile after process > termination rests with the worker process. I see that it does, but that was unintentional and I think it is a mistake. I think that the main process should delete the pidfile, because the main process has the actual responsibility for everything that ovs-vswitchd does. The worker process only does what it is told to do. > When we restart openvswitch using the startup scripts, we SIGTERM the main > process and once it is cleaned up, we start ovs-vswitchd again. This results > in a race condition. The new ovs-vswitchd will create a pidfile because it is > unlocked. But, if the old worker process exits after the start of new > ovs-vswitchd, it will simply delete the pidfile underneath the new > ovs-vswitchd. > This will eventually result in multiple ovs-vswitchd daemons. > > This patch moves make_pidfile to daemonize_complete. This way, we create the > pidfile after the creation of worker process. This means the responsibility of > deleting it lies with the main process and also as a side-effect, only one > process owns the file descriptor of the pidfile. > > Bug #16669. > Signed-off-by: Gurucharan Shetty <gshe...@nicira.com> I think that it is best to try to create the pidfile as early as we can. If there is a conflicting pidfile, then it is best to recognize this early on and avoid doing other initialization work. After looking at our code to handle forking and its aftermath, though, it seems like fixing this code would be more complicated and thus higher risk than just moving the make_pidfile() call. Do we need to backport this fix to branch-1.10? If so, then I think that the change you have here is the appropriate one. We can do better later, probably reverting this change as part of the improvements. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
