Thanks Ben On Wed, Aug 15, 2018 at 6:40 AM, Ben Pfaff <b...@ovn.org> wrote:
> 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 < > > 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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev