On Wed, Aug 7, 2024 at 8:39 PM Numan Siddique <[email protected]> wrote:
>
> On Sat, Jul 20, 2024 at 1:24 AM Han Zhou <[email protected]> wrote:
> >
> > When there are multiple encap IPs configured for a chassis, there are
> > situations that any of the IP may be used, e.g. when encap-ip is not
> > configured for a VIF or when the output port of the pipeline is not
> > a VIF but a chassis-redirect port. In such cases, the encap IP used is
> > unpredictable. This patch introduces the ovn-encap-ip-default option,
> > allowing the configuration of a default IP to be used to ensure
> > deterministic encap IP selection in such cases.
> >
> > Signed-off-by: Han Zhou <[email protected]>
> > ---
> > NEWS | 3 +++
> > controller/binding.c | 13 +++++-----
> > controller/chassis.c | 46 ++++++++++++++++++++++++++++++---
> > controller/ovn-controller.8.xml | 6 +++++
> > ovn-sb.xml | 10 +++++++
> > tests/ovn.at | 37 ++++++++++++++++++++++++--
> > 6 files changed, 103 insertions(+), 12 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 3e392ff08b5a..b23977a9316d 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -38,6 +38,9 @@ Post v24.03.0
> > ability to disable "VXLAN mode" to extend available tunnel IDs space
> > for
> > datapaths from 4095 to 16711680. For more details see man ovn-nb(5)
> > for
> > mentioned option.
> > + - Add "external_ids:ovn-encap-ip-default" config for ovn-controller to
> > + determine the default encap IP when there are multiple encap IPs
> > + configured.
> >
> > OVN v24.03.0 - 01 Mar 2024
> > --------------------------
> > diff --git a/controller/binding.c b/controller/binding.c
> > index b7e7f48749b8..146b03248f14 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -520,20 +520,21 @@ static struct sbrec_encap *
> > sbrec_get_port_encap(const struct sbrec_chassis *chassis_rec,
> > const struct ovsrec_interface *iface_rec)
> > {
> > -
> > - if (!iface_rec) {
> > + if (chassis_rec->n_encaps < 2) {
> > return NULL;
> > }
> >
> > - const char *encap_ip = smap_get(&iface_rec->external_ids, "encap-ip");
> > - if (!encap_ip) {
> > - return NULL;
> > + const char *encap_ip = NULL;
> > + if (iface_rec) {
> > + encap_ip = smap_get(&iface_rec->external_ids, "encap-ip");
> > }
> >
> > struct sbrec_encap *best_encap = NULL;
> > uint32_t best_type = 0;
> > for (int i = 0; i < chassis_rec->n_encaps; i++) {
> > - if (!strcmp(chassis_rec->encaps[i]->ip, encap_ip)) {
> > + if ((encap_ip && !strcmp(chassis_rec->encaps[i]->ip, encap_ip)) ||
> > + (!encap_ip && smap_get_bool(&chassis_rec->encaps[i]->options,
> > + "is_default", false))) {
> > uint32_t tun_type =
> > get_tunnel_type(chassis_rec->encaps[i]->type);
> > if (tun_type > best_type) {
> > best_type = tun_type;
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 4942ba281d66..7fe38efd1159 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -63,6 +63,8 @@ struct ovs_chassis_cfg {
> > struct sset encap_type_set;
> > /* Set of encap IPs parsed from the 'ovn-encap-ip' external-id. */
> > struct sset encap_ip_set;
> > + /* Default encap IP when there are two or more encap IPs. Optional. */
> > + const char *encap_ip_default;
> > /* Interface type list formatted in the OVN-SB Chassis required
> > format. */
> > struct ds iface_types;
> > /* Is this chassis an interconnection gateway. */
> > @@ -281,6 +283,7 @@ chassis_parse_ovs_config(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > const struct ovsrec_bridge *br_int,
> > struct ovs_chassis_cfg *ovs_cfg)
> > {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > const struct ovsrec_open_vswitch *cfg =
> > ovsrec_open_vswitch_table_first(ovs_table);
> >
> > @@ -298,7 +301,6 @@ chassis_parse_ovs_config(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > "ovn-encap-ip", NULL);
> > if (!encap_type || !encap_ips) {
> > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > VLOG_INFO_RL(&rl, "Need to specify an encap type and ip");
> > return false;
> > }
> > @@ -333,6 +335,16 @@ chassis_parse_ovs_config(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > * multiple NICs and is assigning SR-IOV VFs to a guest (as logical
> > ports).
> > */
> > chassis_parse_ovs_encap_ip(encap_ips, &ovs_cfg->encap_ip_set);
> > + const char *encap_ip_default =
> > + get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > + "ovn-encap-ip-default", NULL);
> > + if (encap_ip_default &&
> > + !sset_contains(&ovs_cfg->encap_ip_set, encap_ip_default)) {
> > + VLOG_INFO_RL(&rl, "ovn-encap-ip-default (%s) must be one of the
> > IPs "
> > + "in ovn-encap-ip.", encap_ip_default);
> > + return false;
> > + }
>
> Hi Han,
>
> Thanks for the patch.
>
> Lets say CMS has configured ovn-encap-ip = [127.0.0.1, 127.0.0.2, 127.0.0.3]
> and ovn-encap-ip-default = 127.0.0.4.
>
> As default encap-ip is not part of the ovn-encap-ip list,
> chassis_run() will always return NULL.
> And after that, ovn-controller will not bind any logical port and the
> I-P engine doesn't run
> until the config option is set to a valid value.
>
> I understand this is a wrong configuration from the user. But since
> this config is optional, I think we
> should log a warning and continue normally. What do you think ?
>
> Other than that, the patch LGTM.
>
> If you agree with my comment to ignore the wrong default ip:
> Acked-by: Numan Siddique <[email protected]>
>
Thanks Numan. Your suggestion makes sense. I applied to main with
below minor change as you suggested:
---
diff --git a/controller/chassis.c b/controller/chassis.c
index fab5109a2f14..2991a0af32c8 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -342,9 +342,9 @@ chassis_parse_ovs_config(const struct
ovsrec_open_vswitch_table *ovs_table,
"ovn-encap-ip-default", NULL);
if (encap_ip_default &&
!sset_contains(&ovs_cfg->encap_ip_set, encap_ip_default)) {
- VLOG_INFO_RL(&rl, "ovn-encap-ip-default (%s) must be one of the IPs "
+ VLOG_WARN_RL(&rl, "ovn-encap-ip-default (%s) must be one of the IPs "
"in ovn-encap-ip.", encap_ip_default);
- return false;
+ encap_ip_default = NULL;
}
ovs_cfg->encap_ip_default = encap_ip_default;
---
Thanks,
Han
> Thanks
> Numan
>
> > + ovs_cfg->encap_ip_default = encap_ip_default;
> >
> > chassis_parse_ovs_iface_types(cfg->iface_types, cfg->n_iface_types,
> > &ovs_cfg->iface_types);
> > @@ -534,6 +546,7 @@ chassis_other_config_changed(const struct
> > ovs_chassis_cfg *ovs_cfg,
> > static bool
> > chassis_tunnels_changed(const struct sset *encap_type_set,
> > const struct sset *encap_ip_set,
> > + const char *encap_ip_default,
> > const char *encap_csum,
> > const struct sbrec_chassis *chassis_rec)
> > {
> > @@ -562,6 +575,19 @@ chassis_tunnels_changed(const struct sset
> > *encap_type_set,
> > changed = true;
> > break;
> > }
> > +
> > + if (smap_get_bool(&chassis_rec->encaps[i]->options,
> > + "is_default", false)) {
> > + if (!encap_ip_default || strcmp(encap_ip_default,
> > + chassis_rec->encaps[i]->ip)) {
> > + changed = true;
> > + break;
> > + }
> > + } else if (encap_ip_default && !strcmp(encap_ip_default,
> > +
> > chassis_rec->encaps[i]->ip)) {
> > + changed = true;
> > + break;
> > + }
> > }
> >
> > if (!changed) {
> > @@ -593,6 +619,7 @@ static struct sbrec_encap **
> > chassis_build_encaps(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > const struct sset *encap_type_set,
> > const struct sset *encap_ip_set,
> > + const char *encap_ip_default,
> > const char *chassis_id,
> > const char *encap_csum,
> > size_t *n_encap)
> > @@ -613,7 +640,15 @@ chassis_build_encaps(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> >
> > sbrec_encap_set_type(encap, encap_type);
> > sbrec_encap_set_ip(encap, encap_ip);
> > - sbrec_encap_set_options(encap, &options);
> > + if (encap_ip_default && !strcmp(encap_ip_default, encap_ip)) {
> > + struct smap _options;
> > + smap_clone(&_options, &options);
> > + smap_add(&_options, "is_default", "true");
> > + sbrec_encap_set_options(encap, &_options);
> > + smap_destroy(&_options);
> > + } else {
> > + sbrec_encap_set_options(encap, &options);
> > + }
> > sbrec_encap_set_chassis_name(encap, chassis_id);
> >
> > encaps[tunnel_count] = encap;
> > @@ -748,7 +783,9 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
> > /* If any of the encaps should change, update them. */
> > bool tunnels_changed =
> > chassis_tunnels_changed(&ovs_cfg->encap_type_set,
> > - &ovs_cfg->encap_ip_set,
> > ovs_cfg->encap_csum,
> > + &ovs_cfg->encap_ip_set,
> > + ovs_cfg->encap_ip_default,
> > + ovs_cfg->encap_csum,
> > chassis_rec);
> > if (!tunnels_changed) {
> > return updated;
> > @@ -759,7 +796,8 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
> >
> > encaps =
> > chassis_build_encaps(ovnsb_idl_txn, &ovs_cfg->encap_type_set,
> > - &ovs_cfg->encap_ip_set, chassis_id,
> > + &ovs_cfg->encap_ip_set,
> > + ovs_cfg->encap_ip_default, chassis_id,
> > ovs_cfg->encap_csum, &n_encap);
> > sbrec_chassis_set_encaps(chassis_rec, encaps, n_encap);
> > free(encaps);
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index 6dad00eb146f..89f08843fcbc 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -204,6 +204,12 @@
> > </p>
> > </dd>
> >
> > + <dt><code>external_ids:ovn-encap-ip-default</code></dt>
> > + <dd>
> > + When <code>ovn-encap-ip</code> contains multiple IPs, this field
> > + indicates the default one.
> > + </dd>
> > +
> > <dt><code>external_ids:ovn-encap-df_default</code></dt>
> > <dd>
> > indicates the DF flag handling of the encapulation. Set to
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 73a1be5ed9b8..66d03bd7cdec 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -562,6 +562,16 @@
> > </p>
> > </column>
> >
> > + <column name="options" key="is_default" type='{"type": "boolean"}'>
> > + <p>
> > + When there are multiple encaps for a chassis with different IPs,
> > this
> > + option indicates if the encap is the default one that matches the
> > IP in
> > + <ref table="Open_vSwitch"
> > column="external_ids:ovn-encap-ip-default"/>
> > + column of the Open_vSwitch database's <ref table="Open_vSwitch"
> > + db="Open_vSwitch"/> table.
> > + </p>
> > + </column>
> > +
> > <column name="ip">
> > The IPv4 address of the encapsulation tunnel endpoint.
> > </column>
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index adc7cb2c8fc6..8250589814aa 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -30133,8 +30133,21 @@ check_packet_tunnel() {
> > local dst_mac=f0:00:00:00:88:01 # lrp-ls1's MAC
> > local src_ip=$(vif_to_ip vif$src)
> > local dst_ip=$(vif_to_ip vif$dst)
> > - local local_encap_ip=192.168.0.$src
> > - local remote_encap_ip=192.168.0.$dst
> > +
> > + local local_encap_ip
> > + if test -n "$3"; then
> > + local_encap_ip=$3
> > + else
> > + local_encap_ip=192.168.0.$src
> > + fi
> > +
> > + local remote_encap_ip
> > + if test -n "$4"; then
> > + remote_encap_ip=$4
> > + else
> > + remote_encap_ip=192.168.0.$dst
> > + fi
> > +
> > local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
> > IP(dst='${dst_ip}', src='${src_ip}')/ \
> > ICMP(type=8)")
> > @@ -30151,6 +30164,26 @@ for i in 1 2; do
> > done
> > done
> >
> > +# Set default encap-ip and remove VIF's encap-ip settings. Packets should
> > go
> > +# through default encap-ip.
> > +
> > +for i in 1 2; do
> > + as hv$i
> > + check ovs-vsctl set open .
> > external_ids:ovn-encap-ip-default=192.168.0.${i}2
> > +
> > + for j in 1 2; do
> > + check ovs-vsctl remove Interface vif$i$j external_ids encap-ip
> > + done
> > +done
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +for i in 1 2; do
> > + for j in 1 2; do
> > + check_packet_tunnel 1$i 2$j 192.168.0.12 192.168.0.22
> > + done
> > +done
> > +
> > OVN_CLEANUP([hv1],[hv2])
> > AT_CLEANUP
> > ])
> > --
> > 2.38.1
> >
> > _______________________________________________
> > 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
