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. > > 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
