Re: [ovs-dev] [RFC ovn PATCH 0/5] Separate pinctrl to its own process

[Mark Michelson]

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 > 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 

Re: [ovs-dev] [RFC ovn PATCH 0/5] Separate pinctrl to its own process

[Han Zhou]

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  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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC ovn PATCH 0/5] Separate pinctrl to its own process

[Mark Michelson]

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%)



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC ovn PATCH 0/5] Separate pinctrl to its own process

[Ben Pfaff]

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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [RFC ovn PATCH 0/5] Separate pinctrl to its own process

[Mark Michelson]

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%)

-- 
2.14.5

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev