On Mon, Jan 21, 2019 at 4:02 PM Numan Siddique <[email protected]> wrote:
> > Hi Han, > > I have addressed your comments. But before posting the patch I wanted to > get an opinion > on the HA support for these external ports. > > The proposed patch doesn't support HA. If the requested chassis goes down > for some reason > it is expected that CMS would detect it and change the requested-chassis > option to other > suitable chassis. > > The openstack OVN folks think this would be too much for the CMS to handle > and it would > complicate the code in networking-ovn which I agree with. > > Not only the complexity part. If we implement this from the CMS, then every CMS using ovn will need to replicate that behaviour. That's in my opinion a good reason why it's better to handle HA within OVN itself. > I am thinking to add the HA support on the lines of gateway chassis > support and I want to > submit this patch after adding the HA support. I think this would be > better as we won't add > more options in OVN (first requested-chassis for external ports and then > later HA chassis support). > Thoughts? > > Thanks > Numan > > > On Sat, Jan 19, 2019 at 12:42 AM Numan Siddique <[email protected]> > wrote: > >> >> >> 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 >>> >> -- Miguel Ángel Ajo OSP / Networking DFG, OVN Squad Engineering _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
