On Fri, Jan 18, 2019 at 2:11 AM Han Zhou <[email protected]> wrote: > On Thu, Jan 17, 2019 at 11:32 AM Han Zhou <[email protected]> wrote: > > > > On Thu, Jan 17, 2019 at 11:25 AM Numan Siddique <[email protected]> > wrote: > > > > > > > > > > > > On Fri, Jan 18, 2019 at 12:21 AM Han Zhou <[email protected]> wrote: > > >> > > >> Hi Numan, > > >> > > >> With v5 the new test case "external logical port" fails. > > >> And please see more comments inlined. > > >> > > >> On Tue, Jan 15, 2019 at 12:09 PM <[email protected]> wrote: > > >> > > > >> > From: Numan Siddique <[email protected]> > > >> > > > >> > In the case of OpenStack + OVN, when the VMs are booted on > > >> > hypervisors supporting SR-IOV nics, there are no OVS ports > > >> > for these VMs. When these VMs sends DHCPv4, DHPCv6 or IPv6 > > >> > Router Solicitation requests, the local ovn-controller > > >> > cannot reply to these packets. OpenStack Neutron dhcp agent > > >> > service needs to be run to serve these requests. > > >> > > > >> > With the new logical port type - 'external', OVN itself can > > >> > handle these requests avoiding the need to deploy any > > >> > external services like neutron dhcp agent. > > >> > > > >> > To make use of this feature, CMS has to > > >> > - create a logical port for such VMs > > >> > - set the type to 'external' > > >> > - set requested-chassis="<chassis-name>" in the options > > >> > column. > > >> > - create a localnet port for the logical switch > > >> > - configure the ovn-bridge-mappings option in the OVS db. > > >> > > > >> > When the ovn-controller running in that 'chassis', detects > > >> > the Port_Binding row, it adds the necessary DHCPv4/v6 OF > > >> > flows. Since the packet enters the logical switch pipeline > > >> > via the localnet port, the inport register (reg14) is set > > >> > to the tunnel key of localnet port in the match conditions. > > >> > > > >> > In case the chassis goes down for some reason, it is the > > >> > responsibility of CMS to change the 'requested-chassis' > > >> > option to some other active chassis, so that it can serve > > >> > these requests. > > >> > > > >> > When the VM with the external port, sends an ARP request for > > >> > the router ips, only the chassis which has claimed the port, > > >> > will reply to the ARP requests. Rest of the chassis on > > >> > receiving these packets drop them in the ingress switch > > >> > datapath stage - S_SWITCH_IN_EXTERNAL_PORT which is just > > >> > before S_SWITCH_IN_L2_LKUP. > > >> > > > >> > This would guarantee that only the chassis which has claimed > > >> > the external ports will run the router datapath pipeline. > > >> > > > >> > Signed-off-by: Numan Siddique <[email protected]> > > >> > --- > > >> > > > >> > v4 -> v5 > > >> > ------ > > >> > * Addressed review comments from Han Zhou. > > >> > > > >> > v3 -> v4 > > >> > ------ > > >> > * Updated the documention as per Han Zhou's suggestion. > > >> > > > >> > v2 -> v3 > > >> > ------- > > >> > * Rebased > > >> > > > >> > ovn/controller/binding.c | 12 + > > >> > ovn/controller/lflow.c | 41 ++- > > >> > ovn/controller/lflow.h | 2 + > > >> > ovn/controller/lport.c | 26 ++ > > >> > ovn/controller/lport.h | 5 + > > >> > ovn/controller/ovn-controller.c | 6 + > > >> > ovn/lib/ovn-util.c | 1 + > > >> > ovn/northd/ovn-northd.8.xml | 37 ++- > > >> > ovn/northd/ovn-northd.c | 85 ++++- > > >> > ovn/ovn-architecture.7.xml | 78 +++++ > > >> > ovn/ovn-nb.xml | 47 +++ > > >> > tests/ovn.at | 530 > +++++++++++++++++++++++++++++++- > > >> > 12 files changed, 848 insertions(+), 22 deletions(-) > > >> > > > >> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > > >> > index 021ecddcf..64e605b92 100644 > > >> > --- a/ovn/controller/binding.c > > >> > +++ b/ovn/controller/binding.c > > >> > @@ -471,6 +471,18 @@ consider_local_datapath(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > >> > * for them. */ > > >> > sset_add(local_lports, binding_rec->logical_port); > > >> > our_chassis = false; > > >> > + } else if (!strcmp(binding_rec->type, "external")) { > > >> > + const char *chassis_id = smap_get(&binding_rec->options, > > >> > + "requested-chassis"); > > >> > + our_chassis = chassis_id && ( > > >> > + !strcmp(chassis_id, chassis_rec->name) || > > >> > + !strcmp(chassis_id, chassis_rec->hostname)); > > >> > + if (our_chassis) { > > >> > + add_local_datapath(sbrec_datapath_binding_by_key, > > >> > + sbrec_port_binding_by_datapath, > > >> > + sbrec_port_binding_by_name, > > >> > + binding_rec->datapath, true, > local_datapaths); > > >> > + } > > >> > } > > >> > > > >> > if (our_chassis > > >> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > >> > index 8db81927e..98e8ed3b9 100644 > > >> > --- a/ovn/controller/lflow.c > > >> > +++ b/ovn/controller/lflow.c > > >> > @@ -52,7 +52,10 @@ lflow_init(void) > > >> > struct lookup_port_aux { > > >> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath; > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type; > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key; > > >> > const struct sbrec_datapath_binding *dp; > > >> > + const struct sbrec_chassis *chassis; > > >> > }; > > >> > > > >> > struct condition_aux { > > >> > @@ -66,6 +69,8 @@ static void consider_logical_flow( > > >> > struct ovsdb_idl_index *sbrec_chassis_by_name, > > >> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > >> > const struct sbrec_logical_flow *, > > >> > const struct hmap *local_datapaths, > > >> > const struct sbrec_chassis *, > > >> > @@ -89,8 +94,24 @@ lookup_port_cb(const void *aux_, const char > *port_name, unsigned int *portp) > > >> > const struct sbrec_port_binding *pb > > >> > = lport_lookup_by_name(aux->sbrec_port_binding_by_name, > port_name); > > >> > if (pb && pb->datapath == aux->dp) { > > >> > - *portp = pb->tunnel_key; > > >> > - return true; > > >> > + if (strcmp(pb->type, "external")) { > > >> > + *portp = pb->tunnel_key; > > >> > + return true; > > >> > + } > > >> > + const char *chassis_id = smap_get(&pb->options, > > >> > + "requested-chassis"); > > >> > + if (chassis_id && (!strcmp(chassis_id, aux->chassis->name) > || > > >> > + !strcmp(chassis_id, > aux->chassis->hostname))) { > > >> > + const struct sbrec_port_binding *localnet_pb > > >> > + = > lport_lookup_by_type(aux->sbrec_datapath_binding_by_key, > > >> > + > aux->sbrec_port_binding_by_type, > > >> > + aux->dp->tunnel_key, > "localnet"); > > >> > + if (localnet_pb) { > > >> > + *portp = localnet_pb->tunnel_key; > > >> > + return true; > > >> > + } > > >> > + } > > >> > + return false; > > >> > } > > >> > > > >> > const struct sbrec_multicast_group *mg = > mcgroup_lookup_by_dp_name( > > >> > @@ -144,6 +165,8 @@ add_logical_flows( > > >> > struct ovsdb_idl_index *sbrec_chassis_by_name, > > >> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > >> > const struct sbrec_dhcp_options_table *dhcp_options_table, > > >> > const struct sbrec_dhcpv6_options_table *dhcpv6_options_table, > > >> > const struct sbrec_logical_flow_table *logical_flow_table, > > >> > @@ -183,6 +206,8 @@ add_logical_flows( > > >> > consider_logical_flow(sbrec_chassis_by_name, > > >> > > sbrec_multicast_group_by_name_datapath, > > >> > sbrec_port_binding_by_name, > > >> > + sbrec_port_binding_by_type, > > >> > + sbrec_datapath_binding_by_key, > > >> > lflow, local_datapaths, > > >> > chassis, &dhcp_opts, &dhcpv6_opts, > &nd_ra_opts, > > >> > addr_sets, port_groups, > active_tunnels, > > >> > @@ -200,6 +225,8 @@ consider_logical_flow( > > >> > struct ovsdb_idl_index *sbrec_chassis_by_name, > > >> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > >> > const struct sbrec_logical_flow *lflow, > > >> > const struct hmap *local_datapaths, > > >> > const struct sbrec_chassis *chassis, > > >> > @@ -292,7 +319,10 @@ consider_logical_flow( > > >> > .sbrec_multicast_group_by_name_datapath > > >> > = sbrec_multicast_group_by_name_datapath, > > >> > .sbrec_port_binding_by_name = sbrec_port_binding_by_name, > > >> > - .dp = lflow->logical_datapath > > >> > + .sbrec_port_binding_by_type = sbrec_port_binding_by_type, > > >> > + .sbrec_datapath_binding_by_key = > sbrec_datapath_binding_by_key, > > >> > + .dp = lflow->logical_datapath, > > >> > + .chassis = chassis > > >> > }; > > >> > struct condition_aux cond_aux = { > > >> > .sbrec_chassis_by_name = sbrec_chassis_by_name, > > >> > @@ -463,6 +493,8 @@ void > > >> > lflow_run(struct ovsdb_idl_index *sbrec_chassis_by_name, > > >> > struct ovsdb_idl_index > *sbrec_multicast_group_by_name_datapath, > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > >> > const struct sbrec_dhcp_options_table > *dhcp_options_table, > > >> > const struct sbrec_dhcpv6_options_table > *dhcpv6_options_table, > > >> > const struct sbrec_logical_flow_table > *logical_flow_table, > > >> > @@ -481,7 +513,8 @@ lflow_run(struct ovsdb_idl_index > *sbrec_chassis_by_name, > > >> > > > >> > add_logical_flows(sbrec_chassis_by_name, > > >> > sbrec_multicast_group_by_name_datapath, > > >> > - sbrec_port_binding_by_name, > dhcp_options_table, > > >> > + sbrec_port_binding_by_name, > sbrec_port_binding_by_type, > > >> > + sbrec_datapath_binding_by_key, > dhcp_options_table, > > >> > dhcpv6_options_table, logical_flow_table, > > >> > local_datapaths, chassis, addr_sets, > port_groups, > > >> > active_tunnels, local_lport_ids, flow_table, > group_table, > > >> > diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h > > >> > index d19338140..b2911e0eb 100644 > > >> > --- a/ovn/controller/lflow.h > > >> > +++ b/ovn/controller/lflow.h > > >> > @@ -68,6 +68,8 @@ void lflow_init(void); > > >> > void lflow_run(struct ovsdb_idl_index *sbrec_chassis_by_name, > > >> > struct ovsdb_idl_index > *sbrec_multicast_group_by_name_datapath, > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > >> > const struct sbrec_dhcp_options_table *, > > >> > const struct sbrec_dhcpv6_options_table *, > > >> > const struct sbrec_logical_flow_table *, > > >> > diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c > > >> > index cc5c5fbb2..9c827d9b0 100644 > > >> > --- a/ovn/controller/lport.c > > >> > +++ b/ovn/controller/lport.c > > >> > @@ -64,6 +64,32 @@ lport_lookup_by_key(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > >> > return retval; > > >> > } > > >> > > > >> > +const struct sbrec_port_binding * > > >> > +lport_lookup_by_type(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > >> > + struct ovsdb_idl_index > *sbrec_port_binding_by_type, > > >> > + uint64_t dp_key, const char *port_type) > > >> > +{ > > >> > + /* Lookup datapath corresponding to dp_key. */ > > >> > + const struct sbrec_datapath_binding *db = > datapath_lookup_by_key( > > >> > + sbrec_datapath_binding_by_key, dp_key); > > >> > + if (!db) { > > >> > + return NULL; > > >> > + } > > >> > + > > >> > + /* Build key for an indexed lookup. */ > > >> > + struct sbrec_port_binding *pb = > sbrec_port_binding_index_init_row( > > >> > + sbrec_port_binding_by_type); > > >> > + sbrec_port_binding_index_set_datapath(pb, db); > > >> > + sbrec_port_binding_index_set_type(pb, port_type); > > >> > + > > >> > + const struct sbrec_port_binding *retval = > sbrec_port_binding_index_find( > > >> > + sbrec_port_binding_by_type, pb); > > >> > + > > >> > + sbrec_port_binding_index_destroy_row(pb); > > >> > + > > >> > + return retval; > > >> > +} > > >> > + > > >> > const struct sbrec_datapath_binding * > > >> > datapath_lookup_by_key(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > >> > uint64_t dp_key) > > >> > diff --git a/ovn/controller/lport.h b/ovn/controller/lport.h > > >> > index 7dcd5bee0..2d49792f6 100644 > > >> > --- a/ovn/controller/lport.h > > >> > +++ b/ovn/controller/lport.h > > >> > @@ -42,6 +42,11 @@ const struct sbrec_port_binding > *lport_lookup_by_key( > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > >> > uint64_t dp_key, uint64_t port_key); > > >> > > > >> > +const struct sbrec_port_binding *lport_lookup_by_type( > > >> > + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type, > > >> > + uint64_t dp_key, const char *port_type); > > >> > + > > >> > const struct sbrec_datapath_binding *datapath_lookup_by_key( > > >> > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > uint64_t dp_key); > > >> > > > >> > diff --git a/ovn/controller/ovn-controller.c > b/ovn/controller/ovn-controller.c > > >> > index 4e9a5865f..5aab9142f 100644 > > >> > --- a/ovn/controller/ovn-controller.c > > >> > +++ b/ovn/controller/ovn-controller.c > > >> > @@ -145,6 +145,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > >> > * ports that have a Gateway_Chassis that point's to our own > > >> > * chassis */ > > >> > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, > "chassisredirect"); > > >> > + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, > "external"); > > >> > if (chassis) { > > >> > /* This should be mostly redundant with the other clauses > for port > > >> > * bindings, but it allows us to catch any ports that are > assigned to > > >> > @@ -616,6 +617,9 @@ main(int argc, char *argv[]) > > >> > struct ovsdb_idl_index *sbrec_port_binding_by_datapath > > >> > = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > >> > > &sbrec_port_binding_col_datapath); > > >> > + struct ovsdb_idl_index *sbrec_port_binding_by_type > > >> > + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > >> > + &sbrec_port_binding_col_type); > > >> > > >> This index is used with two columns: datapath_binding and type, so it > > >> should be created with both columns using create2. > > >> > > >> > struct ovsdb_idl_index *sbrec_datapath_binding_by_key > > >> > = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > >> > > &sbrec_datapath_binding_col_tunnel_key); > > >> > @@ -743,6 +747,8 @@ main(int argc, char *argv[]) > > >> > sbrec_chassis_by_name, > > >> > sbrec_multicast_group_by_name_datapath, > > >> > sbrec_port_binding_by_name, > > >> > + sbrec_port_binding_by_type, > > >> > + sbrec_datapath_binding_by_key, > > >> > > sbrec_dhcp_options_table_get(ovnsb_idl_loop.idl), > > >> > > sbrec_dhcpv6_options_table_get(ovnsb_idl_loop.idl), > > >> > > sbrec_logical_flow_table_get(ovnsb_idl_loop.idl), > > >> > diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c > > >> > index aa03919bb..a9d4b8736 100644 > > >> > --- a/ovn/lib/ovn-util.c > > >> > +++ b/ovn/lib/ovn-util.c > > >> > @@ -319,6 +319,7 @@ static const char *OVN_NB_LSP_TYPES[] = { > > >> > "localport", > > >> > "router", > > >> > "vtep", > > >> > + "external", > > >> > }; > > >> > > > >> > bool > > >> > diff --git a/ovn/northd/ovn-northd.8.xml > b/ovn/northd/ovn-northd.8.xml > > >> > index 392a5efc9..c8883d60d 100644 > > >> > --- a/ovn/northd/ovn-northd.8.xml > > >> > +++ b/ovn/northd/ovn-northd.8.xml > > >> > @@ -626,7 +626,8 @@ nd_na_router { > > >> > <p> > > >> > This table adds the DHCPv4 options to a DHCPv4 packet from > the > > >> > logical ports configured with IPv4 address(es) and DHCPv4 > options, > > >> > - and similarly for DHCPv6 options. > > >> > + and similarly for DHCPv6 options. This table also adds flows > for the > > >> > + logical ports of type <code>external</code>. > > >> > </p> > > >> > > > >> > <ul> > > >> > @@ -827,7 +828,39 @@ output; > > >> > </li> > > >> > </ul> > > >> > > > >> > - <h3>Ingress Table 16 Destination Lookup</h3> > > >> > + <h3>Ingress table 16 External ports</h3> > > >> > + > > >> > + <p> > > >> > + Traffic from the <code>external</code> logical ports enter > the ingress > > >> > + datapath pipeline via the <code>localnet</code> port. This > table adds the > > >> > + below logical flows to handle the traffic from these ports. > > >> > + </p> > > >> > + > > >> > + <ul> > > >> > + <li> > > >> > + <p> > > >> > + A priority-100 flow is added for each > <code>external</code> logical > > >> > + port which doesn't reside on a chassis to drop the > ARP/IPv6 NS > > >> > + request to the router IP(s) (of the logical switch) > which matches > > >> > + on the <code>inport</code> of the <code>external</code> > logical port > > >> > + and the valid <code>eth.src</code> address(es) of the > > >> > + <code>external</code> logical port. > > >> > + </p> > > >> > + > > >> > + <p> > > >> > + This flow guarantees that the ARP/NS request to the > router IP > > >> > + address from the external ports is responded by only the > chassis > > >> > + which has claimed these external ports. All the other > chassis, > > >> > + drops these packets. > > >> > + </p> > > >> > + </li> > > >> > + > > >> > + <li> > > >> > + A priority-0 flow that matches all packets to advances to > table 17. > > >> > + </li> > > >> > + </ul> > > >> > + > > >> > + <h3>Ingress Table 17 Destination Lookup</h3> > > >> > > > >> > <p> > > >> > This table implements switching behavior. It contains these > logical > > >> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > >> > index 3fd8a8757..87208c6c1 100644 > > >> > --- a/ovn/northd/ovn-northd.c > > >> > +++ b/ovn/northd/ovn-northd.c > > >> > @@ -119,7 +119,8 @@ enum ovn_stage { > > >> > PIPELINE_STAGE(SWITCH, IN, DHCP_RESPONSE, 13, > "ls_in_dhcp_response") \ > > >> > PIPELINE_STAGE(SWITCH, IN, DNS_LOOKUP, 14, > "ls_in_dns_lookup") \ > > >> > PIPELINE_STAGE(SWITCH, IN, DNS_RESPONSE, 15, > "ls_in_dns_response") \ > > >> > - PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 16, > "ls_in_l2_lkup") \ > > >> > + PIPELINE_STAGE(SWITCH, IN, EXTERNAL_PORT, 16, > "ls_in_external_port") \ > > >> > + PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 17, > "ls_in_l2_lkup") \ > > >> > > \ > > >> > /* Logical switch egress stages. */ > \ > > >> > PIPELINE_STAGE(SWITCH, OUT, PRE_LB, 0, "ls_out_pre_lb") > \ > > >> > @@ -2942,6 +2943,12 @@ lsp_is_up(const struct > nbrec_logical_switch_port *lsp) > > >> > return !lsp->up || *lsp->up; > > >> > } > > >> > > > >> > +static bool > > >> > +lsp_is_external(const struct nbrec_logical_switch_port *nbsp) > > >> > +{ > > >> > + return !strcmp(nbsp->type, "external"); > > >> > +} > > >> > + > > >> > static bool > > >> > build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip, > > >> > struct ds *options_action, struct ds > *response_action, > > >> > @@ -4185,7 +4192,7 @@ build_lswitch_flows(struct hmap *datapaths, > struct hmap *ports, > > >> > * - port type is localport > > >> > */ > > >> > if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, > "router") && > > >> > - strcmp(op->nbsp->type, "localport")) { > > >> > + strcmp(op->nbsp->type, "localport") && > lsp_is_external(op->nbsp)) { > > >> > > >> Sorry that I missed this in last review. The && condition has problem. > > >> It will cause ARP responder flows added for all lports that are not > > >> external. I think it should be || here. > > > > > > > > > Agree. To make it easier to read, I will add a new "if" with continue > - below this one for > > > external port types. > > > > > > > > >> > > >> > > >> > continue; > > >> > } > > >> > > > >> > @@ -4297,6 +4304,13 @@ build_lswitch_flows(struct hmap *datapaths, > struct hmap *ports, > > >> > continue; > > >> > } > > >> > > > >> > + bool is_external = lsp_is_external(op->nbsp); > > >> > + if (is_external && !op->od->localnet_port) { > > >> > + /* If it's an external port and there is no localnet > port > > >> > + * ignore it. */ > > >> > + continue; > > >> > + } > > >> > + > > >> > for (size_t i = 0; i < op->n_lsp_addrs; i++) { > > >> > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; > j++) { > > >> > struct ds options_action = DS_EMPTY_INITIALIZER; > > >> > @@ -4309,8 +4323,8 @@ build_lswitch_flows(struct hmap *datapaths, > struct hmap *ports, > > >> > ds_put_format( > > >> > &match, "inport == %s && eth.src == %s && " > > >> > "ip4.src == 0.0.0.0 && ip4.dst == > 255.255.255.255 && " > > >> > - "udp.src == 68 && udp.dst == 67", > op->json_key, > > >> > - op->lsp_addrs[i].ea_s); > > >> > + "udp.src == 68 && udp.dst == 67", > > >> > + op->json_key, op->lsp_addrs[i].ea_s); > > >> > > >> No change here? > > > > > > > > > I think it's unwanted and unrelated change. I will correct it. > > >> > > >> > > > >> > ovn_lflow_add(lflows, op->od, > S_SWITCH_IN_DHCP_OPTIONS, > > >> > 100, ds_cstr(&match), > > >> > @@ -4415,7 +4429,9 @@ build_lswitch_flows(struct hmap *datapaths, > struct hmap *ports, > > >> > /* Ingress table 12 and 13: DHCP options and response, by > default goto > > >> > * next. (priority 0). > > >> > * Ingress table 14 and 15: DNS lookup and response, by > default goto next. > > >> > - * (priority 0).*/ > > >> > + * (priority 0). > > >> > + * Ingress table 16 - External port handling, by default goto > next. > > >> > + * (priority 0). */ > > >> > > > >> > HMAP_FOR_EACH (od, key_node, datapaths) { > > >> > if (!od->nbs) { > > >> > @@ -4426,9 +4442,58 @@ build_lswitch_flows(struct hmap *datapaths, > struct hmap *ports, > > >> > ovn_lflow_add(lflows, od, S_SWITCH_IN_DHCP_RESPONSE, 0, > "1", "next;"); > > >> > ovn_lflow_add(lflows, od, S_SWITCH_IN_DNS_LOOKUP, 0, "1", > "next;"); > > >> > ovn_lflow_add(lflows, od, S_SWITCH_IN_DNS_RESPONSE, 0, > "1", "next;"); > > >> > + ovn_lflow_add(lflows, od, S_SWITCH_IN_EXTERNAL_PORT, 0, > "1", "next;"); > > >> > } > > >> > > > >> > - /* Ingress table 16: Destination lookup, broadcast and > multicast handling > > >> > + HMAP_FOR_EACH (op, key_node, ports) { > > >> > + if (!op->nbsp || !lsp_is_external(op->nbsp)) { > > >> > + continue; > > >> > + } > > >> > + > > >> > + /* Table 16: External port. Drop ARP request for router > ips from > > >> > + * external ports on chassis not binding those ports. > > >> > + * This makes the router pipeline to be run only on the > chassis > > >> > + * binding the external ports. */ > > >> > + > > >> > + for (size_t i = 0; i < op->n_lsp_addrs; i++) { > > >> > + for (size_t j = 0; j < op->od->n_router_ports; j++) { > > >> > + struct ovn_port *rp = op->od->router_ports[j]; > > >> > + for (size_t k = 0; k < rp->n_lsp_addrs; k++) { > > >> > + for (size_t l = 0; l < > rp->lsp_addrs[k].n_ipv4_addrs; > > >> > + l++) { > > >> > + ds_clear(&match); > > >> > + ds_put_cstr(&match, "ip4"); > > >> > + ds_put_format( > > >> > + &match, "inport == %s && eth.src == %s" > > >> > + " && !is_chassis_resident(%s)" > > >> > + " && arp.tpa == %s && arp.op == 1", > > >> > + op->json_key, op->lsp_addrs[i].ea_s, > op->json_key, > > >> > > >> I believe the inport should match the localnet port's json_key here, > > >> since it is coming from a localnet port. > > > > > > > > > Both would work. If you see the code in lflow.c in this patch - it > will get the tunnel > > > key of the localnet port if the port_binding type is "external". > > > > > > That's how even the DHCP requests are handled. ovn-controller will > translate > > > the logical flows with action "put_dhcp_opts" only the chassis > claiming the > > > external ports. > > > > Oh, yes you are right. Actually I read that part in v4 and it somehow > > slipped my mind. Thanks for explain. > > I thought it a second time, and I'd suggest to do the convertion here > in northd instead of ovn-controller, for two reasons: > > 1. In ovn-controller there is no extra context so it just blindly > transate all references to external logical port into localnet port > key. This could lead to unexpected behavior. For example, if someone > uses external logical port in ACL match condition. The match condition > would then apply to all packets to/from localnet port which is > definitely unwanted. (at the same time it would be better to document > that features like port-security, ACL should not be used for external > logical ports) > > That's not how it works in the present patch. Lets say you have 2 chassis hv1 and hv2 and an external port sw0-ext1 and a localnet port "ln-public". Suppose if the requested-chassis is set to hv1, then all the logical flows with the match "inport == sw0-ext1" will be converted to OF flows only on hv1 as this port is bound by hv1 and the function 'lookup_port_cb()' would return true only on hv1 . In hv2, lookup_port_cb() would return false.
If we want to do the conversion in ovn-northd.c the match condition would have to be - "inport == "ln-public" && is_chassis_resident(sw0-ext1) && ...." instead of the present one - "inport == sw0-ext1 && ...". And the ACL match condition would not be an issue because of the above mentioned reason. i.e the ACL flows will be applied only on the chassis binding the external port. The test case added checks that the OF flows are applied only on the bound chassis. I think it is better to do it in ovn-controller instead of ovn-northd. Please let me know if you still have any concerns. Thanks Numan 2. A less important reason is, it is better to do it at earlier stage > than later. northd handles common processing. This part of logic is > common for all chassises, so it would be better if we explicitely > handle it in northd, instead of let every chassis to process. And the > change in northd would likely be simpler than in ovn-controller. > > Thanks, > Han > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
