I've asked Justin to take a look at this series.
On Tue, Aug 14, 2018 at 08:48:25PM +0530, Anil Venkata wrote: > Gentle reminder requesting more reviews.. > > Mark has reviewed the patch series and Miguel has already tested the patch > series. > > Thanks > Anil > > On Tue, Aug 14, 2018 at 7:45 PM, Miguel Angel Ajo Pelayo < > [email protected]> wrote: > > > Thank you very much, > > > > I wasn't able to perform a proper code review, but you can add the > > Tested-By: Miguel Angel Ajo <[email protected]> > > > > 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 <[email protected]> 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 <[email protected] > >> > <mailto:[email protected]>> wrote: > >> > > >> > On 08/01/2018 08:16 AM, [email protected] > >> > <mailto:[email protected]> wrote: > >> > > >> > From: venkata anil <[email protected] > >> > <mailto:[email protected]>> > >> > > >> > 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 <[email protected] > >> > <mailto:[email protected]>> > >> > 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 <[email protected] > >> > <mailto:[email protected]>> > >> > --- > >> > > >> > 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 > >> [email protected] > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
