| Message ID | 20240720051648.3332549-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Support ovn-encap-ip-default option. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Sat, Jul 20, 2024 at 1:24 AM Han Zhou <hzhou@ovn.org> 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 <hzhou@ovn.org> > --- > 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 <numans@ovn.org> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Aug 7, 2024 at 8:39 PM Numan Siddique <numans@ovn.org> wrote: > > On Sat, Jul 20, 2024 at 1:24 AM Han Zhou <hzhou@ovn.org> 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 <hzhou@ovn.org> > > --- > > 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 <numans@ovn.org> > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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; + } + 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 ])
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 <hzhou@ovn.org> --- 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(-)