[ovs-dev] [PATCH v2] OVN: Add support for Transport Zones
Lucas Alvares Gomes Martins
lmartins at redhat.com
Thu Mar 28 10:03:38 UTC 2019
On Thu, Mar 28, 2019 at 9:19 AM Numan Siddique <nusiddiq at redhat.com> 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 <mmichels at redhat.com> wrote:
>>
>> Thanks Lucas, looks good to me.
>>
>> Acked-by: Mark Michelson <mmichels at redhat.com>
>>
>> On 3/25/19 2:24 PM, lmartins at redhat.com wrote:
>> > From: Lucas Alvares Gomes <lucasagomes at 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 at redhat.com>
>> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html
>> > Signed-off-by: Lucas Alvares Gomes <lucasagomes at 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.
>
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.
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.
> 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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list