On Fri, Oct 7, 2016 at 5:25 PM, Han Zhou <zhou...@gmail.com> wrote: > Overall it looks good to me. Just suggestions for rewording. > > On Thu, Oct 6, 2016 at 10:34 AM, Darrell Ball <dlu...@gmail.com> wrote: > > > > There has been enough confusion regarding logical switch datapath > > arp responders in ovn to warrant some additional comments; > > hence add a general description regarding why they exist and > > document the special cases. > > > > Signed-off-by: Darrell Ball <dlu...@gmail.com> > > --- > > > > Note this patch is meant to be merge with the code change for vtep > > inport handling here. > > https://patchwork.ozlabs.org/patch/675796/ > > > > v3->v4: Capitalization fixes. > > Reinstate comment regarding L2 learning confusion. > > > > v2->v3: Reword and further elaborate. > > > > v1->v2: Dropped RFC code change for logical switch router > > type ports > > > > ovn/northd/ovn-northd.8.xml | 52 ++++++++++++++++++++++++++++++ > +++++++++------ > > 1 file changed, 46 insertions(+), 6 deletions(-) > > > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml > > index 77eb3d1..5ac351d 100644 > > --- a/ovn/northd/ovn-northd.8.xml > > +++ b/ovn/northd/ovn-northd.8.xml > > @@ -415,20 +415,60 @@ > > <h3>Ingress Table 9: ARP/ND responder</h3> > > > > <p> > > - This table implements ARP/ND responder for known IPs. > > To avoid confusing with lrouter ARP responder, it is better to be: this > table implements ARP/ND responder in *logical switch*, for known IPs. >
This section is under "Logical Switch Datapaths", so the context is should be clear that it refers to "Logical Switch" but it you are confused, I can add a redundant reference to Logical Switches - it is just a few words. > > > + ARP requests received by multiple hypervisors, as in the case of > > + <code>localnet</code> and <code>vtep</code> logical inports need > > + to skip these logical switch ARP responders; the reason being > > + that northd downloads the same mac binding rules to all > hypervisors > > + and all hypervisors will receive the ARP request from the external > > + network and respond. This will confuse L2 learning on the source > > + of the ARP requests. These skip rules are mentioned under > > + priority-100 flows. > > Suggest to avoid this detail here, but just describe in the priority-100 > flow section. Too much details here adds redundancy and not helpful to > overall understanding. For example, there is also another case to skip > which is when IP is owned by the inport, and it is described separately > already. > I am fine with moving it back to the priority-100 flow sub-section. > > > + ARP requests arrive from VMs with a > logical > > + switch inport type of type empty, which is the default. For this > > + case, > > This part of text is correct, but I feel it doesn't help understanding but > add confusion, so suggest to remove. > I would like to keep it as I found it useful; I will remove the "empty" and keep default for type. > > > + If this logical switch router type port does not have an > > + IP address configured, ARP requests will hit another ARP responder > > + on the logical router datapath itself, which is most commonly a > > + distributed logical router. The advantage of using the logical > > + switch proxy ARP rule for logical router ports is that this rule > > + is hit before the logical switch L2 broadcast rule. This means > > + the ARP request is not broadcast on this logical switch. > > How about: > > If this logical switch router type port does not have an IP address > configured, although the request will still be responded by the ARP > responder on the logical router datapath, the ARP request will be broadcast > on the logical switch. > I can reword the concept folding in part of your shorter wording. > > > + Note that ARP requests > received from > > + <code>localnet</code> or <code>vtep</code> logical inports can > > + either go directly to VMs, in which case the VM responds or can > > + hit an ARP responder for a logical router port if the packet is > > + used to resolve a logical router port next hop address. > > This part of text is somehow redundant with the above. If this is to be > emphasised, we may put it under the section for router ARP responder flows > description, to explain why we still need ARP responder for on router > datapath. > I am ok to omit this part for now. I thought it was obvious - I only had added it to clarify a misleading feedback I had previously received to set that straight. > > > + The > inport being of type > > + <code>router</code> has no known use case for these ARP > > + responders. However, no skip flows are installed for these > > + packets, as there would be some additional flow cost for this > > + and the value appears limited. > > Maybe we don't even need to mention this if we don't want to skip this > kind of packet. If there is no such case, it means we don't need to add > flows to skip it; if there is such case, but we want the ARP responder to > work for that case, we don't need to a flows to skip either. > Since I plan to revisit the logical switch router type arp responders in general I think we can skip this for now. > > > Acked-by: Han Zhou <zhou...@gmail.com> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
