On Mon, Jan 21, 2019 at 7:06 AM Miguel Angel Ajo Pelayo <[email protected]> wrote: > > > > 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?
I thought it would be easier to support outside of OVN combining with chassis life-cycle management, but I didn't go deeper in any CMS implementation. I agree it is better to handle HA in OVN than implementing it in every CMS. But I am also worring about the complexity in OVN itself. Could you describe briefly how would you support it in OVN? For example, how to detect if a chassis failed? It is different from gateway chassis because the major use case of external port is for bridged networks (vlan/flat), so I think the BFD mechanism for tunnel health monitoring may not be a good fit here. Thanks, Han >> >> 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
