| Message ID | 20191018204259.1113-1-mmichels@redhat.com |
|---|---|
| Headers | show |
| Series | Separate pinctrl to its own process | expand |
On Fri, Oct 18, 2019 at 04:42:54PM -0400, Mark Michelson wrote: > This proposes a set of patches to move pinctrl operations out of the > ovn-controller process and into its own. Interesting! I would not have guessed that the operations were independent enough to make this practical. I have not reviewed the series yet.
I realized that after my latest rebase, there are three tests that are failing with this changeset: IGMP snoop/querier/relay ARP lookup before learning vtep 3HVs, 1 VIFs/HV, 1 GW, 1 LS They don't fail in master, so I know they're the fault of the branch. With that in mind, I will fix these failures and post a v2 of this RFC. Don't let that deter you from having a look at v1 though, since you still can get a feel for what the change is proposing. On 10/18/19 4:42 PM, Mark Michelson wrote: > This proposes a set of patches to move pinctrl operations out of the > ovn-controller process and into its own. > > The main reasons for doing this are the following: > 1) Separating pinctrl makes it so that receiving a packet-in can't wake > up ovn-controller. > 2) Separating pinctrl allows for manipulating the southbound database > directly while handling packets in, thus minimizing the need for storing > local copies of data > 3) This lays the groundwork for an easier eventual conversion of > ovn-controller to DDlog, since the DDlog code would need to only handle > flow creation, not packet in handling. > > This is an RFC. With this set of changes, item (2) above is not well > addressed here. While the multithreading is removed from pinctrl, the > structural components have not been altered. Were this idea to be > approved, point (2) would be addressed when creating the final patch. > > Please share your thoughts. > > Mark Michelson (5): > Separate pinctrl to its own process. > Resolve duplicate functions in ovn-controller and ovn-pinctrl. > Remove multithreading from pinctrl. > Move ovn-pinctrl to its own directory. > Flesh out manpage with more details about ovn-pinctrl > > Makefile.am | 1 + > controller/automake.mk | 3 +- > controller/binding.c | 22 +- > controller/binding.h | 3 +- > controller/controller-utils.c | 220 +++++++++++ > controller/ovn-controller.c | 233 +----------- > controller/ovn-controller.h | 20 + > pinctrl/automake.mk | 25 ++ > pinctrl/ovn-pinctrl.8.xml | 110 ++++++ > pinctrl/ovn-pinctrl.c | 413 +++++++++++++++++++++ > {controller => pinctrl}/pinctrl.c | 748 ++++++++++---------------------------- > {controller => pinctrl}/pinctrl.h | 0 > tests/automake.mk | 2 +- > tests/ofproto-macros.at | 3 + > tests/ovn.at | 13 +- > tutorial/ovs-sandbox | 5 + > utilities/ovn-ctl | 40 ++ > 17 files changed, 1064 insertions(+), 797 deletions(-) > create mode 100644 controller/controller-utils.c > create mode 100644 pinctrl/automake.mk > create mode 100644 pinctrl/ovn-pinctrl.8.xml > create mode 100644 pinctrl/ovn-pinctrl.c > rename {controller => pinctrl}/pinctrl.c (84%) > rename {controller => pinctrl}/pinctrl.h (100%) >
Hi Mark, Thanks for the patch. We had a brief discussion during last OVN meeting. Let me put my points inlined. On Fri, Oct 18, 2019 at 1:43 PM Mark Michelson <mmichels@redhat.com> wrote: > > This proposes a set of patches to move pinctrl operations out of the > ovn-controller process and into its own. > > The main reasons for doing this are the following: > 1) Separating pinctrl makes it so that receiving a packet-in can't wake > up ovn-controller. To avoid waking up ovn-controller, it doesn't have to be in a separate process. A thread with its own OVSDB IDL to SB DB can achieve the same, as what this old patch did: https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332887.html However, the problem of a separate SB connection introduced the concern for scalability. There were discussions and thoughts for a separate thread without introducing new SB connection, but once the two threads share same SB connection, there has to be some synchronization between the threads that ends up waking up or blocking each other whenever there is a pinctrl processing that requires read/write SB data. The current multi-thread implementation from Numan is a trade off that avoids new SB connection but syncing with the main thread when SB data is needed. It is perfect for pinctrl handling that doesn't require SB data, and then wakes up ovn-controller for updating SB data. Today (2.12) there were improvements on both ovn-controller and OVSDB server, that alleviated the scale problems on both side. - For ovn-controller, with incremental processing, when there is no input change, it doesn't trigger flow recomputing, even when pinctrl wakes up the main thread. The major concern may be when main thread does need a recompute, it could block pinctrl processing for messages that requires SB data accessing, such as ARP handling. - For SB DB - Active-active cluster alleviates the burden of a single server and spread to 3 or 5. However, RAFT is not designed for scale. Write always happen on the leader node, and the cost of cluster sync between leader and follower becomes higher when number of nodes increases. - The fast-resync feature (requiring active-active clustered mode) avoids the slowness of data resync to all clients after DB restart/failover. However, it doesn't help if ovsdb-server is overloaded for regular updates and notifications during normal operations, given that it is single threaded. Also, there are corner cases that fast-resync doesn't help, e.g. when DB restart/failover happened just after a compress, when all the transaction history is lost. I'd suggest to reconsider these scalability concerns, the pros and cons for a dedicated SB connection for pinctrl, before moving forward to this approach. > 2) Separating pinctrl allows for manipulating the southbound database > directly while handling packets in, thus minimizing the need for storing > local copies of data This is true, but similar as point 1), it doesn't necessarily need a separate process. The point is whether pinctrl (thread or process) should use a dedicated SB connection. > 3) This lays the groundwork for an easier eventual conversion of > ovn-controller to DDlog, since the DDlog code would need to only handle > flow creation, not packet in handling. > Agree with this point. This is probably the most important benefit of separating pinctrl as a process. Although it is still possible to have pinctrl as a thread sharing SB connection while converting the flow processing part with DDlog, a separate process does make the conversion cleaner. In addition, a separate process introduces some operational costs, although not a big concern. The tooling like ovn-ctl and packaging also needs to be updated. Thanks, Han
Hi Han, I had some time to get back to this. See my comments below. On 10/21/19 4:01 PM, Han Zhou wrote: > Hi Mark, > > Thanks for the patch. We had a brief discussion during last OVN meeting. > Let me put my points inlined. > > On Fri, Oct 18, 2019 at 1:43 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > > > This proposes a set of patches to move pinctrl operations out of the > > ovn-controller process and into its own. > > > > The main reasons for doing this are the following: > > 1) Separating pinctrl makes it so that receiving a packet-in can't wake > > up ovn-controller. > > To avoid waking up ovn-controller, it doesn't have to be in a separate > process. A thread with its own OVSDB IDL to SB DB can achieve the same, > as what this old patch did: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332887.html > > However, the problem of a separate SB connection introduced the concern > for scalability. There were discussions and thoughts for a separate > thread without introducing new SB connection, but once the two threads > share same SB connection, there has to be some synchronization between > the threads that ends up waking up or blocking each other whenever there > is a pinctrl processing that requires read/write SB data. The current > multi-thread implementation from Numan is a trade off that avoids new SB > connection but syncing with the main thread when SB data is needed. It > is perfect for pinctrl handling that doesn't require SB data, and then > wakes up ovn-controller for updating SB data. I followed the discussions that resulted from the patch you sent. It looks like the concerns are that you either have to 1) Have two separate connections to the SB database, resulting in double the connections (this is what my patch does) 2) Have one connection to the SB database but synchronise the efforts of the different concerns of ovn-controller (namely logical flow processing and pinctrl processing). 1 is easy but resource intensive, and 2 is difficult but has the potential for not having the same bottlenecks. But this has me thinking. The current IDL code assumes that one IDL client == one database connection. I suppose it may be possible to alter the OVSDB IDL code so that a single connection could be shared by multiple IDL clients. In other words, you could create the SB connection, then create a separate thread. Each thread would then create an IDL that makes use of the same connection. The OVSDB client code would need to be altered to be able to notify multiple IDLs about changes. The client would also need to be modified to be thread safe, in the case that multiple threads want to write to the database at once. This would allow for multithreading, and the controller code wouldn't need to worry about synchronization. Each thread would have its own data it manipulates, and the synchronization would be handled by the IDL itself. Having said all this, though, it would not be a trivial task to implement this. And so the question becomes, is it worth it? > > Today (2.12) there were improvements on both ovn-controller and OVSDB > server, that alleviated the scale problems on both side. > - For ovn-controller, with incremental processing, when there is no > input change, it doesn't trigger flow recomputing, even when pinctrl > wakes up the main thread. The major concern may be when main thread does > need a recompute, it could block pinctrl processing for messages that > requires SB data accessing, such as ARP handling. > - For SB DB > - Active-active cluster alleviates the burden of a single server and > spread to 3 or 5. However, RAFT is not designed for scale. Write always > happen on the leader node, and the cost of cluster sync between leader > and follower becomes higher when number of nodes increases. > - The fast-resync feature (requiring active-active clustered mode) > avoids the slowness of data resync to all clients after DB > restart/failover. However, it doesn't help if ovsdb-server is overloaded > for regular updates and notifications during normal operations, given > that it is single threaded. Also, there are corner cases that > fast-resync doesn't help, e.g. when DB restart/failover happened just > after a compress, when all the transaction history is lost. > > I'd suggest to reconsider these scalability concerns, the pros and cons > for a dedicated SB connection for pinctrl, before moving forward to this > approach. > > > 2) Separating pinctrl allows for manipulating the southbound database > > directly while handling packets in, thus minimizing the need for storing > > local copies of data > > This is true, but similar as point 1), it doesn't necessarily need a > separate process. The point is whether pinctrl (thread or process) > should use a dedicated SB connection. Yep, you're definitely correct here. I guess my thought here was that if the two threads have no need to share any memory or state, then it makes more sense for them to exist as two processes instead. However, what I suggested above about multiple IDL clients sharing a connection would require that the code be multithreaded rather than multiprocess. > > > 3) This lays the groundwork for an easier eventual conversion of > > ovn-controller to DDlog, since the DDlog code would need to only handle > > flow creation, not packet in handling. > > > > Agree with this point. This is probably the most important benefit of > separating pinctrl as a process. Although it is still possible to have > pinctrl as a thread sharing SB connection while converting the flow > processing part with DDlog, a separate process does make the conversion > cleaner. > > In addition, a separate process introduces some operational costs, > although not a big concern. The tooling like ovn-ctl and packaging also > needs to be updated. If you look at the ovn-northd implementation of DDLog, you'll see that there is still a C process that controls everything. So it definitely could still work that for ovn-controller, the C program would create a separate thread for pinctrl and then perform the DDLog flow generation in the main thread. If we were to attempt to limit the SB DB connections but also allow for multiple processes, then you'd likely need to create a third process that actually performs database communications, and use IPC to communicate between this third process and the pinctrl and controller processes. I don't think this is any simpler than just having two threads in a single process. > > Thanks, > Han I'm going to back out with this change for the time being. I'm going to take a closer look at the IDL client code to see how feasible it would be to make it thread-safe and allow for multiple clients to share a single connection. No guarantee that I actually come forward with such a patch any time soon though :)
This proposes a set of patches to move pinctrl operations out of the ovn-controller process and into its own. The main reasons for doing this are the following: 1) Separating pinctrl makes it so that receiving a packet-in can't wake up ovn-controller. 2) Separating pinctrl allows for manipulating the southbound database directly while handling packets in, thus minimizing the need for storing local copies of data 3) This lays the groundwork for an easier eventual conversion of ovn-controller to DDlog, since the DDlog code would need to only handle flow creation, not packet in handling. This is an RFC. With this set of changes, item (2) above is not well addressed here. While the multithreading is removed from pinctrl, the structural components have not been altered. Were this idea to be approved, point (2) would be addressed when creating the final patch. Please share your thoughts. Mark Michelson (5): Separate pinctrl to its own process. Resolve duplicate functions in ovn-controller and ovn-pinctrl. Remove multithreading from pinctrl. Move ovn-pinctrl to its own directory. Flesh out manpage with more details about ovn-pinctrl Makefile.am | 1 + controller/automake.mk | 3 +- controller/binding.c | 22 +- controller/binding.h | 3 +- controller/controller-utils.c | 220 +++++++++++ controller/ovn-controller.c | 233 +----------- controller/ovn-controller.h | 20 + pinctrl/automake.mk | 25 ++ pinctrl/ovn-pinctrl.8.xml | 110 ++++++ pinctrl/ovn-pinctrl.c | 413 +++++++++++++++++++++ {controller => pinctrl}/pinctrl.c | 748 ++++++++++---------------------------- {controller => pinctrl}/pinctrl.h | 0 tests/automake.mk | 2 +- tests/ofproto-macros.at | 3 + tests/ovn.at | 13 +- tutorial/ovs-sandbox | 5 + utilities/ovn-ctl | 40 ++ 17 files changed, 1064 insertions(+), 797 deletions(-) create mode 100644 controller/controller-utils.c create mode 100644 pinctrl/automake.mk create mode 100644 pinctrl/ovn-pinctrl.8.xml create mode 100644 pinctrl/ovn-pinctrl.c rename {controller => pinctrl}/pinctrl.c (84%) rename {controller => pinctrl}/pinctrl.h (100%)