@@ -2156,7 +2156,11 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
lbinding = local_binding_create(iface_id, iface_rec);
local_binding_add(local_bindings, lbinding);
} else {
- lbinding->iface = iface_rec;
+ if (lbinding->iface != iface_rec) {
+ lbinding->iface = iface_rec;
+ lbinding->count++;
+ b_ctx_out->local_lports_changed = true;
+ }
}
struct binding_lport *b_lport =
@@ -2219,13 +2223,21 @@ static bool
consider_iface_release(const struct ovsrec_interface *iface_rec,
const char *iface_id,
struct binding_ctx_in *b_ctx_in,
- struct binding_ctx_out *b_ctx_out)
+ struct binding_ctx_out *b_ctx_out,
+ int *count)
{
struct local_binding *lbinding;
struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
+ int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
lbinding = local_binding_find(local_bindings, iface_id);
+ if (lbinding && lbinding->iface != iface_rec && !ofport) {
+ VLOG_DBG("Not releasing lport %s as %s was claimed "
+ "and %s was never bound)",
+ iface_id, lbinding->iface->name, iface_rec->name);
+ return true;
+ }
struct binding_lport *b_lport =
local_binding_get_primary_or_localport_lport(lbinding);
if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
@@ -2254,6 +2266,7 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
}
if (lbinding) {
+ *count = lbinding->count - 1;
/* Clear the iface of the local binding. */
lbinding->iface = NULL;
}
@@ -2375,6 +2388,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
bool handled = true;
+ struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
+ struct hmap *qos_map_ptr =
+ sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
+
/* Run the tracked interfaces loop twice. One to handle deleted
* changes. And another to handle add/update changes.
* This will ensure correctness.
@@ -2435,8 +2452,57 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
}
if (cleared_iface_id) {
+ int count = 0;
handled = consider_iface_release(iface_rec, cleared_iface_id,
- b_ctx_in, b_ctx_out);
+ b_ctx_in, b_ctx_out, &count);
+ if (!handled) {
+ break;
+ }
+ if (!count) {
+ continue;
+ }
+ /* There is another OVS interface bound to this port.
+ * First find its name
+ */
+ VLOG_DBG("%d binding(s) remaining for %s",
+ count, cleared_iface_id);
+ struct smap_node *node;
+ bool found = false;
+ SMAP_FOR_EACH (node, b_ctx_out->local_iface_ids) {
+ if (strcmp(node->value, cleared_iface_id) ||
+ !strcmp(node->key, iface_rec->name)) {
+ continue;
+ }
+ /* Then find its iface_rec */
+ char *other_iface_rec_name = node->key;
+ const struct ovsrec_interface *other_iface_rec;
+ OVSREC_INTERFACE_TABLE_FOR_EACH (other_iface_rec,
+ b_ctx_in->iface_table) {
+ if (!strcmp(other_iface_rec->name, other_iface_rec_name)) {
+ found = true;
+ break;
+ }
+ }
+ if (found) {
+ int64_t ofport = other_iface_rec->n_ofport ?
+ *other_iface_rec->ofport : 0;
+ if (cleared_iface_id && ofport > 0 &&
+ is_iface_in_int_bridge(other_iface_rec,
+ b_ctx_in->br_int)) {
+ handled = consider_iface_claim(other_iface_rec,
+ cleared_iface_id,
+ b_ctx_in,
+ b_ctx_out, qos_map_ptr);
+ }
+ }
+ break;
+ }
+ if (!found) {
+ /* This is unexpected. Recompute to clean up */
+ VLOG_WARN("Did not find which OVS interface still bound to %s",
+ cleared_iface_id);
+ handled = false;
+ }
}
if (!handled) {
@@ -2448,13 +2514,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
/* This can happen if any non vif OVS interface is in the tracked
* list or if consider_iface_release() returned false.
* There is no need to process further. */
+ destroy_qos_map(&qos_map);
return false;
}
- struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
- struct hmap *qos_map_ptr =
- sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
-
/*
* We consider an OVS interface for claiming if the following
* 2 conditions are met:
@@ -3034,6 +3097,7 @@ local_binding_create(const char *name, const struct ovsrec_interface *iface)
struct local_binding *lbinding = xzalloc(sizeof *lbinding);
lbinding->name = xstrdup(name);
lbinding->iface = iface;
+ lbinding->count = 1;
ovs_list_init(&lbinding->binding_lports);
return lbinding;
@@ -135,6 +135,7 @@ struct local_binding {
char *name;
const struct ovsrec_interface *iface;
struct ovs_list binding_lports;
+ int count;
};
struct local_binding_data {
@@ -32975,3 +32975,109 @@ check ovn-nbctl --wait=hv sync
OVN_CLEANUP([hv1])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovn-appctl vlog/set dbg
+
+get_flows()
+{
+ cookie=$1
+ ovs-ofctl dump-flows br-int | grep $cookie |
+ sed -e 's/duration=[[0-9.]]*s, //g' |
+ sed -e 's/idle_age=[[0-9]]*, //g' |
+ sed -e 's/n_packets=[[0-9]]*, //g' |
+ sed -e 's/n_bytes=[[0-9]]*, //g'
+}
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls lp
+check ovn-nbctl lsp-set-type lp localport
+check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2"
+
+check ovn-nbctl lsp-add ls vm1
+check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11"
+check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal external_ids:iface-id=vm1
+
+check ovn-nbctl --wait=hv sync
+
+check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal
+check ovs-vsctl set interface lpold external_ids:iface-id=lp
+
+OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=lp) != x])
+echo ======================================================
+echo === Flows after iface-id set for the old interface ===
+echo ======================================================
+COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//')
+
+OVS_WAIT_UNTIL([
+ ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold)
+ ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport"
+])
+nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
+echo $nb_flows "flows after iface-id set for old interface"
+
+echo ======================================================
+echo === Flows after iface-id set for the new interface ===
+echo ======================================================
+check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal
+check ovs-vsctl set interface lpnew external_ids:iface-id=lp
+OVS_WAIT_UNTIL([
+ ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew)
+ ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport"
+])
+check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l)
+flows_lpnew=$(get_flows $COOKIE)
+
+echo ======================================================
+echo ======= Flows after old interface is deleted =========
+echo ======================================================
+check ovs-vsctl del-port br-int lpold
+# We do not expect changes, so let's wait a few seconds to check nothing changed
+sleep 2
+check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l)
+flows_after_deletion=$(get_flows $COOKIE)
+check test "$flows_lpnew" = "$flows_after_deletion"
+
+echo ======================================================
+echo ======= Flows after lptemp interface is created ====
+echo ======================================================
+check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal
+check ovs-vsctl set Interface lptemp external_ids:iface-id=lp
+OVS_WAIT_UNTIL([
+ ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp)
+ ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport"
+])
+check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l)
+
+echo ======================================================
+echo ======= Flows after lptemp interface is deleted ======
+echo ======================================================
+check ovs-vsctl del-port br-int lptemp
+OVS_WAIT_UNTIL([
+ ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew)
+ ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport"
+])
+check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l)
+flows_after_deletion=$(get_flows $COOKIE)
+check test "$flows_lpnew" = "$flows_after_deletion"
+
+echo ======================================================
+echo ======= Flows after new interface is deleted =========
+echo ======================================================
+check ovs-vsctl del-port br-int lpnew
+OVS_WAIT_UNTIL([
+ nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
+ test "${nb_flows}" = 0
+])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
In the following scenario: - interface "old" is created and external_ids:iface-id is set (to lp) - interface "new" is created and external_ids:iface-id is set (to same lp) - interface "old" is deleted flows related to lp were deleted. Note that after "new" interface is created, flows use "new" ofport. This patch only supports up to two ovs interfaces bound to the same iface-id. Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output engine.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/binding.c | 78 ++++++++++++++++++++++++++++--- controller/binding.h | 1 + tests/ovn.at | 106 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 7 deletions(-)