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 < majop...@redhat.com> 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 <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
