On Sat, Jan 19, 2019, 12:32 AM Han Zhou <[email protected] wrote: > On Fri, Jan 18, 2019 at 10:16 AM Numan Siddique <[email protected]> > wrote: > > > > > > > > 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. > > Yes, this is well understood. > > > > > 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 && ...". > > Yes, this is what I would suggest (see reason below). > > > > > 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. > > Here is the concern. For example, chassis A has regular port sw0-lsp1 > bound. Chassis A is also set as requested-chassis for external port > sw0-ext1. And now the user adds an ACL: to-port, 1001, 'outport == > "sw0-ext1", drop. This would get translated to something like: > to-port, 1001, 'outport == "ln-public"', drop. Wouldn't this impact > the traffic from sw0-lsp1 to the bridged network? Even if it doesn't > impact because of some subtle reasons of current implementation, I > would say it is risky and could leads to problems under certain > conditions, because the conversion in ovn-controller widens the > original intent. Whereas doing it in northd only for specific lflows > would ensure it has impact only for intended use cases. >
Thanks for the detailed explanation. I agree. It's clear to me now. I will update accordingly in v6. Regards Numan > > > > 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. > > > > > > > >> 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. > > This is less critical problem, but I think it is worth consideration, > too. With current logic, although the conversion would take effect > only if "is_chassis_resident()" is true, but the code logic and > processing has to happen on every chassis. > > >> > >> Thanks, > >> Han > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
