Ben Pfaff <[email protected]> wrote on 03/30/2016 02:36:01 PM:
> From: Ben Pfaff <[email protected]> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: [email protected] > Date: 03/30/2016 02:36 PM > Subject: Re: [ovs-dev] [PATCH v2] Dynamically reconnect ovn- > controller if ovn-remote value changes > > On Thu, Mar 24, 2016 at 10:50:47AM -0500, Ryan Moats wrote: > > From: RYAN D. MOATS <[email protected]> > > > > Allows for auto detection and reconnect if the ovn-remote needs > > to change. ovn-controller test case updated to include testing > > this code > > > > Signed-off-by: RYAN D. MOATS <[email protected]> > > Thanks for the patch! > > Ordinarily we don't add code at the end of a program's main loop, > following the call to poll_block(). I'd recommend putting this code at > the top of the main loop instead. > > I'd prefer to put this new code in a function instead of inline. > > This approach to changing the remote blocks the entire ovn-controller > main loop while it retrieves a snapshot from the new remote. That is > not necessary and it prevents other work from getting done. (Also, if > the retrieval stalls, e.g. because the new remote does not point to an > ovsdb-server, and the remote gets changed back to a correct database > server, then ovn-controller will never recover.) Thus, I suggest > instead adding an ovsdb_idl_*() function to set a new remote and letting > the IDL retrieve a new snapshot asynchronously; it already knows how to > do the latter. > > Thanks for writing a test. I don't think the "sleep" calls are needed > because check_patches will already wait until the change takes effect. > > In the test, please use on_exit to ensure that the new ovsdb-server is > killed if the test is interrupted or fails. Also, please use > OVS_APP_EXIT_AND_WAIT to cause the new ovsdb-server to exit gracefully. > Thanks for the comments, Ben. I'll see about addressing them in the next patch set (but I'm not 100% sure when that will be)... Ryan _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
