| Message ID | 20210916173028.9527-1-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: configure only matching encaps between chassis | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 9/16/21 7:30 PM, Vladislav Odintsov wrote: > Previously tunnels encap on a chassis was based on the remote chassis > "best" encap type. Suppose we have 2 chassis: one with STT, another with > GENEVE. In this case on chassis 1 there was configured STT tunnel, and > GENEVE on another one. No traffic could be sent between these chassis. > > With this approach it was impossible to change encap type > for chassis one-by-one, because different tunnel types were > configured on different edges of the link. > Suppose we have 2 chassis: one with STT and VXLAN configured encaps, > another with GENEVE and STT. In > this case on chassis 1 there was configured STT tunnel (best of VXLAN > and STT) and GENEVE on another one ("best" of GENEVE and STT). > No traffic could be sent between these chassis. Though the common STT > could be used. > > Now we configure only matching encaps between nodes. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- Hi Vladislav, > controller/encaps.c | 23 ++++++++++++++++++----- > controller/ovn-controller.h | 3 ++- > tests/ovn-controller.at | 14 +++++++++++--- > 3 files changed, 31 insertions(+), 9 deletions(-) > > diff --git a/controller/encaps.c b/controller/encaps.c > index fc93bf1ee..200b4405d 100644 > --- a/controller/encaps.c > +++ b/controller/encaps.c > @@ -248,15 +248,26 @@ exit: > smap_destroy(&options); > } > > +static bool > +chassis_has_type(const struct sbrec_chassis *chassis, uint32_t tun_type) { > + for (int i =0; i < chassis->n_encaps; i++) { Nit: missing space after '=' but I guess this can be fixed at apply time, probably no need for a v2. > + if (get_tunnel_type(chassis->encaps[i]->type) == tun_type) { > + return true; > + } > + } > + return false; > +} > + > struct sbrec_encap * > -preferred_encap(const struct sbrec_chassis *chassis_rec) > +preferred_encap(const struct sbrec_chassis *chassis_rec, > + const struct sbrec_chassis *this_chassis) > { > struct sbrec_encap *best_encap = NULL; > uint32_t best_type = 0; > > for (int i = 0; i < chassis_rec->n_encaps; i++) { > uint32_t tun_type = get_tunnel_type(chassis_rec->encaps[i]->type); > - if (tun_type > best_type) { > + if (tun_type > best_type && chassis_has_type(this_chassis, tun_type)) { > best_type = tun_type; > best_encap = chassis_rec->encaps[i]; > } > @@ -270,9 +281,11 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) > * as there are VTEP of that type (differentiated by remote_ip) on that chassis. > */ > static int > -chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc) > +chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, > + const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc, > + const struct sbrec_chassis *this_chassis) > { > - struct sbrec_encap *encap = preferred_encap(chassis_rec); > + struct sbrec_encap *encap = preferred_encap(chassis_rec, this_chassis); > int tuncnt = 0; > > if (!encap) { > @@ -390,7 +403,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > continue; > } > > - if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { > + if (chassis_tunnel_add(chassis_rec, sbg, &tc, this_chassis) == 0) { > VLOG_INFO("Creating encap for '%s' failed", chassis_rec->name); > continue; > } > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > index 78a53312f..ea48a36cb 100644 > --- a/controller/ovn-controller.h > +++ b/controller/ovn-controller.h > @@ -43,7 +43,8 @@ struct ct_zone_pending_entry { > const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, > const char *br_name); > > -struct sbrec_encap *preferred_encap(const struct sbrec_chassis *); > +struct sbrec_encap *preferred_encap(const struct sbrec_chassis *, > + const struct sbrec_chassis *); Not really related to your change, but this function might as well be static in encaps.c. Acked-by: Dumitru Ceara <dceara@redhat.com> Regards, Dumitru
On Mon, Sep 20, 2021 at 6:16 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 9/16/21 7:30 PM, Vladislav Odintsov wrote: > > Previously tunnels encap on a chassis was based on the remote chassis > > "best" encap type. Suppose we have 2 chassis: one with STT, another with > > GENEVE. In this case on chassis 1 there was configured STT tunnel, and > > GENEVE on another one. No traffic could be sent between these chassis. > > > > With this approach it was impossible to change encap type > > for chassis one-by-one, because different tunnel types were > > configured on different edges of the link. > > Suppose we have 2 chassis: one with STT and VXLAN configured encaps, > > another with GENEVE and STT. In > > this case on chassis 1 there was configured STT tunnel (best of VXLAN > > and STT) and GENEVE on another one ("best" of GENEVE and STT). > > No traffic could be sent between these chassis. Though the common STT > > could be used. > > > > Now we configure only matching encaps between nodes. > > > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > --- > > Hi Vladislav, > > > controller/encaps.c | 23 ++++++++++++++++++----- > > controller/ovn-controller.h | 3 ++- > > tests/ovn-controller.at | 14 +++++++++++--- > > 3 files changed, 31 insertions(+), 9 deletions(-) > > > > diff --git a/controller/encaps.c b/controller/encaps.c > > index fc93bf1ee..200b4405d 100644 > > --- a/controller/encaps.c > > +++ b/controller/encaps.c > > @@ -248,15 +248,26 @@ exit: > > smap_destroy(&options); > > } > > > > +static bool > > +chassis_has_type(const struct sbrec_chassis *chassis, uint32_t tun_type) { > > + for (int i =0; i < chassis->n_encaps; i++) { > > Nit: missing space after '=' but I guess this can be fixed at apply > time, probably no need for a v2. > > > + if (get_tunnel_type(chassis->encaps[i]->type) == tun_type) { > > + return true; > > + } > > + } > > + return false; > > +} > > + > > struct sbrec_encap * > > -preferred_encap(const struct sbrec_chassis *chassis_rec) > > +preferred_encap(const struct sbrec_chassis *chassis_rec, > > + const struct sbrec_chassis *this_chassis) > > { > > struct sbrec_encap *best_encap = NULL; > > uint32_t best_type = 0; > > > > for (int i = 0; i < chassis_rec->n_encaps; i++) { > > uint32_t tun_type = get_tunnel_type(chassis_rec->encaps[i]->type); > > - if (tun_type > best_type) { > > + if (tun_type > best_type && chassis_has_type(this_chassis, tun_type)) { > > best_type = tun_type; > > best_encap = chassis_rec->encaps[i]; > > } > > @@ -270,9 +281,11 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) > > * as there are VTEP of that type (differentiated by remote_ip) on that chassis. > > */ > > static int > > -chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc) > > +chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, > > + const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc, > > + const struct sbrec_chassis *this_chassis) > > { > > - struct sbrec_encap *encap = preferred_encap(chassis_rec); > > + struct sbrec_encap *encap = preferred_encap(chassis_rec, this_chassis); > > int tuncnt = 0; > > > > if (!encap) { > > @@ -390,7 +403,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > continue; > > } > > > > - if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { > > + if (chassis_tunnel_add(chassis_rec, sbg, &tc, this_chassis) == 0) { > > VLOG_INFO("Creating encap for '%s' failed", chassis_rec->name); > > continue; > > } > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > > index 78a53312f..ea48a36cb 100644 > > --- a/controller/ovn-controller.h > > +++ b/controller/ovn-controller.h > > @@ -43,7 +43,8 @@ struct ct_zone_pending_entry { > > const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, > > const char *br_name); > > > > -struct sbrec_encap *preferred_encap(const struct sbrec_chassis *); > > +struct sbrec_encap *preferred_encap(const struct sbrec_chassis *, > > + const struct sbrec_chassis *); > > Not really related to your change, but this function might as well be > static in encaps.c. > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Vladislav and Dumitru. I applied this patch to the main branch and to branch-21.09 with the below changes (addressing Dumitru's comments) ---------------- diff --git a/controller/encaps.c b/controller/encaps.c index e4a1d17bc2..66e0cd8cd0 100644 --- a/controller/encaps.c +++ b/controller/encaps.c @@ -263,7 +263,7 @@ exit: static bool chassis_has_type(const struct sbrec_chassis *chassis, uint32_t tun_type) { - for (int i =0; i < chassis->n_encaps; i++) { + for (size_t i = 0; i < chassis->n_encaps; i++) { if (get_tunnel_type(chassis->encaps[i]->type) == tun_type) { return true; } @@ -271,14 +271,14 @@ chassis_has_type(const struct sbrec_chassis *chassis, uint32_t tun_type) { return false; } -struct sbrec_encap * +static struct sbrec_encap * preferred_encap(const struct sbrec_chassis *chassis_rec, const struct sbrec_chassis *this_chassis) { struct sbrec_encap *best_encap = NULL; uint32_t best_type = 0; - for (int i = 0; i < chassis_rec->n_encaps; i++) { + for (size_t i = 0; i < chassis_rec->n_encaps; i++) { uint32_t tun_type = get_tunnel_type(chassis_rec->encaps[i]->type); if (tun_type > best_type && chassis_has_type(this_chassis, tun_type)) { best_type = tun_type; diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h index ea48a36cb0..df28c62cb7 100644 --- a/controller/ovn-controller.h +++ b/controller/ovn-controller.h @@ -43,9 +43,6 @@ struct ct_zone_pending_entry { const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, const char *br_name); -struct sbrec_encap *preferred_encap(const struct sbrec_chassis *, - const struct sbrec_chassis *); - uint32_t get_tunnel_type(const char *name); struct pb_ld_binding { --------------- Thanks Numan > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/encaps.c b/controller/encaps.c index fc93bf1ee..200b4405d 100644 --- a/controller/encaps.c +++ b/controller/encaps.c @@ -248,15 +248,26 @@ exit: smap_destroy(&options); } +static bool +chassis_has_type(const struct sbrec_chassis *chassis, uint32_t tun_type) { + for (int i =0; i < chassis->n_encaps; i++) { + if (get_tunnel_type(chassis->encaps[i]->type) == tun_type) { + return true; + } + } + return false; +} + struct sbrec_encap * -preferred_encap(const struct sbrec_chassis *chassis_rec) +preferred_encap(const struct sbrec_chassis *chassis_rec, + const struct sbrec_chassis *this_chassis) { struct sbrec_encap *best_encap = NULL; uint32_t best_type = 0; for (int i = 0; i < chassis_rec->n_encaps; i++) { uint32_t tun_type = get_tunnel_type(chassis_rec->encaps[i]->type); - if (tun_type > best_type) { + if (tun_type > best_type && chassis_has_type(this_chassis, tun_type)) { best_type = tun_type; best_encap = chassis_rec->encaps[i]; } @@ -270,9 +281,11 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) * as there are VTEP of that type (differentiated by remote_ip) on that chassis. */ static int -chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc) +chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, + const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc, + const struct sbrec_chassis *this_chassis) { - struct sbrec_encap *encap = preferred_encap(chassis_rec); + struct sbrec_encap *encap = preferred_encap(chassis_rec, this_chassis); int tuncnt = 0; if (!encap) { @@ -390,7 +403,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, continue; } - if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { + if (chassis_tunnel_add(chassis_rec, sbg, &tc, this_chassis) == 0) { VLOG_INFO("Creating encap for '%s' failed", chassis_rec->name); continue; } diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h index 78a53312f..ea48a36cb 100644 --- a/controller/ovn-controller.h +++ b/controller/ovn-controller.h @@ -43,7 +43,8 @@ struct ct_zone_pending_entry { const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, const char *br_name); -struct sbrec_encap *preferred_encap(const struct sbrec_chassis *); +struct sbrec_encap *preferred_encap(const struct sbrec_chassis *, + const struct sbrec_chassis *); uint32_t get_tunnel_type(const char *name); diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 4ae218ed6..3d27ddd2f 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -263,9 +263,8 @@ check_tunnel_property () { test "`ovs-vsctl get interface ovn-fakech-0 $1`" = "$2" } -# Start off with a remote chassis supporting STT -ovn-sbctl chassis-add fakechassis stt 192.168.0.2 -OVS_WAIT_UNTIL([check_tunnel_property type stt]) +# create "empty" chassis. vxlan is used here as a stub +ovn-sbctl chassis-add fakechassis vxlan 192.168.0.2 # See if we switch to Geneve as the first choice when it is available # With multi-VTEP support we support tunnels with different IPs to the @@ -278,6 +277,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type stt]) encap_uuid=$(ovn-sbctl add chassis fakechassis encaps @encap -- --id=@encap create encap type=geneve ip="192.168.0.2") OVS_WAIT_UNTIL([check_tunnel_property type geneve]) +# change geneve to stt and check that tun interface was deleted and there is +# no stt encap on the second chassis, only vxlan +ovn-sbctl set encap ${encap_uuid} type=stt +OVS_WAIT_WHILE([check_tunnel_property type stt]) +OVS_WAIT_UNTIL([check_tunnel_property type vxlan]) + +# change back to geneve +ovn-sbctl set encap ${encap_uuid} type=geneve + # Check that changes within an encap row are propagated ovn-sbctl set encap ${encap_uuid} ip=192.168.0.2 OVS_WAIT_UNTIL([check_tunnel_property options:remote_ip "\"192.168.0.2\""])
Previously tunnels encap on a chassis was based on the remote chassis "best" encap type. Suppose we have 2 chassis: one with STT, another with GENEVE. In this case on chassis 1 there was configured STT tunnel, and GENEVE on another one. No traffic could be sent between these chassis. With this approach it was impossible to change encap type for chassis one-by-one, because different tunnel types were configured on different edges of the link. Suppose we have 2 chassis: one with STT and VXLAN configured encaps, another with GENEVE and STT. In this case on chassis 1 there was configured STT tunnel (best of VXLAN and STT) and GENEVE on another one ("best" of GENEVE and STT). No traffic could be sent between these chassis. Though the common STT could be used. Now we configure only matching encaps between nodes. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- controller/encaps.c | 23 ++++++++++++++++++----- controller/ovn-controller.h | 3 ++- tests/ovn-controller.at | 14 +++++++++++--- 3 files changed, 31 insertions(+), 9 deletions(-)