Skip to content

Commit

Permalink
ovn-controller: Fixed missing flows after interface deletion
Browse files Browse the repository at this point in the history 
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.
The state where old and new interfaces have both external_ids:iface-id set at
the same time is "invalid", and all flows are not installed for lpold.

Fixes: 3ae8470 ("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>
Acked-by: Han Zhou <hzhou@ovn.org>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from commit 52cb9c3)
  • Loading branch information
@simonartxavier @numansiddique
simonartxavier authored and numansiddique committed Nov 23, 2022
1 parent 6fbdf81 commit 2283713
Show file tree
Hide file tree
Showing 3 changed files with 200 additions and 12 deletions.
43 changes: 31 additions & 12 deletions controller/binding.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1622,6 +1622,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
lbinding = local_binding_create(iface_id, iface_rec);
local_binding_add(local_bindings, lbinding);
} else {
lbinding->multiple_bindings = true;
static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(
Expand Down Expand Up @@ -1888,6 +1889,10 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
lbinding = local_binding_create(iface_id, iface_rec);
local_binding_add(local_bindings, lbinding);
} else {
if (lbinding->iface && lbinding->iface != iface_rec) {
lbinding->multiple_bindings = true;
b_ctx_out->local_lports_changed = true;
}
lbinding->iface = iface_rec;
}

Expand All @@ -1906,6 +1911,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
return true;
}

/* If multiple bindings to the same port, remove the "old" binding.
* This ensures that change tracking is correct.
*/
if (lbinding->multiple_bindings) {
remove_related_lport(pb, b_ctx_out);
}

enum en_lport_type lport_type = get_lport_type(pb);
if (lport_type == LP_LOCALPORT) {
return consider_localport(pb, b_ctx_in, b_ctx_out);
Expand Down Expand Up @@ -1960,18 +1972,24 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
lbinding = local_binding_find(local_bindings, iface_id);

if (lbinding) {
int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
if (lbinding->iface != iface_rec && !ofport) {
/* If external_ids:iface-id is set within the same transaction
* as adding an interface to a bridge, ovn-controller is
* usually initially notified of ovs interface changes with
* ofport == 0. If the lport was bound to a different interface
* we do not want to release it.
*/
VLOG_DBG("Not releasing lport %s as %s was claimed "
"and %s was never bound)", iface_id, lbinding->iface ?
lbinding->iface->name : "", iface_rec->name);
return true;
if (lbinding->multiple_bindings) {
VLOG_INFO("Multiple bindings for %s: force recompute to clean up",
iface_id);
return false;
} else {
int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
if (lbinding->iface != iface_rec && !ofport) {
/* If external_ids:iface-id is set within the same transaction
* as adding an interface to a bridge, ovn-controller is
* usually initially notified of ovs interface changes with
* ofport == 0. If the lport was bound to a different interface
* we do not want to release it.
*/
VLOG_DBG("Not releasing lport %s as %s was claimed "
"and %s was never bound)", iface_id, lbinding->iface ?
lbinding->iface->name : "", iface_rec->name);
return true;
}
}
}

Expand Down Expand Up @@ -2745,6 +2763,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->multiple_bindings = false;
ovs_list_init(&lbinding->binding_lports);

return lbinding;
Expand Down
1 change: 1 addition & 0 deletions controller/binding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ struct local_binding {
char *name;
const struct ovsrec_interface *iface;
struct ovs_list binding_lports;
bool multiple_bindings;
};


Expand Down
168 changes: 168 additions & 0 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30477,3 +30477,171 @@ check ovn-nbctl --wait=hv sync
OVN_CLEANUP([hv1])
AT_CLEANUP
])

m4_define([MULTIPLE_OVS_INT],
[OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port ($1)])
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

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
if test X$1 != X; then
check ovn-nbctl lsp-set-type lp $1
fi
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 ======================================================
# Set external_ids:iface-id within same transaction as adding the port.
# This will generally cause ovn-controller to get initially notified of ovs interface changes with ofport == 0.
check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal -- 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 for controller to get time to process any update
check ovn-nbctl --wait=hv sync
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 ======================================================
# Set external_ids:iface-id in a different transaction as adding the port.
# This will generally cause ovn-controller to get notified of ovs interface changes with a proper ofport.
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)
echo $ofport
ovs-ofctl dump-flows br-int | grep $COOKIE
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
])

echo ======================================================
echo ======= Three interfaces bound to the same port ======
echo ======================================================
check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal
check ovs-vsctl set interface lpold external_ids:iface-id=lp
check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal
check ovs-vsctl set interface lpnew external_ids:iface-id=lp

# Wait for lpnew flows to be installed
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"
])
flows_lpnew=$(get_flows $COOKIE)
nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`

check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal
check ovs-vsctl set Interface lptemp external_ids:iface-id=lp

# Wait for lptemp flows to be installed
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"
])

# Delete both lpold and lptemp to go to a stable situation
check ovs-vsctl del-port br-int lptemp
check ovs-vsctl del-port br-int lpold

OVS_WAIT_UNTIL([
test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l)
])

# Wait for correct/lpnew flows to be installed
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"

# Check that recompute still works
check ovn-appctl -t ovn-controller recompute
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"

OVN_CLEANUP([hv1])
AT_CLEANUP
])])

MULTIPLE_OVS_INT([localport])
MULTIPLE_OVS_INT([])

0 comments on commit 2283713

Please sign in to comment.