On Thu, Mar 28, 2019 at 3:34 PM Lucas Alvares Gomes Martins < [email protected]> wrote:
> On Thu, Mar 28, 2019 at 9:19 AM Numan Siddique <[email protected]> > wrote: > > > > > > 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 <[email protected]> > wrote: > >> > >> Thanks Lucas, looks good to me. > >> > >> Acked-by: Mark Michelson <[email protected]> > >> > >> On 3/25/19 2:24 PM, [email protected] wrote: > >> > From: Lucas Alvares Gomes <[email protected]> > >> > > >> > 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 <[email protected]> > >> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html > >> > Signed-off-by: Lucas Alvares Gomes <[email protected]> > >> > --- > >> > > >> > 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. > > > > Hmm I was looking at the code and I think this has to stay because > this value (which comes from the Open_vSwitch table) is being compared > with the one from the Chassis table to see if something changed and > before updating it. > Oops. My bad. Please ignore this comment. I think its fine the way its done. > > An alternative would be to always update the Chassis table with the > value from the Open_vSwitch table without comparing it before. But > that wouldn't conform with two other options (cms_options and > bridge_mappings) which are being updated in the same way; and probably > we do not want to issue an update if nothing has changed. Here [0]. > > [0] > https://github.com/openvswitch/ovs/blob/8e738337a2c25c3d6ede2829d6ffd9af6bcd36a5/ovn/controller/chassis.c#L172-L184 > > >> > >> > > >> > 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. > > > > That's correct. Chasiss with no TZ set will have tunnels formed between > them. > > > With the present design, I suppose hv1 cannot be part of "hv4 and hv5" > (where 'ovn-transport-zones' is not > > defined) right ? > > > > That's right, since hv1 is part of a transport zone already so it will > create tunnels to any other chassis that matches at least one of its > transport zones. That said, it's quite flexible. So if we want a > tunnel formed between hv1, hv4 and hv5 I would suggest setting them to > "tz3" accordingly with the examples from the test. > > Alternatively, we could add a "special" value for the default TZ so > that it would also form tunnels between Chassis with no TZ set. But, > IMHO, this would just add complexity to the code because with the > current design there's nothing preventing someone from creating a TZ > called "default" for example and every new chassis have this option > set to it to achieve the same effect. > > Let me know what you think. In the meantime I'm going to enhance the > tests to include the scenario you suggested. > > Agree. Probably there is no need to compilcate. CMS can just set "default" if its want. I just wanted to confirm current behavior. Thanks Numan > > Thanks > > Numan > > > > Thank you for the review. > > > > >> > >> > +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 > >> [email protected] > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
