| Message ID | 2c0c3723cad1ad74b754a514cf6cd8a57bfd10cb.1584022333.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] northd: do not insert remote FIP entries in S_ROUTER_IN_ARP_RESOLVE stage | expand |
On Thu, Mar 12, 2020 at 7:43 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > Do not insert FIP entries in S_ROUTER_IN_ARP_RESOLVE stage if the > destination is not resident on the local chassis since the the L2 > address will be resolved by the arp action Hi Lorenzo, If eth.dst for the FIP can be resolved using the arp action, then is there a need to add these flows at all ? If so, we can probably just remove these lflows from the S_ROUTER_IN_ARP_RESOLVE stage right ? Right now for N dnat_and_snat entries defined for a logical router we are adding N logical flows in S_ROUTER_IN_ARP_RESOLVE stage. Even after this patch we would still do that. Just that these lflows will not be installed on other chassis. I don't see much benefit in this patch. I think it'll add benefit if we remove these lflows altogether. what do you think? Thanks Numan > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > northd/ovn-northd.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 921fe1865..16710c6cd 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -8847,6 +8847,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > &match, "outport == %s && %s == %s", > od->l3dgw_port->json_key, > is_v6 ? "xxreg0" : "reg0", nat->external_ip); > + if (distributed) { > + ds_put_format(&match, > + " && is_chassis_resident(\"%s\")", > + nat->logical_port); > + } > ds_clear(&actions); > ds_put_format( > &actions, "eth.dst = %s; next;", > -- > 2.24.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Thu, Mar 12, 2020 at 7:43 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > Do not insert FIP entries in S_ROUTER_IN_ARP_RESOLVE stage if the > > destination is not resident on the local chassis since the the L2 > > address will be resolved by the arp action > > Hi Lorenzo, > Hi Numan, thx for the review. > If eth.dst for the FIP can be resolved using the arp action, then is > there a need to add these > flows at all ? If so, we can probably just remove these lflows from > the S_ROUTER_IN_ARP_RESOLVE > stage right ? there is just one case where we need these flows, when both FIPs are on the same hv (e.g ovn -- DNAT and SNAT on distributed router - E/W in system-ovn.at). In this case the arp action can't resolve the L2 address. So IIUC we can't remove them. Do you agree? Regards, Lorenzo > > Right now for N dnat_and_snat entries defined for a logical router > we are adding N logical flows in S_ROUTER_IN_ARP_RESOLVE stage. Even > after this patch we would > still do that. Just that these lflows will not be installed on other > chassis. I don't see much benefit in this patch. > > I think it'll add benefit if we remove these lflows altogether. what > do you think? > > Thanks > Numan > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > northd/ovn-northd.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 921fe1865..16710c6cd 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -8847,6 +8847,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > > &match, "outport == %s && %s == %s", > > od->l3dgw_port->json_key, > > is_v6 ? "xxreg0" : "reg0", nat->external_ip); > > + if (distributed) { > > + ds_put_format(&match, > > + " && is_chassis_resident(\"%s\")", > > + nat->logical_port); > > + } > > ds_clear(&actions); > > ds_put_format( > > &actions, "eth.dst = %s; next;", > > -- > > 2.24.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 921fe1865..16710c6cd 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -8847,6 +8847,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, &match, "outport == %s && %s == %s", od->l3dgw_port->json_key, is_v6 ? "xxreg0" : "reg0", nat->external_ip); + if (distributed) { + ds_put_format(&match, + " && is_chassis_resident(\"%s\")", + nat->logical_port); + } ds_clear(&actions); ds_put_format( &actions, "eth.dst = %s; next;",
Do not insert FIP entries in S_ROUTER_IN_ARP_RESOLVE stage if the destination is not resident on the local chassis since the the L2 address will be resolved by the arp action Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/ovn-northd.c | 5 +++++ 1 file changed, 5 insertions(+)