On 2/20/19 11:12 PM, Numan Siddique wrote:
Thanks for the review and comments.
Please see below for few comments.
Thanks
Numan
On Thu, Feb 21, 2019 at 3:29 AM Mark Michelson <[email protected]
<mailto:[email protected]>> wrote:
Hi Numan,
This is quite a large patchset, but I believe I understand it. It
essentially does two things:
1) Deprecates Gateway_Chassis in favor of HA_Chassis_Groups and
HA_Chassis. The benefit of HA_Chassis_Groups is that it allows for
automatic failover of chassis using BFD.
The existing Gateway_Chassis also supports automatic failover using BFD.
(1) actully doesn't add any new extra functionality to support HA for
gateway router ports.
It only provides a different way to do the same HA. (1) maintains full
backward compatibility so
that CMS can still use Gateway_Chassis in NB DB and ovn-northd will
create HA_Chassis_Group
in SB DB instead of creating Gateway_Chassis in SB DB.
2) Adds external logical switch port type that makes use of
HA_Chassis_Groups to allow for services (such as DHCP) to be offered on
ports outside of the OVN environment. This uses the
HA_Chassis_Groups in
order to "bind" the external port to a specific instance of
ovn-controller.
The problem I'm having is that I don't really understand what value (1)
is adding. It seems like it's just changing the syntax behind the
existing Gateway_Chassis. You can already specify multiple
Gateway_Chassis for a logical router port, and you can associate a
priority with a Gateway_Chassis, allowing for controlled failover. And
it seems like you could do the same thing for (2) as well. Am I missing
something?
The present Gateway_Chassis approach is very much tied to the logical
router ports.
And I felt odd to associate a set of Gateway_Chassis to 'external'
logical ports.
Right now the association of the logical_router_port to Gateway_Chassis
in NB DB
is a strong reference.
If we want to use the existing Gateway_Chassis , we need to add a column
'gateway_chassis' in
Logical_switch_port. If suppose there are like 10 'external' ports in a
logical switch, CMS has to create 10 sets
of Gateway_Chassis rows (with each set representing few chassis which
provides gateway
functionality) and associate each set to the logical_switch_port.
Right now Gateway_Chassis table in the schema doesn't have 'is_Root' set
to True.
So I think using the present Gateway_Chassis table for the HA of
'external' ports would be more complicated
for CMS and also semantically it feels odd for a logical port to have
Gatway_Chassis associated to it.
That's the reason I thought I would add a generic table which could be
used by both distributed logical router ports
and 'external' ports.
This patch series also simplifies the code to establish BFD tunnels. I
felt the present
code [1] which does this is quite complicated.
Please let me know your thoughts and and if you have any more questions.
Thanks for the explanation Numan. Everything you've brought up sounds
good to me. I especially like the idea of being able to update a
HA_Chassis_Group instead of having to update lots of individual switch
or router ports.
[1] -
https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L210
https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L91
As far as individual critiques of the code, I would recommend
double-checking the spelling of "chassis" throughout the code.
Sometimes
it's "chassi" and other times it's "chasss". Also, in
ovn/controller/physical.c, I noticed that the "Gateway_Chassis" is
still
referenced in an error message instead of HA_Chassis. Other than
that, I
didn't see anything that was jumped out as wrong.
Thanks I will address that.
Numan
On 2/18/19 2:26 PM, [email protected] <mailto:[email protected]>
wrote:
> From: Numan Siddique <[email protected]
<mailto:[email protected]>>
>
> This patch series adds a generic HA chassis group support in OVN
> deprecating the existing Gateway chassis support. The final patch
> of the series adds the 'external' port support in OVN.
> The 'external' port patch addresses the review comments from Han Zhou
> which he provided when 'external' port patch was submitted without
> the HA support.
>
> A generic HA chassis group support is added so that both the
distributed
> logical router ports (providing gateway functionality) and 'external'
> ports can use it for HA and also to simplify the existing HA code
> (which seems to be a bit complicated).
>
> To support HA, BFD is configured on tunnel ports. And even though
> 'external' ports are expected to be used with the logical
> switches having localnet ports (representing physical networks),
> BFD is used for now since each chassis uses geneve tunnels with
> all other chassis in the OVN cluster.
>
> Numan Siddique (5):
> ovn-northd: Reuse the hmaps - datapaths and ports in
ovnsb_db_run()
> ovn: Add generic HA chassis group
> ovn-controller: Make use of ha_chassis_group table to bind the
> chassisredirect ports
> ovn-northd: Delete the references to gateway_chasss in SB DB
> ovn: Support a new Logical_Switch_Port.type - 'external'
>
> NEWS | 3 +
> ovn/controller/automake.mk <http://automake.mk> | 4 +-
> ovn/controller/bfd.c | 229 +++----
> ovn/controller/bfd.h | 11 +-
> ovn/controller/binding.c | 31 +-
> ovn/controller/binding.h | 1 -
> ovn/controller/gchassis.c | 222 -------
> ovn/controller/gchassis.h | 71 ---
> ovn/controller/ha-chassis.c | 203 ++++++
> ovn/controller/ha-chassis.h | 50 ++
> ovn/controller/lflow.c | 29 +-
> ovn/controller/lflow.h | 3 +-
> ovn/controller/ovn-controller.c | 14 +-
> ovn/controller/physical.c | 109 ++--
> ovn/controller/physical.h | 3 +-
> ovn/controller/pinctrl.c | 38 +-
> ovn/controller/pinctrl.h | 1 -
> ovn/lib/chassis-index.c | 26 +
> ovn/lib/chassis-index.h | 4 +
> ovn/lib/ovn-util.c | 1 +
> ovn/northd/ovn-northd.8.xml | 37 +-
> ovn/northd/ovn-northd.c | 779 ++++++++++++++++-------
> ovn/ovn-architecture.7.xml | 71 +++
> ovn/ovn-nb.ovsschema | 42 +-
> ovn/ovn-nb.xml | 132 ++++
> ovn/ovn-sb.ovsschema | 43 +-
> ovn/ovn-sb.xml | 63 ++
> ovn/utilities/ovn-nbctl.8.xml | 41 ++
> ovn/utilities/ovn-nbctl.c | 221 +++++++
> ovn/utilities/ovn-sbctl.c | 6 +
> tests/ovn-northd.at <http://ovn-northd.at> | 396
+++++++++++-
> tests/ovn.at <http://ovn.at> | 1031
++++++++++++++++++++++++++++++-
> 32 files changed, 3061 insertions(+), 854 deletions(-)
> delete mode 100644 ovn/controller/gchassis.c
> delete mode 100644 ovn/controller/gchassis.h
> create mode 100644 ovn/controller/ha-chassis.c
> create mode 100644 ovn/controller/ha-chassis.h
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
