Thanks Lucas for the patch. The patch looks good to me. Just a few comments inline.
Thanks Numan On Tue, Mar 26, 2019 at 11:55 PM Mark Michelson <mmich...@redhat.com> wrote: > Thanks Lucas, looks good to me. > > Acked-by: Mark Michelson <mmich...@redhat.com> > > On 3/25/19 2:24 PM, lmart...@redhat.com wrote: > > From: Lucas Alvares Gomes <lucasago...@gmail.com> > > > > This patch is adding support for Transport Zones. Transport zones (a.k.a > > TZs) is way to enable users of OVN to separate Chassis into different > > logical groups that will form tunnels only between members of the same > > group(s). > > > > Each Chassis can belong to one or more Transport Zones. If not set, > > the Chassis will be considered part of a default group; this feature > > is backward compatible and did not require any changes to the database > > schemas. > > > > Configuring Transport Zones is done by creating a key called > > "ovn-transport-zones" in the external_ids of the Open_vSwitch table of > the > > local OVS instance. The value is a string with the name of the Transport > > Zone that this instance is part of. Multiple TZs may be specified with > > a comma-separated list. For example: > > > > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1 > > > > or > > > > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1,tz2,tz3 > > > > This configuration will be also exposed in the Chassis table of the OVN > > Southbound Database so that external systems can see what TZ(s) each > > Chassis are part of and make decisions based those values. > > > > The use for Transport Zones includes but are not limited to: > > > > * Edge computing: As a way to preventing edge sites from trying to create > > tunnels with every node on every other edge site while still allowing > > these sites to create tunnels with the central node. > > > > * Extra security layer: Where users wants to create "trust zones" > > and prevent computes in a more secure zone to communicate with a less > > secure zone. > > > > Reported-by: Daniel Alvarez Sanchez <dalva...@redhat.com> > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html > > Signed-off-by: Lucas Alvares Gomes <lucasago...@gmail.com> > > --- > > > > v1 -> v2 > > * Rename the function check_chassis_tzones to chassis_tzones_overlap. > > * Fix a memory leak in chassis_tzones_overlap. > > * Pass the transport_zones to encaps_run() as a "const char *" > > instead of "struct sbrec_chassis". With this we can also avoid not > > running the function in case the Chassis entry is not yet created. > > > > NEWS | 3 + > > ovn/controller/chassis.c | 8 ++- > > ovn/controller/encaps.c | 58 +++++++++++++++++- > > ovn/controller/encaps.h | 3 +- > > ovn/controller/ovn-controller.8.xml | 9 +++ > > ovn/controller/ovn-controller.c | 14 ++++- > > tests/ovn.at | 93 +++++++++++++++++++++++++++++ > > 7 files changed, 183 insertions(+), 5 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 1e4744dbd..4adf49f57 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -24,6 +24,9 @@ Post-v2.11.0 > > protocol extension. > > - OVN: > > * Select IPAM mac_prefix in a random manner if not provided by > the user > > + * Support for Transport Zones, a way to separate chassis into > > + logical groups which results in tunnels only been formed between > > + members of the same transport zone(s). > > - New QoS type "linux-netem" on Linux. > > > > v2.11.0 - 19 Feb 2019 > > diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c > > index 3ea908d18..34c260410 100644 > > --- a/ovn/controller/chassis.c > > +++ b/ovn/controller/chassis.c > > @@ -139,6 +139,8 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const char *datapath_type = > > br_int && br_int->datapath_type ? br_int->datapath_type : ""; > > const char *cms_options = get_cms_options(&cfg->external_ids); > > + const char *transport_zones = smap_get_def(&cfg->external_ids, > > + "ovn-transport-zones", > ""); > I think you can delete this. It's not used any where. At Line 172 below you are getting the value of "ovn-transport-zones" with in the "if (chassis_rec)" scope. > > > > struct ds iface_types = DS_EMPTY_INITIALIZER; > > ds_put_cstr(&iface_types, ""); > > @@ -167,18 +169,22 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > = smap_get_def(&chassis_rec->external_ids, "iface-types", > ""); > > const char *chassis_cms_options > > = get_cms_options(&chassis_rec->external_ids); > > + const char *chassis_transport_zones = smap_get_def( > > + &chassis_rec->external_ids, "ovn-transport-zones", ""); > > > > /* If any of the external-ids should change, update them. */ > > if (strcmp(bridge_mappings, chassis_bridge_mappings) || > > strcmp(datapath_type, chassis_datapath_type) || > > strcmp(iface_types_str, chassis_iface_types) || > > - strcmp(cms_options, chassis_cms_options)) { > > + strcmp(cms_options, chassis_cms_options) || > > + strcmp(transport_zones, chassis_transport_zones)) { > > struct smap new_ids; > > smap_clone(&new_ids, &chassis_rec->external_ids); > > smap_replace(&new_ids, "ovn-bridge-mappings", > bridge_mappings); > > smap_replace(&new_ids, "datapath-type", datapath_type); > > smap_replace(&new_ids, "iface-types", iface_types_str); > > smap_replace(&new_ids, "ovn-cms-options", cms_options); > > + smap_replace(&new_ids, "ovn-transport-zones", > transport_zones); > > sbrec_chassis_verify_external_ids(chassis_rec); > > sbrec_chassis_set_external_ids(chassis_rec, &new_ids); > > smap_destroy(&new_ids); > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > index 610b833de..72c43a9dd 100644 > > --- a/ovn/controller/encaps.c > > +++ b/ovn/controller/encaps.c > > @@ -195,13 +195,56 @@ chassis_tunnel_add(const struct sbrec_chassis > *chassis_rec, const struct sbrec_s > > return tuncnt; > > } > > > > +/* > > +* Returns true if our_chassis_tzones and chassis_tzones have at least > > +* one common transport zone. > > +*/ > > +static bool > > +chassis_tzones_overlap(const char *our_chassis_tzones, > > + const char *chassis_tzones) > > +{ > > + if (!strcmp(our_chassis_tzones, "") && !strcmp(chassis_tzones, "")) > { > > + return true; > > + } > > + > > + bool found = false; > > + char *our_tzones_orig; > > + char *our_tzones = xstrdup(our_chassis_tzones); > > + char *i = our_tzones_orig = our_tzones; > > + > > + while ((i = strsep(&our_tzones, ","))) { > > + > > + char *tzones_orig; > > + char *tzones = xstrdup(chassis_tzones); > > + char *j = tzones_orig = tzones; > > + > > + while ((j = strsep(&tzones, ","))) { > > + > > + if (!strcmp(i, j)) { > > + found = true; > > + break; > > + } > > + } > > + > > + free(tzones_orig); > > + if (found) { > > + break; > > + } > > + > > + } > > + > > + free(our_tzones_orig); > > + return found; > > +} > > + > > void > > encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > const struct ovsrec_bridge_table *bridge_table, > > const struct ovsrec_bridge *br_int, > > const struct sbrec_chassis_table *chassis_table, > > const char *chassis_id, > > - const struct sbrec_sb_global *sbg) > > + const struct sbrec_sb_global *sbg, > > + const char *transport_zones) > > { > > if (!ovs_idl_txn || !br_int) { > > return; > > @@ -251,7 +294,18 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > > > SBREC_CHASSIS_TABLE_FOR_EACH (chassis_rec, chassis_table) { > > if (strcmp(chassis_rec->name, chassis_id)) { > > - /* Create tunnels to the other chassis. */ > > + /* Create tunnels to the other Chassis belonging to the > > + * same transport zone */ > > + const char *chassis_tzones = smap_get_def( > > + &chassis_rec->external_ids, "ovn-transport-zones", ""); > > + > > + if (!chassis_tzones_overlap(transport_zones, > chassis_tzones)) { > > + VLOG_DBG("Skipping encap creation for Chassis '%s' > because " > > + "it belongs to different transport zones", > > + chassis_rec->name); > > + continue; > > + } > > + > > if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { > > VLOG_INFO("Creating encap for '%s' failed", > chassis_rec->name); > > continue; > > diff --git a/ovn/controller/encaps.h b/ovn/controller/encaps.h > > index 3e0e110ef..9eafec948 100644 > > --- a/ovn/controller/encaps.h > > +++ b/ovn/controller/encaps.h > > @@ -32,7 +32,8 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > const struct ovsrec_bridge *br_int, > > const struct sbrec_chassis_table *, > > const char *chassis_id, > > - const struct sbrec_sb_global *); > > + const struct sbrec_sb_global *, > > + const char *transport_zones); > > > > bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, > > const struct ovsrec_bridge *br_int); > > diff --git a/ovn/controller/ovn-controller.8.xml > b/ovn/controller/ovn-controller.8.xml > > index fd2e10a7a..072ec5820 100644 > > --- a/ovn/controller/ovn-controller.8.xml > > +++ b/ovn/controller/ovn-controller.8.xml > > @@ -167,6 +167,15 @@ > > specific to this particular chassis. An example would be: > > <code>cms_option1,cms_option2:foo</code>. > > </dd> > > + > > + <dt><code>external_ids:ovn-transport-zones</code></dt> > > + <dd> > > + The transport zone(s) that this chassis belongs to. Transport > > + zones is a way to group different chassis so that tunnels are > only > > + formed between members of the same group(s). Multiple transport > > + zones may be specified with a comma-separated list. For example: > > + tz1,tz2,tz3. > > + </dd> > > </dl> > > > > <p> > > diff --git a/ovn/controller/ovn-controller.c > b/ovn/controller/ovn-controller.c > > index 882cc195f..9383ec191 100644 > > --- a/ovn/controller/ovn-controller.c > > +++ b/ovn/controller/ovn-controller.c > > @@ -511,6 +511,14 @@ get_nb_cfg(const struct sbrec_sb_global_table > *sb_global_table) > > return sb ? sb->nb_cfg : 0; > > } > > > > +static const char * > > +get_transport_zones(const struct ovsrec_open_vswitch_table *ovs_table) > > +{ > > + const struct ovsrec_open_vswitch *cfg > > + = ovsrec_open_vswitch_table_first(ovs_table); > > + return smap_get_def(&cfg->external_ids, "ovn-transport-zones", ""); > > +} > > + > > static void > > ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > { > > @@ -686,6 +694,9 @@ main(int argc, char *argv[]) > > const char *chassis_id > > = get_chassis_id(ovsrec_open_vswitch_table_get( > > ovs_idl_loop.idl)); > > + const char *transport_zones > > + = get_transport_zones(ovsrec_open_vswitch_table_get( > > + ovs_idl_loop.idl)); > > > > const struct sbrec_chassis *chassis = NULL; > > if (chassis_id) { > > @@ -697,7 +708,8 @@ main(int argc, char *argv[]) > > ovs_idl_txn, > > ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int, > > sbrec_chassis_table_get(ovnsb_idl_loop.idl), > chassis_id, > > - sbrec_sb_global_first(ovnsb_idl_loop.idl)); > > + sbrec_sb_global_first(ovnsb_idl_loop.idl), > > + transport_zones); > > > > if (ofctrl_is_connected()) { > > /* Calculate the active tunnels only if have an an > active > > diff --git a/tests/ovn.at b/tests/ovn.at > > index f2f2bc405..3988474d0 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -12281,3 +12281,96 @@ ovn-nbctl list logical_switch_port > > ovn-nbctl list logical_router_port > > > > AT_CLEANUP > > + > > +AT_SETUP([ovn -- test transport zones]) > > +ovn_start > > + > > +net_add n1 > > +for i in 1 2 3; do > > + sim_add hv$i > > + as hv$i > > + ovs-vsctl add-br br-phys > > + ovn_attach n1 br-phys 192.168.$i.1 > > +done > > + > > +dnl Assert that each Chassis has a tunnel formed to every other Chassis > > +as hv1 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv2-0 > > +ovn-hv3-0 > > +]]) > > + > > +as hv2 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +ovn-hv3-0 > > +]]) > > + > > +as hv3 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +ovn-hv2-0 > > +]]) > > + > > +dnl Let's now add some Chassis to different transport zones > > +dnl * hv1: Will be part of two transport zones: tz1 and tz2 so it > > +dnl should have tunnels formed between the other two Chassis (hv2 and > hv3) > > +dnl > > +dnl * hv2: Will be part of one transport zone: tz1. It should have a > tunnel > > +dnl to hv1 but not to hv3 > > +dnl > > +dnl * hv3: Will be part of one transport zone: tz2. It should have a > tunnel > > +dnl to hv1 but not to hv2 > > +dnl > > +as hv1 > > +ovs-vsctl set open . external-ids:ovn-transport-zones=tz1,tz2 > > + > > +as hv2 > > +ovs-vsctl set open . external-ids:ovn-transport-zones=tz1 > > + > > +as hv3 > > +ovs-vsctl set open . external-ids:ovn-transport-zones=tz2 > > + > Can you please enhance the test case to add a couple more chassis - hv4 and hv5 with no tzs set. hv4 and hv5 should have tunnel between them. With the present design, I suppose hv1 cannot be part of "hv4 and hv5" (where 'ovn-transport-zones' is not defined) right ? Thanks Numan > > +as hv1 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv2-0 > > +ovn-hv3-0 > > +]]) > > + > > +as hv2 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +]]) > > + > > +as hv3 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +]]) > > + > > +dnl Removing the transport zones should make all Chassis to create > > +dnl tunnels between every other Chassis again > > +for i in 1 2 3; do > > + as hv$i > > + ovs-vsctl remove open . external-ids ovn-transport-zones > > +done > > + > > +as hv1 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv2-0 > > +ovn-hv3-0 > > +]]) > > + > > +as hv2 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +ovn-hv3-0 > > +]]) > > + > > +as hv3 > > +AT_CHECK([ovs-vsctl --bare --columns=name find interface type="geneve" > | awk NF | sort], [0], > > +[[ovn-hv1-0 > > +ovn-hv2-0 > > +]]) > > + > > +OVN_CLEANUP([hv1], [hv2], [hv3]) > > +AT_CLEANUP > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
