| Message ID | 1555432656-25077-1-git-send-email-ankur.sharma@nutanix.com |
|---|---|
| Headers | show |
| Series | OVN: Distributed Virtual Router for Vlan Backed Networks | expand |
On Tue, Apr 16, 2019 at 11:37:14PM +0000, Ankur Sharma wrote: > This series is about enhancing the logical router functionality in OVN to work > with vlan backed logical switches. This series has a lot of style and cosmetic issues reported by the robot. Please take a look at them. I also noticed that it tends to use 3-space indents in many places rather than the 4-space standard adopted in OVS. The patch titles should begin with "ovn: ". The commit messages for this series are written mostly as bullet points that in some cases just say in words the same thing that the patch does in code. This is not the usual style for OVS/OVN. While it is useful to say at a high level what a patch does, generally we expect commit messages to focus on rationale. I do really appreciate that the patches contain references to the overall goal of the series. The documentation should give more details. The documentation for network_type, for example, just says this: + <column name="network_type"> + <p> + Whether logical switch is VLAN backed or Overlay. + </p> + + <p> + Default is overlay. + </p> + </column> which formats in the manpage as: network_type: optional string, either overlay or vlan Whether logical switch is VLAN backed or Overlay. Default is overlay. So, the first paragraph just repeats what the manpage will say anyway in the autogenerated text, and thus it can be removed. The second paragraph is useful. But it leaves out all the important information on what are VLAN backed or overlay networks and how the user can decide which one is appropriate in a given situation. Maybe this is not the exact place where that should be explained--but, if not, then it should be explained somewhere else, such as the ovn-architecture document, and this should refer the reader to wherever that is. It looks like "ovn-nbctl ls-add" now will fail to work with older databases, since it sets the network_type unconditionally. I think it would be a good idea to have it set the network_type only if the user specifies one. (Maybe it should validate that it is "vlan" or "overlay"?) Thanks, Ben.
Hi Ben, I have submitted a V4 with following changes. a. More elaborate documentation and commit messages. b. Converged the patch set to 2 patches. Dividing them as for E-W and N-S traffic. c. Changes in nbctl to NOT to set network_type if not given in argument. d. Fixed the 3 space indentations. Please take a look. Appreciate your feedback. Regards, Ankur -----Original Message----- From: Ben Pfaff <blp@ovn.org> Sent: Wednesday, April 17, 2019 11:17 AM To: Ankur Sharma <ankur.sharma@nutanix.com> Cc: ovs-dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v3 0/4] OVN: Distributed Virtual Router for Vlan Backed Networks On Tue, Apr 16, 2019 at 11:37:14PM +0000, Ankur Sharma wrote: > This series is about enhancing the logical router functionality in OVN > to work with vlan backed logical switches. This series has a lot of style and cosmetic issues reported by the robot. Please take a look at them. I also noticed that it tends to use 3-space indents in many places rather than the 4-space standard adopted in OVS. The patch titles should begin with "ovn: ". The commit messages for this series are written mostly as bullet points that in some cases just say in words the same thing that the patch does in code. This is not the usual style for OVS/OVN. While it is useful to say at a high level what a patch does, generally we expect commit messages to focus on rationale. I do really appreciate that the patches contain references to the overall goal of the series. The documentation should give more details. The documentation for network_type, for example, just says this: + <column name="network_type"> + <p> + Whether logical switch is VLAN backed or Overlay. + </p> + + <p> + Default is overlay. + </p> + </column> which formats in the manpage as: network_type: optional string, either overlay or vlan Whether logical switch is VLAN backed or Overlay. Default is overlay. So, the first paragraph just repeats what the manpage will say anyway in the autogenerated text, and thus it can be removed. The second paragraph is useful. But it leaves out all the important information on what are VLAN backed or overlay networks and how the user can decide which one is appropriate in a given situation. Maybe this is not the exact place where that should be explained--but, if not, then it should be explained somewhere else, such as the ovn-architecture document, and this should refer the reader to wherever that is. It looks like "ovn-nbctl ls-add" now will fail to work with older databases, since it sets the network_type unconditionally. I think it would be a good idea to have it set the network_type only if the user specifies one. (Maybe it should validate that it is "vlan" or "overlay"?) Thanks, Ben.
This series is about enhancing the logical router functionality in OVN to work with vlan backed logical switches. Intial proposal was discused here: [1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-October/353066.html [2] https://docs.google.com/document/d/1uoQH478wM1OZ16HrxzbOUvk5LvFnfNEWbkPT6Zmm9OU/edit?usp=sharing This series covers following: a. L2: Associate a type with logical switches. Type value could be vlan or overlay. b. L3 E-W: In the absence of encapsulation, we cannot use router port mac as source mac (since it is distributed), hence replace the same with a chassis unique mac. c. L3 N-S: Use gateway-chassis construct to respond to ARP requests for router port, so that it becomes entry point for all chassis bound traffic coming from "external" network. Some additional changes, like no need to redirect south to north traffic in the absence of NAT etc. This series does not cover following: (will be sent out for review in follow up series once this series is reviewed/committed) a. Network Address Translation. b. Ensuring VMs mac is learnt in underlay network to avoid flooding of L3 flows. Ankur Sharma (4): L3 E-W Support in ovn, replace router port mac with chassis mac. L3 N-S support in ovn, advertise router port mac from gateway chassis L3 N-S support in ovn, do not replace router port mac on gateway chassis L3 N-S support in ovn, avoid chassis redirection as default for vlan backed networks ovn/controller/binding.c | 12 +- ovn/controller/chassis.c | 66 ++++- ovn/controller/chassis.h | 4 + ovn/controller/ovn-controller.8.xml | 7 + ovn/controller/ovn-controller.c | 2 +- ovn/controller/ovn-controller.h | 5 +- ovn/controller/physical.c | 116 +++++++++ ovn/controller/pinctrl.c | 200 ++++++++++++-- ovn/controller/pinctrl.h | 6 + ovn/lib/ovn-util.c | 31 +++ ovn/lib/ovn-util.h | 6 + ovn/northd/ovn-northd.c | 83 +++++- ovn/ovn-architecture.7.xml | 12 + ovn/ovn-nb.ovsschema | 8 +- ovn/ovn-nb.xml | 9 + ovn/ovn-sb.xml | 15 ++ ovn/utilities/ovn-nbctl.c | 45 +++- tests/ovn-nbctl.at | 48 +++- tests/ovn-northd.at | 22 ++ tests/ovn.at | 502 ++++++++++++++++++++++++++++++++++++ 20 files changed, 1127 insertions(+), 72 deletions(-)