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 > + 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
