Thank you very much, I wasn't able to perform a proper code review, but you can add the Tested-By: Miguel Angel Ajo <majop...@redhat.com>
The issue I described previously seems to be fixed on the v7 of the series. On Mon, Aug 6, 2018 at 9:19 PM Mark Michelson <mmich...@redhat.com> wrote: > On 08/06/2018 01:58 PM, Anil Venkata wrote: > > Thanks Mark. Kindly look at my comment inline. > > > > On Fri, Aug 3, 2018 at 2:17 AM, Mark Michelson <mmich...@redhat.com > > <mailto:mmich...@redhat.com>> wrote: > > > > On 08/01/2018 08:16 AM, vkomm...@redhat.com > > <mailto:vkomm...@redhat.com> wrote: > > > > From: venkata anil <vkomm...@redhat.com > > <mailto:vkomm...@redhat.com>> > > > > Previous patches in the series doesn't address issue 1 explained > > in [1] > > i.e > > 1) removal of router gateway port MAC address on external > switches > > after expiring of aging time. > > 2) then external switches unable to learn the gateway MAC as > > reply packets carry router internal port MAC address as > source > > > > To fix this, router on gateway node will use router gateway MAC > > address > > instead of router internal port MAC address as source for reply > > packets, > > so that external switches can learn gateway MAC address. > > This is done only for reply packets from router gateway to > > tenant VLAN > > switch ports. > > Later before delivering the packet to the port, ovn-controller > will > > replace the gateway MAC with router internal port MAC in table > 33. > > > > [1] > > //mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html > > < > http://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html> > > > > Reported-by: Miguel Angel Ajo <majop...@redhat.com > > <mailto:majop...@redhat.com>> > > Reported-at: > > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html > > < > https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349803.html> > > Signed-off-by: Venkata Anil <vkomm...@redhat.com > > <mailto:vkomm...@redhat.com>> > > --- > > > > v6->v7: > > * Added this patch > > > > > > ovn/controller/physical.c | 60 > > ++++++++++++++++++++++++++++++++++++++++++--- > > ovn/northd/ovn-northd.8.xml | 10 ++++++++ > > ovn/northd/ovn-northd.c | 29 ++++++++++++++++++++++ > > ovn/ovn-architecture.7.xml | 4 ++- > > 4 files changed, 99 insertions(+), 4 deletions(-) > > > > diff --git a/ovn/controller/physical.c > b/ovn/controller/physical.c > > index f269a1d..1f41f59 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -190,7 +190,9 @@ get_zone_ids(const struct sbrec_port_binding > > *binding, > > static void > > put_local_common_flows(uint32_t dp_key, uint32_t port_key, > > bool nested_container, const struct > > zone_ids *zone_ids, > > - struct ofpbuf *ofpacts_p, struct hmap > > *flow_table) > > + struct ofpbuf *ofpacts_p, struct hmap > > *flow_table, > > + struct local_datapath *ld, > > + const struct hmap *local_datapaths) > > { > > struct match match; > > @@ -221,11 +223,63 @@ put_local_common_flows(uint32_t dp_key, > > uint32_t port_key, > > } > > } > > + struct ofpbuf *clone = NULL; > > + clone = ofpbuf_clone(ofpacts_p); > > + > > > > > > I don't understand the use of the cloned ofpbuf here. You could just > > ofpbuf_clear(ofpacts_p) in each iteration of the for loop below and > > reuse ofpacts_p where you've used clone below. > > > > > > > > > > I need to add additional flow with priority 150 along with default flow > > which exists with prioirty 100. > > > > 1) table=33, priority=100,reg15=0x5,metadata=0x2 > > > actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],resubmit(,34) > > 2) table=33, > > priority=150,reg15=0x5,metadata=0x2,dl_src=fa:16:3e:48:66:e7 > > > actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],mod_dl_src:fa:16:3e:65:f2:ae,resubmit(,34) > > > > The first flow with priority 100 is the default flow. This flow is added > > for each output port residing on the current chassis, sets some > > registers and resubmits to table 34. > > But when the packet has router gateway MAC as source MAC address, I want > > to replace the MAC before resubmitting to table 34. Second flow has same > > match conditions as first flow and also same actions, additionally it > > checks for the source MAC and then modifies the source MAC address. > > > > Before I construct the second flow, I have the registers and resubmit as > > part of actions i.e > > > actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],resubmit(,34) > > > > If I simply add MAC to actions with put_load(mac64, MFF_ETH_SRC, 0, 48, > > ofpacts_p); then actions in 2nd flow(with 150 priority) will look like > > > actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],resubmit(,34),mod_dl_src:fa:16:3e:65:f2:ae > > i.e modifying the MAC will happen after resubmit, which is not the > > expected behaviour. > > > > So I am cloning the ofpacts_p before resubmit is added to it and using > > the cloned one in constructing flow with priority 150. > > With this changes, actions will look like > > > actions=load:0x2->NXM_NX_REG13[],load:0xc->NXM_NX_REG11[],load:0xb->NXM_NX_REG12[],mod_dl_src:fa:16:3e:65:f2:ae,resubmit(,34) > > Thank you very much for the explanation. It makes a lot more sense now. > > > > > > > /* Resubmit to table 34. */ > > put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p); > > ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, 0, > > &match, ofpacts_p); > > + /* For a reply packet from gateway with VLAN switch port > > as destination > > + * (excluding localnet_port and external VLAN networks), > > gateway router > > + * will use gateway MAC address as source MAC instead of > > router internal > > + * port MAC, so that external switches can learn gateway > > MAC address. > > + * Here (before packet is given to the port) we replace > > router gateway > > + * MAC address with router internal port MAC. */ > > + if (ld->localnet_port && (port_key != > > ld->localnet_port->tunnel_key)) { > > + for (int i = 0; i < ld->n_peer_dps; i++) { > > + struct local_datapath *peer_ldp = > get_local_datapath( > > + local_datapaths, > > ld->peer_dps[i]->peer_dp->tunnel_key); > > + const struct sbrec_port_binding *crp; > > + crp = peer_ldp->chassisredirect_port; > > + if (!crp) { > > + continue; > > + } > > + > > + if (strcmp(smap_get(&crp->options, > "distributed-port"), > > + ld->peer_dps[i]->peer->logical_port) && > > + (port_key != > ld->peer_dps[i]->patch->tunnel_key)) { > > + for (int j = 0; j < crp->n_mac; j++) { > > + struct lport_addresses laddrs; > > + if (!extract_lsp_addresses(crp->mac[j], > > &laddrs)) { > > + continue; > > + } > > + match_set_dl_src(&match, laddrs.ea); > > + destroy_lport_addresses(&laddrs); > > + break; > > + } > > + for (int j = 0; j < > > ld->peer_dps[i]->peer->n_mac; j++) { > > + struct lport_addresses laddrs; > > + uint64_t mac64; > > + if (!extract_lsp_addresses( > > + ld->peer_dps[i]->peer->mac[j], > &laddrs)) { > > + continue; > > + } > > + mac64 = eth_addr_to_uint64(laddrs.ea); > > + put_load(mac64, > > + MFF_ETH_SRC, 0, 48, clone); > > + destroy_lport_addresses(&laddrs); > > + break; > > + } > > + put_resubmit(OFTABLE_CHECK_LOOPBACK, clone); > > + ofctrl_add_flow(flow_table, > > OFTABLE_LOCAL_OUTPUT, 150, 0, > > + &match, clone); > > + } > > + } > > + } > > + ofpbuf_delete(clone); > > + > > /* Table 34, Priority 100. > > * ======================= > > * > > @@ -330,7 +384,7 @@ consider_port_binding(struct ovsdb_idl_index > > *sbrec_chassis_by_name, > > struct zone_ids binding_zones = > > get_zone_ids(binding, ct_zones); > > put_local_common_flows(dp_key, port_key, false, > > &binding_zones, > > - ofpacts_p, flow_table); > > + ofpacts_p, flow_table, ld, > > local_datapaths); > > match_init_catchall(&match); > > ofpbuf_clear(ofpacts_p); > > @@ -531,7 +585,7 @@ consider_port_binding(struct ovsdb_idl_index > > *sbrec_chassis_by_name, > > struct zone_ids zone_ids = get_zone_ids(binding, > > ct_zones); > > put_local_common_flows(dp_key, port_key, > > nested_container, &zone_ids, > > - ofpacts_p, flow_table); > > + ofpacts_p, flow_table, ld, > > local_datapaths); > > /* Table 0, Priority 200, 150 and 100. > > * ============================== > > diff --git a/ovn/northd/ovn-northd.8.xml > > b/ovn/northd/ovn-northd.8.xml > > index 8fa5272..876c121 100644 > > --- a/ovn/northd/ovn-northd.8.xml > > +++ b/ovn/northd/ovn-northd.8.xml > > @@ -2013,6 +2013,16 @@ next; > > </li> > > <li> > > + A priority-100 logical flow with match > > + <code>inport == <var>GW</var> && > > + flags.rcv_from_vlan == 1</code> has actions > > + <code>eth.dst = <var>E</var>; next;</code>, where > > + <var>GW</var> is the logical router distributed gateway > > + port and <var>E</var> is the MAC address of router > > + distributed gateway port. > > + </li> > > + > > + <li> > > For each NAT rule in the OVN Northbound database that > can > > be handled in a distributed manner, a priority-100 > > logical > > flow with match <code>ip4.src == <var>B</var> > && > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index bcf0b66..d012bb8 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -4419,6 +4419,15 @@ add_route(struct hmap *lflows, const > > struct ovn_port *op, > > } else { > > ds_put_format(&actions, "ip%s.dst", is_ipv4 ? "4" : > "6"); > > } > > + > > + if (op->peer && op->peer->od->localnet_port && > > + op->od->l3dgw_port && op->od->l3redirect_port && > > + (op != op->od->l3redirect_port) && > > + (op != op->od->l3dgw_port)) { > > + ds_put_format(&match, " && is_chassis_resident(%s)", > > + op->od->l3redirect_port->json_key); > > + ds_put_format(&actions, "; flags.rcv_from_vlan = 1"); > > + } > > ds_put_format(&actions, "; " > > "%sreg1 = %s; " > > "eth.src = %s; " > > @@ -6131,6 +6140,26 @@ build_lrouter_flows(struct hmap > > *datapaths, struct hmap *ports, > > > op->lrp_networks.ipv6_addrs[i].network_s, > > op->lrp_networks.ipv6_addrs[i].plen, > > NULL, NULL); > > } > > + > > + /* For a reply packet from gateway with VLAN switch > port as > > + * destination, replace router internal port MAC with > > router gateway > > + * MAC address, so that external switches can learn > > gateway MAC > > + * address. Later before delivering the packet to the > port, > > + * controller will replace the gateway MAC with router > > internal port > > + * MAC in table 33. */ > > + if (op->od->l3dgw_port && (op == op->od->l3dgw_port) && > > + op->od->l3redirect_port) { > > + ds_clear(&actions); > > + ds_clear(&match); > > + ds_put_format(&match, "inport == %s", op->json_key); > > + ds_put_format(&match, " && flags.rcv_from_vlan == > 1"); > > + ds_put_format(&match, " && is_chassis_resident(%s)", > > + op->od->l3redirect_port->json_key); > > + ds_put_format(&actions, > > + "eth.src = %s; next;", > > op->lrp_networks.ea_s); > > + ovn_lflow_add(lflows, op->od, > > S_ROUTER_IN_GW_REDIRECT, 100, > > + ds_cstr(&match), ds_cstr(&actions)); > > + } > > } > > /* Convert the static routes to flows. */ > > diff --git a/ovn/ovn-architecture.7.xml > b/ovn/ovn-architecture.7.xml > > index ad2101c..0de41d2 100644 > > --- a/ovn/ovn-architecture.7.xml > > +++ b/ovn/ovn-architecture.7.xml > > @@ -1067,7 +1067,9 @@ > > <p> > > Flows in table 33 resemble those in table 32 but for > > logical ports that > > - reside locally rather than remotely. For unicast > > logical output ports > > + reside locally rather than remotely. If these are VLAN > > ports and > > + packet has router gateway port MAC address as source, > > replace it with > > + router internal port MAC address. For unicast logical > > output ports > > on the local hypervisor, the actions just resubmit to > > table 34. For > > multicast output ports that include one or more > > logical ports on the > > local hypervisor, for each such logical port > > <var>P</var>, the actions > > > > > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev