| Message ID | 20190325143646.14263-1-lmartins@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] OVN: Add support for Transport Zones | expand |
Hi Lucas, Thanks for the patch. It's mostly good but I have a few issues with it. See below. On 3/25/19 10:36 AM, lmartins@redhat.com wrote: > From: Lucas Alvares Gomes <lucasagomes@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 <dalvarez@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html > Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com> > --- > NEWS | 3 + > ovn/controller/chassis.c | 8 ++- > ovn/controller/encaps.c | 54 +++++++++++++++-- > ovn/controller/encaps.h | 3 +- > ovn/controller/ovn-controller.8.xml | 9 +++ > ovn/controller/ovn-controller.c | 2 +- > tests/ovn.at | 93 +++++++++++++++++++++++++++++ > 7 files changed, 164 insertions(+), 8 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", ""); > > 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..072be4ac2 100644 > --- a/ovn/controller/encaps.c > +++ b/ovn/controller/encaps.c > @@ -195,20 +195,52 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_s > return tuncnt; > } > > +/* > + * Check if both chassis are in the same transport zone. > + */ > +static bool > +check_chassis_tzones(const char *our_chassis_tzones, > + const char *chassis_tzones) I think this function could have a more descriptive name. "check" doesn't describe what's being done very well. Something like chassis_tzones_overlap() might make it more clear that the function returns true if our_chassis_tzones and chassis_tzones have at least one common tzone. > +{ > + if (!strcmp(our_chassis_tzones, "") && !strcmp(chassis_tzones, "")) { > + return true; > + } > + > + char *our_tzones = xstrdup(our_chassis_tzones); > + char *i, *j; > + > + for (i = strsep(&our_tzones, ","); i != NULL; > + i = strsep(&our_tzones, ",")) { > + > + char *tzones = xstrdup(chassis_tzones); > + for (j = strsep(&tzones, ","); j != NULL; > + j = strsep(&tzones, ",")) { > + > + if (!strcmp(i, j)) { > + return true; > + } > + } > + } You're leaking our_tzones and tzones. our_tzones needs to be freed outside the outer for loop, and tzones needs to be freed at the end of the outer for loop. The problem is that you can't just free(our_tzones) and free(tzones) because strsep will have changed them from where they were originally pointing when allocated. Passing them as-is to free() will cause bad things. The easiest way to fix this is to declare an extra pointer that points to where the string was originally allocated: 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))) { ... } free(tzones_orig); } free(our_tzones_orig); (You don't have to convert to a while loop here. That's just a personal preference of mine when working with strsep.) > + > + return false; > +} > + > 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_chassis *our_chassis, Can you explain the rationale for this change? > const struct sbrec_sb_global *sbg) > { > - if (!ovs_idl_txn || !br_int) { > + if (!ovs_idl_txn || !br_int || !our_chassis) { > return; > } This change has some knock-on effects that I'm not sure I'm comfortable with. For instance, this will skip deleting existing OVN tunnels that are not around any more. Is this OK? > > const struct sbrec_chassis *chassis_rec; > const struct ovsrec_bridge *br; > + const char *our_chassis_tzones = smap_get_def( > + &our_chassis->external_ids, "ovn-transport-zones", ""); > > struct tunnel_ctx tc = { > .chassis = SHASH_INITIALIZER(&tc.chassis), > @@ -219,7 +251,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > tc.ovs_txn = ovs_idl_txn; > ovsdb_idl_txn_add_comment(tc.ovs_txn, > "ovn-controller: modifying OVS tunnels '%s'", > - chassis_id); > + our_chassis->name); > > /* Collect all port names into tc.port_names. > * > @@ -250,8 +282,20 @@ 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. */ > + > + if (strcmp(chassis_rec->name, our_chassis->name)) { Since you are passing a chassis record instead of a chassis_id, you can save the string comparison and instead do a direct pointer comparison. if (chassis_rec != our_chassis) { The only reason this would be semantically different is if chassis records are allowed to share the same name. They are not allowed to do that, though. > + /* 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 (!check_chassis_tzones(our_chassis_tzones, 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..71ff78b5e 100644 > --- a/ovn/controller/encaps.h > +++ b/ovn/controller/encaps.h > @@ -23,6 +23,7 @@ struct ovsdb_idl_txn; > struct ovsrec_bridge; > struct ovsrec_bridge_table; > struct sbrec_chassis_table; > +struct sbrec_chassis; > struct sbrec_sb_global; > struct ovsrec_open_vswitch_table; > > @@ -31,7 +32,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > const struct ovsrec_bridge_table *, > const struct ovsrec_bridge *br_int, > const struct sbrec_chassis_table *, > - const char *chassis_id, > + const struct sbrec_chassis *, > const struct sbrec_sb_global *); > > bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, > 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..421bd0acb 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -696,7 +696,7 @@ main(int argc, char *argv[]) > encaps_run( > ovs_idl_txn, > ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int, > - sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id, > + sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis, > sbrec_sb_global_first(ovnsb_idl_loop.idl)); > > if (ofctrl_is_connected()) { > 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 > + > +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 >
Hi, Thanks a lot Mark for the quick review. On Mon, Mar 25, 2019 at 3:40 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Lucas, > > Thanks for the patch. It's mostly good but I have a few issues with it. > See below. > > On 3/25/19 10:36 AM, lmartins@redhat.com wrote: > > From: Lucas Alvares Gomes <lucasagomes@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 <dalvarez@redhat.com> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html > > Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com> > > --- > > NEWS | 3 + > > ovn/controller/chassis.c | 8 ++- > > ovn/controller/encaps.c | 54 +++++++++++++++-- > > ovn/controller/encaps.h | 3 +- > > ovn/controller/ovn-controller.8.xml | 9 +++ > > ovn/controller/ovn-controller.c | 2 +- > > tests/ovn.at | 93 +++++++++++++++++++++++++++++ > > 7 files changed, 164 insertions(+), 8 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", ""); > > > > 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..072be4ac2 100644 > > --- a/ovn/controller/encaps.c > > +++ b/ovn/controller/encaps.c > > @@ -195,20 +195,52 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_s > > return tuncnt; > > } > > > > +/* > > + * Check if both chassis are in the same transport zone. > > + */ > > +static bool > > +check_chassis_tzones(const char *our_chassis_tzones, > > + const char *chassis_tzones) > > I think this function could have a more descriptive name. "check" > doesn't describe what's being done very well. Something like > chassis_tzones_overlap() might make it more clear that the function > returns true if our_chassis_tzones and chassis_tzones have at least one > common tzone. > You're right. It's often said that the two hardest problems in computer science are naming things and cache invalidation. I tend to agree. Thanks for the name suggestion I will rename that function. > > +{ > > + if (!strcmp(our_chassis_tzones, "") && !strcmp(chassis_tzones, "")) { > > + return true; > > + } > > + > > + char *our_tzones = xstrdup(our_chassis_tzones); > > + char *i, *j; > > + > > + for (i = strsep(&our_tzones, ","); i != NULL; > > + i = strsep(&our_tzones, ",")) { > > + > > + char *tzones = xstrdup(chassis_tzones); > > + for (j = strsep(&tzones, ","); j != NULL; > > + j = strsep(&tzones, ",")) { > > + > > + if (!strcmp(i, j)) { > > + return true; > > + } > > + } > > + } > > You're leaking our_tzones and tzones. our_tzones needs to be freed > outside the outer for loop, and tzones needs to be freed at the end of > the outer for loop. > > The problem is that you can't just free(our_tzones) and free(tzones) > because strsep will have changed them from where they were originally > pointing when allocated. Passing them as-is to free() will cause bad things. > > The easiest way to fix this is to declare an extra pointer that points > to where the string was originally allocated: > > 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))) { > ... > } > free(tzones_orig); > } > free(our_tzones_orig); > > (You don't have to convert to a while loop here. That's just a personal > preference of mine when working with strsep.) > Thanks for the explanation and snippet, I found it very useful. My C is a bit rusty at the moment. > > + > > + return false; > > +} > > + > > 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_chassis *our_chassis, > > Can you explain the rationale for this change? > Yes, since the encaps_run() function now needs more than the ID of the Chassis (we need access to the "external_ids" attribute to get the transport zones) and ovn-controller (the code that calls encaps_run()) already had the "sbrec_chassis" allocated, I thought it would be handier to pass that instead of passing the ID and then fetching the Chassis from the database. But, as you rightly pointed out in the comment below, maybe this approach have some undesirable effect. > > const struct sbrec_sb_global *sbg) > > { > > - if (!ovs_idl_txn || !br_int) { > > + if (!ovs_idl_txn || !br_int || !our_chassis) { > > return; > > } > > This change has some knock-on effects that I'm not sure I'm comfortable > with. For instance, this will skip deleting existing OVN tunnels that > are not around any more. Is this OK? > In my tests it seemed fine because and all tests were passing. But I do understand your concerns here. I was trying to avoid having to fetch the Chassis from the database since we already had it allocated by the ovn-controller.c code (the code that calls encaps_run()). But, maybe that's not the right thing to do because as you said, if the "our_chassis" struct is NULL the whole loop will be skipped. In my next patch set I will revert this change and instead fetch and use the Chassis from database only in the part of the function where we need it. > > > > const struct sbrec_chassis *chassis_rec; > > const struct ovsrec_bridge *br; > > + const char *our_chassis_tzones = smap_get_def( > > + &our_chassis->external_ids, "ovn-transport-zones", ""); > > > > struct tunnel_ctx tc = { > > .chassis = SHASH_INITIALIZER(&tc.chassis), > > @@ -219,7 +251,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > tc.ovs_txn = ovs_idl_txn; > > ovsdb_idl_txn_add_comment(tc.ovs_txn, > > "ovn-controller: modifying OVS tunnels '%s'", > > - chassis_id); > > + our_chassis->name); > > > > /* Collect all port names into tc.port_names. > > * > > @@ -250,8 +282,20 @@ 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. */ > > + > > + if (strcmp(chassis_rec->name, our_chassis->name)) { > > Since you are passing a chassis record instead of a chassis_id, you can > save the string comparison and instead do a direct pointer comparison. > > if (chassis_rec != our_chassis) { > > The only reason this would be semantically different is if chassis > records are allowed to share the same name. They are not allowed to do > that, though. > Good point! Yeah, the "name" column is being indexed [0] so we will not allow duplicated values. I will update the code. [0] https://github.com/openvswitch/ovs/blob/170ef7265a36b3716b1bdc6fd067e9dd7f38a133/ovn/ovn-sb.ovsschema#L42 > > + /* 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 (!check_chassis_tzones(our_chassis_tzones, 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..71ff78b5e 100644 > > --- a/ovn/controller/encaps.h > > +++ b/ovn/controller/encaps.h > > @@ -23,6 +23,7 @@ struct ovsdb_idl_txn; > > struct ovsrec_bridge; > > struct ovsrec_bridge_table; > > struct sbrec_chassis_table; > > +struct sbrec_chassis; > > struct sbrec_sb_global; > > struct ovsrec_open_vswitch_table; > > > > @@ -31,7 +32,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > const struct ovsrec_bridge_table *, > > const struct ovsrec_bridge *br_int, > > const struct sbrec_chassis_table *, > > - const char *chassis_id, > > + const struct sbrec_chassis *, > > const struct sbrec_sb_global *); > > > > bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, > > 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..421bd0acb 100644 > > --- a/ovn/controller/ovn-controller.c > > +++ b/ovn/controller/ovn-controller.c > > @@ -696,7 +696,7 @@ main(int argc, char *argv[]) > > encaps_run( > > ovs_idl_txn, > > ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int, > > - sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id, > > + sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis, > > sbrec_sb_global_first(ovnsb_idl_loop.idl)); > > > > if (ofctrl_is_connected()) { > > 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 > > + > > +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 > > > Again, thanks Mark for the review, tips and snippets. I will change the code, test it and upload a new version soon. Cheers, Lucas
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", ""); 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..072be4ac2 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -195,20 +195,52 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_s return tuncnt; } +/* + * Check if both chassis are in the same transport zone. + */ +static bool +check_chassis_tzones(const char *our_chassis_tzones, + const char *chassis_tzones) +{ + if (!strcmp(our_chassis_tzones, "") && !strcmp(chassis_tzones, "")) { + return true; + } + + char *our_tzones = xstrdup(our_chassis_tzones); + char *i, *j; + + for (i = strsep(&our_tzones, ","); i != NULL; + i = strsep(&our_tzones, ",")) { + + char *tzones = xstrdup(chassis_tzones); + for (j = strsep(&tzones, ","); j != NULL; + j = strsep(&tzones, ",")) { + + if (!strcmp(i, j)) { + return true; + } + } + } + + return false; +} + 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_chassis *our_chassis, const struct sbrec_sb_global *sbg) { - if (!ovs_idl_txn || !br_int) { + if (!ovs_idl_txn || !br_int || !our_chassis) { return; } const struct sbrec_chassis *chassis_rec; const struct ovsrec_bridge *br; + const char *our_chassis_tzones = smap_get_def( + &our_chassis->external_ids, "ovn-transport-zones", ""); struct tunnel_ctx tc = { .chassis = SHASH_INITIALIZER(&tc.chassis), @@ -219,7 +251,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, tc.ovs_txn = ovs_idl_txn; ovsdb_idl_txn_add_comment(tc.ovs_txn, "ovn-controller: modifying OVS tunnels '%s'", - chassis_id); + our_chassis->name); /* Collect all port names into tc.port_names. * @@ -250,8 +282,20 @@ 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. */ + + if (strcmp(chassis_rec->name, our_chassis->name)) { + /* 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 (!check_chassis_tzones(our_chassis_tzones, 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..71ff78b5e 100644 --- a/ovn/controller/encaps.h +++ b/ovn/controller/encaps.h @@ -23,6 +23,7 @@ struct ovsdb_idl_txn; struct ovsrec_bridge; struct ovsrec_bridge_table; struct sbrec_chassis_table; +struct sbrec_chassis; struct sbrec_sb_global; struct ovsrec_open_vswitch_table; @@ -31,7 +32,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_bridge_table *, const struct ovsrec_bridge *br_int, const struct sbrec_chassis_table *, - const char *chassis_id, + const struct sbrec_chassis *, const struct sbrec_sb_global *); bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, 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..421bd0acb 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -696,7 +696,7 @@ main(int argc, char *argv[]) encaps_run( ovs_idl_txn, ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int, - sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id, + sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis, sbrec_sb_global_first(ovnsb_idl_loop.idl)); if (ofctrl_is_connected()) { 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 + +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